Refactor vTable Access #2409

fengxue-IS · 2018-07-17T18:10:11Z

add vTable Header structure
update vtable indexing to be zero based
modify vtable size field to hold the real method count
add macro for getting JIT vtable start address
switch all vtable variable assignment to use new macros
calculate vtable offset dynamically based on header size
changed default vtable type to <J9Method**>
removed old comments on magic indexes

Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

fengxue-IS · 2018-07-17T18:14:08Z

@gacholio Can you please take a look?

gacholio · 2018-07-17T18:25:14Z

runtime/oti/j9.h

@@ -301,4 +301,12 @@ static const struct { \
 #define J9_VM_FUNCTION_VIA_JAVAVM(javaVM, function) ((javaVM)->internalVMFunctions->function)
 #endif /* J9_INTERNAL_TO_VM */

+#define J9VTABLE_HEADER_FROM_RAM_CLASS(clazz) ((J9VTableHeader *)((J9Class *)clazz + 1))
+#define J9VTABLE_FROM_HEADER(vtableHeader) ((UDATA *) ((J9VTableHeader *)vtableHeader + 1))


This should probably be J9Method** and a bunch of the casting could be avoided.

gacholio · 2018-07-17T18:25:58Z

runtime/oti/j9.h

+#define J9VTABLE_HEADER_FROM_RAM_CLASS(clazz) ((J9VTableHeader *)((J9Class *)clazz + 1))
+#define J9VTABLE_FROM_HEADER(vtableHeader) ((UDATA *) ((J9VTableHeader *)vtableHeader + 1))
+#define J9VTABLE_FROM_RAM_CLASS(clazz) J9VTABLE_FROM_HEADER(J9VTABLE_HEADER_FROM_RAM_CLASS(clazz))
+#define J9VTABLE_OFFSET_FROM_INDEX(index) (sizeof(J9Class) + sizeof(J9VTableHeader) + index * sizeof(UDATA))


Please bracket the multiplication. Also index itself needs bracketting.

gacholio · 2018-07-17T18:26:21Z

runtime/oti/j9.h

+#define J9VTABLE_OFFSET_FROM_INDEX(index) (sizeof(J9Class) + sizeof(J9VTableHeader) + index * sizeof(UDATA))
+
+/* Skip 1 slot (the vTable size slot) */
+#define JIT_VTABLE_START_ADDRESS(clazz) ((UDATA *)clazz - (sizeof(J9VTableHeader) / sizeof(UDATA)))


clazz needs bracketting.

gacholio · 2018-07-17T18:26:42Z

runtime/compiler/control/HookedByTheJit.cpp

-      J9Method ** superVTable = (J9Method **)((char *)superCl + sizeof(J9Class));   // a pointer to an array of j9method pointers (the vtable!)
-      J9Method ** subVTable = (J9Method **)((char *)cl + sizeof(J9Class));
+      J9VTableHeader * superVTableHeader = J9VTABLE_HEADER_FROM_RAM_CLASS(superCl);
+      intptrj_t methodCount =  (intptrj_t)superVTableHeader->size;      //Vtable starts at end of j9class. First entry is num methods in vtable


The comment here no longer applies.

gacholio · 2018-07-17T18:28:39Z

runtime/oti/j9nonbuilder.h

@@ -3454,6 +3454,11 @@ typedef struct J9ITable {
 	struct J9ITable* next;
 } J9ITable;

+typedef struct J9VTableHeader {
+	UDATA size;
+	UDATA* initialVirtualMethod;


Might as well be J9Method*

gacholio · 2018-07-17T18:29:59Z

runtime/util/hshelp.c


 	/* Search the vTable for a public method of the correct name. */
-	for (searchIndex = 2; searchIndex <= vTableSize; searchIndex++) {
-		J9Method *vTableMethod = (J9Method *)vTable[searchIndex];
+	for (searchIndex; searchIndex < vTableSize; searchIndex++) {


searchIndex; does nothing. It would be much clearer to initialize it to 0 again here.

gacholio · 2018-07-17T18:30:59Z

runtime/util/hshelp.c


-		for (i = 2; i <= vTableSize; i++) {
+		/* Skip the first two slots containing vtable size and the resolve method */


comment no longer applies.

gacholio · 2018-07-17T18:32:17Z

runtime/oti/j9.h

+#define J9VTABLE_FROM_RAM_CLASS(clazz) J9VTABLE_FROM_HEADER(J9VTABLE_HEADER_FROM_RAM_CLASS(clazz))
+#define J9VTABLE_OFFSET_FROM_INDEX(index) (sizeof(J9Class) + sizeof(J9VTableHeader) + index * sizeof(UDATA))
+
+/* Skip 1 slot (the vTable size slot) */


Comment is incorrect - update to say that is skips the header.

gacholio · 2018-07-17T18:34:45Z

runtime/util/resolvehelp.c

@@ -68,14 +68,15 @@ getMethodForSpecialSend(J9VMThread *vmStruct, J9Class *currentClass, J9Class *re
 					 * 3) Walk the current class vtable backwards to find the most "recent" override of that method
 					 * 4) Lookup the method at vtableIndex in currentClass's super class
 					 */
-					UDATA superVTableSize = (*(UDATA*)(superclass + 1) * sizeof(UDATA)) + sizeof(J9Class);
+					J9VTableHeader * superVTable = J9VTABLE_HEADER_FROM_RAM_CLASS(superclass);
+					UDATA superVTableEnd =J9VTABLE_OFFSET_FROM_INDEX(superVTable->size);


space after =

gacholio · 2018-07-17T18:36:40Z

runtime/vm/createramclass.cpp

-				while (tempIndex > 1) {
+				while (tempIndex > 0) {
+					/* Decrement the index, convert from one based to zero based index */
+					tempIndex--;


For single statements, -= 1 is preferred.

gacholio · 2018-07-17T18:38:47Z

runtime/vm/createramclass.cpp

 			/* + 1 to account for size slot */
-			memcpy(vTableAddress, superVTable, (vTableWriteIndex + 1) * sizeof(UDATA));
+			memcpy(vTableAddress, superVTable, (vTableWriteIndex * sizeof(UDATA) + sizeof(J9VTableHeader)));


Bracket multiply

gacholio · 2018-07-17T18:41:46Z

runtime/vm/createramclass.cpp

 				superclassVTableIndex--;
+				*/


Keep the comment, but remove the commented out code

gacholio · 2018-07-17T18:42:26Z

runtime/vm/createramclass.cpp

 	/* fill in vtable, add new entry */
 	vTableAddress[vTableWriteIndex] = (UDATA)storeValue;
+	/* move up the vTableWriteIndex */


Please remove this comment.

fengxue-IS · 2018-07-17T19:58:03Z

fixed comments and type casting with J9Method **

fengxue-IS · 2018-07-18T17:10:30Z

@gacholio fix all the issues in your comments, can I get a re-review?

gacholio · 2018-07-18T18:37:08Z

runtime/oti/j9.h

@@ -301,4 +301,12 @@ static const struct { \
 #define J9_VM_FUNCTION_VIA_JAVAVM(javaVM, function) ((javaVM)->internalVMFunctions->function)
 #endif /* J9_INTERNAL_TO_VM */

+#define J9VTABLE_HEADER_FROM_RAM_CLASS(clazz) ((J9VTableHeader *)((J9Class *)(clazz) + 1))
+#define J9VTABLE_FROM_HEADER(vtableHeader) ((J9Method **) ((J9VTableHeader *)(vtableHeader) + 1))


Both of these could use some extra brackets to isolate the cast from the +1, since either interpretation is syntactically valid, but have wildly different results.

- add vTable Header structure - update vtable indexing to be zero based - modify vtable size field to hold the real method count - add macro for getting JIT vtable start address - switch all vtable variable assignment to use new macros - calculate vtable offset dynamically based on header size - changed default vtable type to <J9Method**> - removed old comments on magic indexes Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

gacholio · 2018-07-18T18:44:04Z

jenkins test sanity xlinux jdk8

Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>

Fix warning introduced in #2409

fengxue-IS force-pushed the vtable2 branch from 39ad1e3 to f1a2e39 Compare July 17, 2018 18:12

gacholio reviewed Jul 17, 2018

View reviewed changes

pshipton added the comp:vm label Jul 17, 2018

gacholio reviewed Jul 17, 2018

View reviewed changes

runtime/vm/createramclass.cpp Outdated

superclassVTableIndex--;

*/

Copy link

Contributor

gacholio Jul 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the comment, but remove the commented out code

gacholio reviewed Jul 17, 2018

View reviewed changes

fengxue-IS force-pushed the vtable2 branch 2 times, most recently from 15f5328 to 284d3c4 Compare July 17, 2018 19:56

fengxue-IS force-pushed the vtable2 branch 2 times, most recently from ae31513 to c6d8d89 Compare July 17, 2018 21:41

gacholio reviewed Jul 18, 2018

View reviewed changes

fengxue-IS force-pushed the vtable2 branch from c6d8d89 to a7efe30 Compare July 18, 2018 18:40

fengxue-IS force-pushed the vtable2 branch from a7efe30 to 19c371d Compare July 18, 2018 18:43

gacholio approved these changes Jul 18, 2018

View reviewed changes

gacholio self-assigned this Jul 18, 2018

gacholio merged commit be4ecff into eclipse-openj9:master Jul 18, 2018

gacholio added a commit to gacholio/openj9 that referenced this pull request Jul 18, 2018

Fix warning introduced in eclipse-openj9#2409

7ee0e3e

Signed-off-by: Graham Chapman <graham_chapman@ca.ibm.com>

gacholio added a commit that referenced this pull request Jul 18, 2018

Merge pull request #2429 from gacholio/warnfix

4dd67e3

Fix warning introduced in #2409

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor vTable Access #2409

Refactor vTable Access #2409

fengxue-IS commented Jul 17, 2018 •

edited

Loading

fengxue-IS commented Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

gacholio Jul 17, 2018

fengxue-IS commented Jul 17, 2018

fengxue-IS commented Jul 18, 2018

gacholio Jul 18, 2018

gacholio commented Jul 18, 2018


		for (i = 2; i <= vTableSize; i++) {
		/* Skip the first two slots containing vtable size and the resolve method */

Refactor vTable Access #2409

Refactor vTable Access #2409

Conversation

fengxue-IS commented Jul 17, 2018 • edited Loading

fengxue-IS commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fengxue-IS commented Jul 17, 2018

fengxue-IS commented Jul 18, 2018

Choose a reason for hiding this comment

gacholio commented Jul 18, 2018

fengxue-IS commented Jul 17, 2018 •

edited

Loading