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

JIT compile AccessController.doPrivileged in JDK 24+ #20859

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

theresa-m
Copy link
Contributor

@theresa-m theresa-m commented Dec 19, 2024

AccessController.doPrivileged methods have historically
not been compiled by the JIT due to security manager use.
Starting in JDK 24 the security manager will be removed
and these methods can be compiled.

Related: #20563

@theresa-m
Copy link
Contributor Author

@dsouzai can you take a look at this change or advise on the best person to review?

@dsouzai
Copy link
Contributor

dsouzai commented Jan 13, 2025

@hzongaro could you review this change?

runtime/compiler/env/j9method.cpp Outdated Show resolved Hide resolved
@hzongaro
Copy link
Member

By the way, may I suggest you remove the words "As I understand it" from the start of the commit comment?

@theresa-m theresa-m force-pushed the jit_scc branch 5 times, most recently from a6818c1 to 3b6b96d Compare January 14, 2025 14:37
@theresa-m theresa-m changed the title Include AccessController methods for jit JDK 24+ Jit compile AccessController.doPrivileged in JDK 24+ Jan 14, 2025
@keithc-ca
Copy link
Contributor

Please update the commit message and the title here to spell JIT properly and consistently.
Please also squash the repeated spaces in the commit message.

@theresa-m theresa-m changed the title Jit compile AccessController.doPrivileged in JDK 24+ JIT compile AccessController.doPrivileged in JDK 24+ Jan 14, 2025
AccessController.doPrivileged methods have historically
 not been compiled by the JIT due to security manager use.
Starting in JDK 24 the security manager will be removed
and these methods can be compiled.

Signed-off-by: Theresa Mammarella <[email protected]>
@theresa-m
Copy link
Contributor Author

I ran this change through a personal build and it is causing a crash during test compilation https://hyc-runtimes-jenkins.swg-devops.com/job/Test_openjdk24_j9_sanity.functional_x86-64_linux_Personal_testList_0/9/consoleFull.

@keithc-ca
Copy link
Contributor

That crash looks like #20870. About your only recourse is to try again.

@theresa-m
Copy link
Contributor Author

theresa-m commented Jan 14, 2025

https://hyc-runtimes-jenkins.swg-devops.com/job/Test_openjdk24_j9_sanity.functional_x86-64_linux_Personal_testList_0/9/consoleFull

Interesting... I've tried twice in a personal build and once locally and I got the same crash all three times. Perhaps I can try again when the issue is resolved.

edit: I tried it again locally and it was okay I'll give it another shot with a personal build

@theresa-m
Copy link
Contributor Author

My latest builds contains only known failures that are not related to this change https://hyc-runtimes-jenkins.swg-devops.com/job/Test_openjdk24_j9_sanity.functional_x86-64_linux_Personal/10/

@hzongaro
Copy link
Member

Jenkins test sanity.functional,sanity.openjdk xlinux,alinux,plinux jdk8,jdk21,jdk24

@hzongaro
Copy link
Member

JDK 24 test failures for alinux and plinux were all known problems. I will rerun the JDK 24 xlinux testing which failed to run to completion.

Jenkins test sanity.functional,sanity.openjdk xlinux jdk24

@hzongaro
Copy link
Member

Trying xlinux JDK 24 testing once more. . . .

Jenkins test sanity.functional,sanity.openjdk xlinux jdk24

@hzongaro
Copy link
Member

In JDK 24 xlinux sanity.functional testing:

  • Failure running testSCCMLTests1_openj9_0 doesn't seem to be a known problem, but it doesn't appear to be related to this change.
  • Other failures are known problems

The JDK 24 xlinux sanity.openjdk failures all appear to be due to known problems.

Merging.

@hzongaro hzongaro merged commit e395c4f into eclipse-openj9:master Jan 17, 2025
23 of 30 checks passed
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.

5 participants