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

Unexclude compiler/rtm tests on JDK8 #4290

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

zzambers
Copy link
Contributor

Compiler/rtm tests can be removed from exclude list of JDK8 in aqa-tests. They were excluded in jdk8u itself as part of effort to fix hotspot/tier1. See: https://bugs.openjdk.org/browse/JDK-8295915

@sophia-guo
Copy link
Contributor

sophia-guo commented Feb 2, 2023

Those tests will be running on all platforms with @requires vm.rtm.cpu & vm.rtm.compiler Upstream jdk8u only excludes specific arch generic-x64,generic-i586 openjdk/jdk8u-dev@fb98418, which would be good for github actions. However in Adoptium we have much more different platforms to support - x64, i586, ppc64, ppc64le, s390x, aarch64 etc.

@sophia-guo
Copy link
Contributor

Could upstream exclude as generic-all? Otherwise it might fail again in Adoptium on other platforms.

@zzambers
Copy link
Contributor Author

zzambers commented Feb 2, 2023

@sophia-guo There should be no need to exclude them on other archs. RTM (Restricted Transactional Memory) is extension of x86 ISA (See: [1]). Checks should get skipped on processors not supporting this extension, which should always be the case for non-x86 (See: [2][3]).

[1] https://en.wikipedia.org/wiki/Transactional_Synchronization_Extensions#RTM
[2] https://github.com/openjdk/jdk8u-dev/blob/784f71f218bdb0e885a03fae35105d2fc96d56ec/hotspot/test/compiler/rtm/locking/TestUseRTMAfterLockInflation.java#L66
[3] https://github.com/openjdk/jdk8u-dev/blob/784f71f218bdb0e885a03fae35105d2fc96d56ec/hotspot/test/compiler/testlibrary/rtm/predicate/SupportedCPU.java#L34

@sophia-guo
Copy link
Contributor

Recently found that one of our ppc64le has rtm feature adoptium/infrastructure#2896

@zzambers
Copy link
Contributor Author

zzambers commented Feb 2, 2023

@sophia-guo, Interesting, didn't know ppc64 can also support RTM. But seems you are right, there is some rtm code added for ppc by JDK-8077838 [1]. Do rtm tests fail there?

Anyway JDK-8077838 was not backported to JDK8 and I could not find arch specific code for rtm other than x86 in JDK8. Not sure if it will ever get backported to JDK8. Deprecation of RTM was actually considered [2] (which btw lists x86 and ppc).

[1] https://bugs.openjdk.org/browse/JDK-8077838
[2] https://bugs.openjdk.org/browse/JDK-8292096

@sophia-guo
Copy link
Contributor

I did't pay much attention to those rtm tests on jdk8 as they were excluded. For recent jdk19 I can say those rtm tests fail intermittently on some ppc64le.

@sophia-guo
Copy link
Contributor

sophia-guo commented Feb 6, 2023

Interesting I did test all jdk8 4 tests on linux64 and ppc64le with rtm or without rtm features all tests passed. https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/6565/ https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/6566/
https://ci.adoptopenjdk.net/view/Test_grinder/job/Grinder/6567/

Good to remove the exclusion and let upstream to decide exclude or not.

@sophia-guo sophia-guo merged commit 9719b47 into adoptium:master Feb 6, 2023
@zzambers
Copy link
Contributor Author

zzambers commented Feb 6, 2023

@sophia-guo In case of JDK 8 I don't think excluding upstream other architectures than x86 (32,64) is necessary (as JDK8 seems to only support transactional memory on x86, if there is some problem in ppc code, it should only affect newer JDKs).

I checked our results, as we also do some testing on ppc64le (rpms), but I have not found any rtm test failures (8, 17). But it is possible, that we don't have right HW to reproduce your issue.

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.

3 participants