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.

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

Remove method TR_J9VMBase::findOrCreateClassAndDepthFlagsSymbolRef #20397

Conversation

hzongaro
Copy link
Member

A method named findOrCreateClassDepthAndFlagsSymbolRef was introduced to TR_J9VMBase by a previous commit to correct the typo in the name of the method, findOrCreateClassAndDepthFlagsSymbolRef. That change kept the old method due to upstream uses in J9_PROJECT_SPECIFIC code in OMR.

This change replaces a call to findOrCreateClassAndDepthFlagsSymbolRef that was recently introduced with a call to
TR_J9VMBase::testIsClassArrayType instead, to hide the manipulation of the classDepthAndFlags field.

As that was the last remaining use of
findOrCreateClassAndDepthFlagsSymbolRef, this change also removes the now obsolete method from TR_J9VMBase.

This is follow on to pull request #19558 which first introduced TR_J9VMBase::findOrCreateClassDepthAndFlagsSymbolRef to replace TR_J9VMBase::findOrCreateClassAndDepthFlagsSymbolRef.

A method named findOrCreateClassDepthAndFlagsSymbolRef was introduced to
TR_J9VMBase by a previous commit to correct the typo in the name of the
method, findOrCreateClassAndDepthFlagsSymbolRef.  That change kept the
old method due to upstream uses in J9_PROJECT_SPECIFIC code in OMR.

This change replaces a call to findOrCreateClassAndDepthFlagsSymbolRef
that was recently introduced with a call to
TR_J9VMBase::testIsClassArrayType instead, to hide the manipulation of
the classDepthAndFlags field.

As that was the last remaining use of
findOrCreateClassAndDepthFlagsSymbolRef, this change also removes the
now obsolete method from TR_J9VMBase.

Signed-off-by:  Henry Zongaro <[email protected]>
@hzongaro
Copy link
Member Author

As this changes code that is guarded by #if defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION), I am running an internal personal build with that enabled to ensure that these changes will not break anything.

@hzongaro hzongaro requested review from 0xdaryl and removed request for 0xdaryl October 24, 2024 12:32
@hzongaro hzongaro marked this pull request as ready for review October 24, 2024 12:32
@hzongaro
Copy link
Member Author

This pull request follows on from pull request #19558 which first introduced findOrCreateClassDepthAndFlagsSymbolRef as a replacement to findOrCreateClassAndDepthFlagsSymbolRef.

The functional part of the change is guarded by the preprocessor directive

#if defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION)

As that would not be tested by regular PR testing, I ran an internal personal build with the help of @rmnattas who verified that there didn't seem to be any unexpected failures, and that the IL that was generated with this change appeared to be correct.

@0xdaryl, @rmnattas, may I ask you both to review this change?

Copy link
Contributor

@rmnattas rmnattas left a comment

Choose a reason for hiding this comment

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

LGTM

@hzongaro hzongaro requested a review from 0xdaryl October 24, 2024 17:35
@0xdaryl
Copy link
Contributor

0xdaryl commented Oct 29, 2024

Jenkins test sanity all jdk21

@0xdaryl 0xdaryl self-assigned this Oct 29, 2024
Copy link
Contributor

@0xdaryl 0xdaryl left a comment

Choose a reason for hiding this comment

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

The x86 Linux tests appear to have passed even though the status in the PR isn't updated. The Z Linux failure does not appear to be one that could be caused by this PR, and has appeared intermittently for several years. I will merge this.

@0xdaryl 0xdaryl merged commit 6fd3a8c into eclipse-openj9:master Oct 29, 2024
24 of 27 checks passed
@hzongaro hzongaro deleted the remove-findOrCreateClassAndDepthFlagsSymbolRef branch October 29, 2024 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants