Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor VTable index/offset variable name #2436

Merged
merged 1 commit into from
Jul 21, 2018
Merged

Refactor VTable index/offset variable name #2436

merged 1 commit into from
Jul 21, 2018

Conversation

fengxue-IS
Copy link
Contributor

@fengxue-IS fengxue-IS commented Jul 19, 2018 •
edited
Loading

  • Change variable name to correctly reflect vtable index/name
  • Added new constant for InitialVirtualMethod offset
  • change initialVirtual constant to use offsetof() macro
  • change convertITableIndexToVTableIndex to convertITableIndexToVTableOffset
  • change jitGetInterfaceVTableIndexFromCP to jitGetInterfaceVTableOffsetFromCP
  • change getVTableIndexForMethod to getVTableOffsetForMethod

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

@fengxue-IS fengxue-IS force-pushed the vtable3 branch 2 times, most recently from 3155bc0 to 8e6dda8 Compare July 19, 2018 23:01
@fengxue-IS
Copy link
Contributor Author

@gacholio can you take a look?

@gacholio gacholio self-requested a review July 20, 2018 04:11
@gacholio gacholio self-assigned this Jul 20, 2018
gacholio
gacholio reviewed Jul 20, 2018
View reviewed changes
runtime/codert_vm/cnathelp.cpp Outdated
@@ -990,12 +990,12 @@ old_slow_jitLookupInterfaceMethod(J9VMThread *currentThread)
UDATA cpIndex = indexAndLiteralsEA[-1];
J9Class *interfaceClass = ((J9Class**)indexAndLiteralsEA)[0];
UDATA methodIndex = indexAndLiteralsEA[1];
UDATA vTableIndex = VM_VMHelpers::convertITableIndexToVTableIndex(currentThread, receiverClass, interfaceClass, methodIndex, ramConstantPool, cpIndex);
UDATA vTableOffset = VM_VMHelpers::convertITableIndexToVTableIndex(currentThread, receiverClass, interfaceClass, methodIndex, ramConstantPool, cpIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably rename index to offset in the helper function as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

The change to this function name was good - it's not the native.

gacholio
gacholio reviewed Jul 20, 2018
View reviewed changes
runtime/jit_vm/cthelpers.cpp Outdated
}

J9Method*
jitGetInterfaceMethodFromCP(J9VMThread *currentThread, J9ConstantPool *constantPool, UDATA cpIndex, J9Class* lookupClass)
{
UDATA vTableIndex = jitGetInterfaceVTableIndexFromCP(currentThread, constantPool, cpIndex, lookupClass);
UDATA vTableOffset = jitGetInterfaceVTableIndexFromCP(currentThread, constantPool, cpIndex, lookupClass);
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, function rename to reflect offset would be good.

gacholio
gacholio reviewed Jul 20, 2018
View reviewed changes
runtime/oti/j9.h Outdated
#define J9VTABLE_HEADER_FROM_RAM_CLASS(clazz) ((J9VTableHeader *)(((J9Class *)(clazz)) + 1))
#define J9VTABLE_FROM_HEADER(vtableHeader) ((J9Method **)(((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)))

/* VTable constants offset */
#define J9VTABLE_INITIAL_VIRTUAL_OFFSET (sizeof(J9Class) + sizeof(UDATA))
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be expressed as VTABLE_HEADER + offsetof(the field).

Copy link
Contributor

Choose a reason for hiding this comment

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

rather, "(sizeof(J9Class) + offsetof(J9VTableHeader, initialVirtualMethod))"

@gacholio
Copy link
Contributor

Once again, looks very good with a few nitpicks. Consider changing (UDATA)-1 to UDATA_MAX, and adding an assert to make sure that's never a successful return value.

@gacholio
Copy link
Contributor

I'm unsure about the JIT/JCL changes (including the renaming of the VM-side native) at the moment - please remove those until we can discuss with the JIT team.

@gacholio
Copy link
Contributor

re last comment - it's just the native I'm concerned about, the other changes in the JIT look fine.

@fengxue-IS
Copy link
Contributor Author

I've removed the renaming for native methods, also fixed the ddr vtable access functions

@gacholio
Copy link
Contributor

Too much deleted! I like the function renames to Offset instead of Index - it's just the natives (and JCL caller) that should not be changed.

gacholio
gacholio requested changes Jul 20, 2018
View reviewed changes
debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9VTablesCommand.java Outdated
UDATAPointer jitVTable = UDATAPointer.NULL;
UDATA vTableMethodCount;
UDATAPointer jitVTable = UDATAPointer.NULL;
J9VTableHeaderPointer vTableHeader;
Copy link
Contributor

Choose a reason for hiding this comment

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

inconsistent spacing here

debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/commands/J9VTablesCommand.java Outdated
vTableSlotCount = vTable.at(0);
vTableHeader = J9ClassHelper.vTableHeader(ramClass);
vTableMethodCount = vTableHeader.size();
vTable = J9ClassHelper.vTable(vTableHeader);
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing

runtime/codert_vm/cnathelp.cpp Outdated
@@ -990,12 +990,12 @@ old_slow_jitLookupInterfaceMethod(J9VMThread *currentThread)
UDATA cpIndex = indexAndLiteralsEA[-1];
J9Class *interfaceClass = ((J9Class**)indexAndLiteralsEA)[0];
UDATA methodIndex = indexAndLiteralsEA[1];
UDATA vTableIndex = VM_VMHelpers::convertITableIndexToVTableIndex(currentThread, receiverClass, interfaceClass, methodIndex, ramConstantPool, cpIndex);
UDATA vTableOffset = VM_VMHelpers::convertITableIndexToVTableIndex(currentThread, receiverClass, interfaceClass, methodIndex, ramConstantPool, cpIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

The change to this function name was good - it's not the native.

@fengxue-IS
Copy link
Contributor Author

fixed code spacing, re-added non-jcl name changes

@fengxue-IS fengxue-IS force-pushed the vtable3 branch 3 times, most recently from 81ab3ff to a998fb3 Compare July 21, 2018 00:59
@fengxue-IS
Refactor VTable index/offset variable name
eab7ce3 
- Change variable name to correctly reflect vtable index/name
- Added new constant for InitialVirtualMethod offset
- change initialVirtual constant to use offsetof() macro
- change convertITableIndexToVTableIndex to convertITableIndexToVTableOffset
- change jitGetInterfaceVTableIndexFromCP to jitGetInterfaceVTableOffsetFromCP
- change getVTableIndexForMethod to getVTableOffsetForMethod

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@gacholio
Copy link
Contributor

jenkins test sanity plinux jdk8

@gacholio gacholio merged commit 11d24ba into eclipse-openj9:master Jul 21, 2018
@gacholio gacholio mentioned this pull request Jul 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gacholio gacholio gacholio approved these changes

Assignees

@gacholio gacholio

Labels
comp:vm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@fengxue-IS @gacholio @DanHeidinga