-
Notifications
You must be signed in to change notification settings - Fork 771
Make client and server agree on exact permanent loaders #22755
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
Make client and server agree on exact permanent loaders #22755
Conversation
@mpirvu, could you please review? |
...within each compilation. There will be at least two data structures that are sensitive to the set of permanent loaders. These could potentially allow the precise set of permanent loaders to vary between the client and server, but the reasoning needed to determine whether certain kinds of variation are allowed gets hairy very quickly. It's much simpler if there is only one set of permanent loaders that will be used for all purposes on both the client and the server. This change also simplifies reasoning about which permanent loaders the server already has. The client no longer attempts to keep track of that at all. Instead, that information is purely in the purview of the server, where it's easy to maintain consistency.
This was avoiding getting the permanent loaders from the compilation object to guard against a scenario where the server side of the compilation was aware of more permanent loaders than the client side. That scenario is no longer possible because a compilation on the server now limits itself to the same set of permanent loaders that the corresponding compilation is aware of on the client. It's no longer necessary for J9::RetainedMethodSet to allow the caller to specify the permanent loaders on creation.
48e2439
to
03efb95
Compare
Pushed a small correction to a comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk21 |
Checks have finished. Rundown of the problems... On all platforms: Missing On AArch64: java/lang/Thread/virtual/Collectable.java: timeout: #18463 On x86-64: java/util/concurrent/ConcurrentHashMap/ConcurrentAssociateTest.java: timeout: #14538 (comment) On AArch64: java/lang/Math/HypotTests.java: NPE from There were a bunch of other instances of NPE from within A bit more concerning... x86-64: java/util/stream/StreamBuilderTest.java: multiple occurrences each of That said, I struggle to see any exception as a realistic failure mode for this PR. The change in the set of permanent loaders should affect maybe about one compilation per process, where it should only influence the set of bond assumptions we might create, unless somehow Most concerning... PPC64LE: There were a number of crashes in
but one had this stack instead:
For the moment I'll take a closer look at the crashes. Please let me know what you think about the other failures. |
I would prefer to have eclipse-omr/omr#7983 and #22768 delivered first, then a rebase and another round of tests. |
I did look at that, but based on the PR description I thought that the relocations would only cause compilation failures. But I could be wrong. And I should be moving eclipse-omr/omr#7983 along regardless. So that plan sounds fine to me. |
I have verified that the fix in eclipse-omr/omr#7983 clears a functional jitserver test with Java11 that started when #22530 was delivered. Since changes in 22530 are not JDK version specific, it's possible that all JDKs are affected in various ways. I would rather eliminate 22530 as the source of potential failures, by merging eclipse-omr/omr#7983 first. |
jenkins sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk21 depends eclipse/omr#master |
1 similar comment
jenkins sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk21 depends eclipse/omr#master |
Jenkins test sanity plinuxjit,xlinuxjit,zlinuxjit,alinux64jit jdk21 depends eclipse/omr#master |
The sanity.openjdk jobs that have finished so far (AArch64, PPC64LE, x86-64) all hit #20995 as expected The only other problem so far is that on AArch64, testList_2 failed due to an infra problem. It had already passed the same six tests that passed in testList_2 on each of PPC64LE and x86-64. I checked, and the partition into test lists is consistent across platforms, so the AArch64 testList_2 passed all of the tests that we can expect it to pass given #20995. There were just a few vector API tests left to run. Still waiting for sanity.openjdk on z, and sanity.functional on z and x86-64 (edited to correct the referenced issue number) |
The failures on sanity.functional on xlinux are all JFR and not related to changes from this PR.
The failures from sanity.openjdk on zlinux are vector API which currently does not work with JITServer. All in all, this PR has not introduced any new failures, so it's good to be merged. |
...within each compilation.
There will be at least two data structures that are sensitive to the set of permanent loaders. These could potentially allow the precise set of permanent loaders to vary between the client and server, but the reasoning needed to determine whether certain kinds of variation are allowed gets hairy very quickly. It's much simpler if there is only one set of permanent loaders that will be used for all purposes on both the client and the server.
This change also simplifies reasoning about which permanent loaders the server already has. The client no longer attempts to keep track of that at all. Instead, that information is purely in the purview of the server, where it's easy to maintain consistency.
Additionally:
comp->permanentLoaders()
for client retained methods analysis