Adding more GHA jobs to cover JDK versions on ARM #1508
Conversation
|
@Baraa-Hasheesh I think you're adding "GHA jobs", not runners |
| jvmArgs = "-Xcomp", | ||
| jvm = Jvm.HOTSPOT | ||
| jvm = Jvm.HOTSPOT, | ||
| jvmVer = {8, 24} // TODO: Enable on next JDK25 is released JDK-8367689 |
There was a problem hiding this comment.
Unclear comment, what does it mean?
There was a problem hiding this comment.
After JDK-8367689 is released in next JDK25 LTS release we need to enable this test back to run on all JDKs
There was a problem hiding this comment.
I see, the comment is quite unclear tho
There was a problem hiding this comment.
TODO: remove version filter after the fix for JDK-8367689 is released
| mainClass = StringBuilderTest.class, | ||
| debugNonSafepoints = true, | ||
| arch = {Arch.ARM64, Arch.ARM32}, | ||
| // C2 Inlining can cause some issues when aggressive inlining happens |
There was a problem hiding this comment.
If the problem is inlining, why can't we run this with -XX:-Inline on the versions affected by this behavior?
There was a problem hiding this comment.
Let's not hide bugs here
I can make the test "pass" in many ways
The problem should be ideally fixed then we can re-enable the tests
There was a problem hiding this comment.
Let's not hide bugs here
I'm not proposing to hide it. I'm saying that instead of disabling the test altogether on modern JDK versions running on ARM, we could think of having a separate test which runs with -XX:-Inline, if that really solves the problem. It could still help us catching possible regressions, especially since there isn't a clear timeline towards a fix.
There was a problem hiding this comment.
I like the idea,
@apangin what are you're thoughts about adding a separate test with -XX:-Inline just to capture possible issues/regressions in Intrinsics on newer JDKs?
There was a problem hiding this comment.
The comment sounds hand-waving. How can you claim something about aggressive inlining if you haven't got to the root cause yet?
You can frame it like "C2 often loses PcDesc mapping from arraycopy intrinsic to the original bytecode."
Link JBS issue when you create it.
There was a problem hiding this comment.
Just to note, I will rephrase the comment
As we've yet to find the exact root cause
But we noticed that the issue is always accompanied with aggressive inlines
Disabling the inline does make the test "pass"
There was a problem hiding this comment.
Testing with -XX:-Inline has no practical benefit.
| image: public.ecr.aws/async-profiler/asprof-builder-arm:latest | ||
| - runson: | ||
| display: linux-arm64 | ||
| # There is no "latest" tag available (yet) as ARM runners are still in public preview |
There was a problem hiding this comment.
I think one of these comments is enough, especially because it does not seem to be a real problem for us.
Baraa-Hasheesh
changed the title
|
Please wait for #1502 to be merged. |
# Conflicts: # .github/workflows/test-and-publish-nightly.yml
# Conflicts: # .github/workflows/test-and-publish-nightly.yml
| jvm = Jvm.HOTSPOT | ||
| jvm = Jvm.HOTSPOT, | ||
| // TODO: remove version filter after the fix for JDK-8367689 is released | ||
| jvmVer = {8, 24} |
There was a problem hiding this comment.
| jvmVer = {8, 24} | |
| jvmVer = {8, 25} |
There was a problem hiding this comment.
private static boolean applicable(Test test) {
Os[] os = test.os();
Arch[] arch = test.arch();
Jvm[] jvm = test.jvm();
int[] jvmVer = test.jvmVer();
return (os.length == 0 || Arrays.asList(os).contains(currentOs)) &&
(arch.length == 0 || Arrays.asList(arch).contains(currentArch)) &&
(jvm.length == 0 || Arrays.asList(jvm).contains(currentJvm)) &&
(jvmVer.length == 0 || (currentJvmVersion >= jvmVer[0] && currentJvmVersion <= jvmVer[jvmVer.length - 1]));
}
values are inclusive, we don't want to run on 25 as it's currently broken until next release
4 participants
Description
Adding GHA runners to cover JDKs 17,21, & 25 on ARM & Also replacing JDK24 with 25 on x86
we recently found some issues on more recent JDK versions on ARM that would have been found by simply running the integration tests
Related issues
#1496
Motivation and context
Increase the coverage & early detection of bugs on ARM
How has this been tested?
GHA runners &
make testBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.