Skip to content

Add var handle method type lookup table walking #18976

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.

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

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

cjjdespres
Copy link
Contributor

@cjjdespres cjjdespres commented Feb 18, 2024

The content of the varHandleMethodTypeLookupTable is now walked in allSlotsInROMClassDo.

The relevant section of the ROM class writer code is here, I believe:

#if defined(J9VM_OPT_METHOD_HANDLE)
void writeVarHandleMethodTypeLookupTable()
{
if (!_markAndCountOnly) {
U_16 lookupTableCount = _constantPoolMap->getVarHandleMethodTypeCount();
if (lookupTableCount > 0) {
U_16 *lookupTable = _constantPoolMap->getVarHandleMethodTypeLookupTable();
/* The lookup table is a U_16[], so we need to pad it in order to keep the J9ROMClass alignment. */
U_16 lookupTablePaddedCount = _constantPoolMap->getVarHandleMethodTypePaddedCount();
U_16 *lookupTablePadding = lookupTable + lookupTableCount;
UDATA lookupTablePaddingLength = lookupTablePaddedCount - lookupTableCount;
/* Write the data section of the array */
_cursor->writeData((U_8 *)lookupTable, lookupTableCount * sizeof(U_16), Cursor::GENERIC);
/* Write the padding section of the array */
if (lookupTablePaddingLength > 0) {
_cursor->writeData((U_8 *)lookupTablePadding, lookupTablePaddingLength * sizeof(U_16), Cursor::GENERIC);
}
}
}
}
#endif /* defined(J9VM_OPT_METHOD_HANDLE) */

I think the individual slots are "cpIndex", but I'm unsure. I also made up the new section name "varHandleMethodTypeLookupTable".

I also updated some of the documentation surrounding JITServerHelpers::packROMClass.

Fixes: #18957

@cjjdespres cjjdespres requested a review from dsouzai as a code owner February 18, 2024 00:07
@cjjdespres cjjdespres force-pushed the romclass-var-table-walk branch from 3983eca to ec93ddb Compare February 18, 2024 00:08
@cjjdespres
Copy link
Contributor Author

Attn @mpirvu. I think @tajila suggested someone to review my previous ROM class walking PRs. Thanks.

@mpirvu mpirvu added the comp:vm label Feb 18, 2024
@mpirvu
Copy link
Contributor

mpirvu commented Feb 18, 2024

@babsingh Could you please review this PR or delegate? Thanks

@mpirvu
Copy link
Contributor

mpirvu commented Feb 18, 2024

jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk11

@mpirvu
Copy link
Contributor

mpirvu commented Feb 18, 2024

jenkins test sanity.openjdk,extended.openjdk xlinuxjit,plinuxjit,zlinuxjit jdk11

@mpirvu
Copy link
Contributor

mpirvu commented Feb 19, 2024

The assert reported in #18957 is no longer seen with this fix

@babsingh
Copy link
Contributor

I think the individual slots are "cpIndex", but I'm unsure.

Your changes are correct. varHandleMethodTypeLookupTable stores CP indices for VarHandle methodrefs. Processing them similar to the adjacent allSlotsIn*MethodRefIndexesDo functions is the correct approach.

@babsingh
Copy link
Contributor

Commit message should be strengthened by adding why these changes are needed and what is fixed. Currently, the commit message only has a title.

The var handle method type lookup table is now walked in
allSlotsInROMClassDo(). This section of a J9ROMClass, written by
ROMClassWriter::writeVarHandleMethodTypeLookupTable() if
J9VM_OPT_METHOD_HANDLE is active, was previously not visited by the ROM
class walker. In addition to being necessary in general to walk this
section for the sake of completeness, walking this ROM class section is
necessary in particular so that JITServerHelpers::packROMClass() can
have access to all ROM class sections and their sizes. Not walking every
section of the ROM class led to a fatal assert being triggered in
packROMClass in the related issue.

Fixes: eclipse-openj9#18957

Signed-off-by: Christian Despres <[email protected]>
@cjjdespres cjjdespres force-pushed the romclass-var-table-walk branch from ec93ddb to 703bea7 Compare February 20, 2024 13:57
@cjjdespres
Copy link
Contributor Author

Force-pushed to address comments.

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

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

LGTM. Old PR builds should still be good since the recent changes don't impact functionality. A JIT server file is also modified; I will defer the merge to a JIT member.

Old PR builds:

@mpirvu
Copy link
Contributor

mpirvu commented Feb 20, 2024

A JIT server file is also modified; I will defer the merge to a JIT member.

The JITServer file only contains comments. This PR should be good to merge.

@babsingh babsingh merged commit a4af783 into eclipse-openj9:master Feb 20, 2024
@keithc-ca
Copy link
Contributor

@hangshao0 Should the shared class cache version have been updated with this change?

Should debugtools/DDR_VM/src/com/ibm/j9ddr/vm29/tools/ddrinteractive/RomClassWalker.java not have been updated similarly?

@hangshao0
Copy link
Contributor

@hangshao0 Should the shared class cache version have been updated with this change?

Is the layout of the rom class changed ? If yes, the shared cache version should be updated.

@cjjdespres
Copy link
Contributor Author

cjjdespres commented Feb 20, 2024

The ROM class layout is unchanged. Only the ROM class walker in romclasswalk.c was changed, to walk an already-existing section that the walker had been missing.

@keithc-ca
Copy link
Contributor

Ok, it would seem to be missing in DDR as well. Is someone working to correct that?

@cjjdespres
Copy link
Contributor Author

Not that I know of. I wasn't aware of RomClassWalker.java until you mentioned it. I'm not familiar with the Java side of ROM class walking.

@babsingh
Copy link
Contributor

Ok, it would seem to be missing in DDR as well. Is someone working to correct that?

I will take a look into it. It is associated to OpenJ9 MethodHandles, which we are trying to replace with OpenJDK MethodHandles in JDK8/JDK11. Since we plan to disable OJ9 MHs, this will take low priority.

@keithc-ca
Copy link
Contributor

Even after we stop using OpenJ9 method handles, the DDR part will be useful for examining core files produced by VMs that predate the adoption of openjdk method handles.

babsingh added a commit to babsingh/openj9 that referenced this pull request Feb 23, 2024
babsingh added a commit to babsingh/openj9 that referenced this pull request Mar 18, 2024
r30shah pushed a commit to r30shah/openj9 that referenced this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants