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

Add CT helper to check for ChangesCurrentThread annotation #18222

Merged
merged 2 commits into from Oct 4, 2023

Conversation

nbhuiyan
Copy link
Member

@nbhuiyan nbhuiyan commented Oct 2, 2023

This is required for implementing the JIT inlining behaviour expected for the methods annotated by @ChangesCurrentThread.

Issue: #17519

@nbhuiyan
Copy link
Member Author

nbhuiyan commented Oct 2, 2023

@tajila I'd appreciate your review.

Signed-off-by: Nazim Bhuiyan <nubhuiyan@ibm.com>
@tajila
Copy link
Contributor

tajila commented Oct 2, 2023

Is the result of that call going to be cached? You dont want to call this repeatedly since it needs to parse the method attributes

@nbhuiyan
Copy link
Member Author

nbhuiyan commented Oct 3, 2023

The result of the call would not get cached as there will be no more than one call per method being considered for inlining during a compilation, similar to @ForceInline and @DontInline which also dictate inlining behaviour. Caching is also not done for any of the other annotation helpers.

@tajila
Copy link
Contributor

tajila commented Oct 3, 2023

jenkins compile win jdk8

@tajila
Copy link
Contributor

tajila commented Oct 3, 2023

jenkins test sanity alinux64 jdk21

@@ -29,6 +29,10 @@

extern "C" {

#if JAVA_SPEC_VERSION >= 20
Copy link
Contributor

@0xdaryl 0xdaryl Oct 3, 2023

Choose a reason for hiding this comment

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

Minor nit, maybe a question for @tajila.

Should this be >= 21 since JDK 20 is no longer available and we no longer build it? Or do we specify it this way because that's when the annotation was first introduced and its good for bookkeeping purposes? I'm fine either way, just curious.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, the annotation appears in JDK 19 as well it seems. So if we're back-leveling the JDKs for bookkeeping purposes then it should be at least JDK 19.

Copy link
Contributor

Choose a reason for hiding this comment

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

For new code, we typically don't account for out-of-support releases. So we can ignore jdk19 and jdk20.

Copy link
Member Author

@nbhuiyan nbhuiyan Oct 3, 2023

Choose a reason for hiding this comment

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

I changed the JAVA_SPEC_VERSION check to 21. Once the tests you launched pass, I can squash and rebase.

JAVA_SPEC_VERSION checks for new code should only be for
what is currently supported.

Signed-off-by: Nazim Bhuiyan <nubhuiyan@ibm.com>
@tajila
Copy link
Contributor

tajila commented Oct 3, 2023

jenkins test sanity alinux64 jdk21

@nbhuiyan
Copy link
Member Author

nbhuiyan commented Oct 4, 2023

The aarch64 test failure:

---TEST RESULTS---
Number of PASSED tests: 10 out of 11
Number of FAILED tests: 1 out of 11

---SUMMARY OF FAILED TESTS---
Check SSL Verbose Log for connection failure with Non SSL Server
-----------------------------

-----------------------------------
cmdLineTester_criu_jitserverPostRestore_1_FAILED

This is not related to the changes in this PR.

@tajila
Copy link
Contributor

tajila commented Oct 4, 2023

LGTM

@0xdaryl any concerns?

@tajila tajila merged commit 8abe35a into eclipse-openj9:master Oct 4, 2023
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants