-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
8330027: Identity hashes of archived objects must be based on a reproducible random seed #18735
8330027: Identity hashes of archived objects must be based on a reproducible random seed #18735
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
@tstuefe This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 48 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
Mac OS aarch64 build error unrelated (infra problem). I built and tested this patch locally on a Mac m1, so it is tested. |
Ping @iklam, @calvinccheung |
Webrevs
|
Do we need to generate I-hash on archive generation at all? |
Yes, see
|
I get that we need to preserve I-hashed. But why do we ever have to generate any new ones? |
That is an excellent question, and here is what I understand:
Therefore I think we don't have to actively generate ihashes. @iklam What do you think? [1]
|
I fixed a typo in the bug's synopsis. The easiest way to update is with: |
Thanks Dan :) /issue JDK-8330027 |
@tstuefe This issue is referenced in the PR title - it will now be updated. |
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.
Looks good.
nit: please update copyright header in thread.hpp.
@iklam @calvinccheung could you take another look please? I rewrote this patch to be both minimally invasive and as bulletproof against concurrent activity as possible. My thoughts on this:
|
This PR doesn't help CDS in terms of making the contents of archived heap objects deterministic. The dumping of archived heap objects is very sensitive to (Java thread) context switching: if two concurrent Java threads call As a result, during jdk/src/hotspot/share/prims/jvm.cpp Lines 2948 to 2963 in 235ba9a
Even if we apply this PR, we still cannot run more than one Java thread. Conversely, if we stick to a single Java thread, then the complexity in this PR is not needed. |
It does though, demonstratably. (comment removed) Once that is fixed, archives are non-deterministic for the reason stated in the PR description. And this PR makes the archive deterministic again. One could argue whether this is the simplest solution (and we do that below), but saying it does not help is just wrong, sorry.
I get all that. But the problem is not restricted to the one java thread. We create ihashes from the single one java thread, as well as the VM thread. They don't run concurrently, so their order of ihash creation calls is fixed for two subsequent runs of the same JVM. Okay. We seed each threads ihash RNG in Thread::Thread(). We seed it with Okay, we can make that global seed constant too. But we are betting that the number of os::random calls happening before the start of the java thread and the VM thread is constant from run to run. It often is, but that is not guaranteed by any means. If it differs, the VMthread and the one java thread get different seeds, therefore will generate different ihashes. The order of os::random() calls can change due to multiple reasons:
I get that the chance for this happening is remote, but hunting sources of entropy is frustrating work, and the patch is really very simple. So, why not fix it? I don't share the opinion that this is added complexity. |
Why not do it inside
|
Because then it would inject |
My last answer was rubbish, sorry, did not read your comment carefully enough. Yes, your approach would also work, but it would lead to the two threads involved in dumping the archive - VMthread and the one Java thread - using the same seed, hence generating the same sequence of ihashes. That, in turn, can lead to different archived objects carrying the same ihash, which may negatively impact performance later when the archive is used. |
I think it's better to just not compute the identity hash inside the VM thread. Here's what I tried We thought that forcing the identity hash computation would increase sharing across processes, as it would mean fewer updates of the object headers during run time. However, most of the heap objects in the CDS archive are not accessible by the application (they are part of the archived module graph, etc). Also the archive contains a large number of Strings, which are unlikely to need the identity hash (String has its own hashcode() method). Since the reason is rather dubious, I think it's better to remove it and simplify the system. |
I like that, that is simpler. Okay, then we will only call ihash from a single thread, so a global constant seed should be fine. I should be able to assert that, right? |
AI think an assert can be added, since we don't allow any Java threads to be launched. So even test cases that run arbitrary Java code during -Xshare:dump (using Java agents of -XX:ArchiveHeapTestClass) will not be able to run any Java code outside of the main Java thread. |
45ee254
to
ddfb218
Compare
@tstuefe Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
@iklam Sorry, had to force push because I messed up the PR branch somehow. The new version contains the change proposed by you - getting rid of explicit ihash generation when dumping - as well as a simplified version of my original patch. We now are back at generating the ihash seed in Thread::Thread(), with a constant if CDS dumps, and on ihash generation we check that we ever only call it with a single thread. |
I think it's possible to just discard your local branch, and check out a new version from the PR, then make changes on top of it. That way you can avoid forced pushes.
This version looks good. I am running our tiers 1-4 just to be sure. Thanks |
Its possible, I do this a lot when I mess up locally. But in this case, I had already accidentally pushed a broken merge to my GH fork, and then saw it touches >5000 files and has >300 commits. No idea what went wrong, but the only way to fix this that I found was to locally redo the patch and switch the branch.
Cool. I like the new version more, its simpler. Thank you!
|
@calvinccheung could you re-review this PR, please? It is simpler than the old version, but completely redone. Thank you! |
// Verify that during CDS dumping, only a single thread | ||
// ever calls ihash | ||
assert(!CDSConfig::is_dumping_archive() || runs_on_one_thread_only(), | ||
"Only one thread should generate ihash during CDS dumps"); |
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.
It should check for CDSConfig::is_dumping_static_archive()
instead (here and in Thread::Thread()
), to match the check in JVM_StartThread
jdk/src/hotspot/share/prims/jvm.cpp
Lines 2946 to 2948 in 7c1fad4
JVM_ENTRY(void, JVM_StartThread(JNIEnv* env, jobject jthread)) | |
#if INCLUDE_CDS | |
if (CDSConfig::is_dumping_static_archive()) { |
Thread* const cur = Thread::current(); | ||
if (t == nullptr) { | ||
Atomic::cmpxchg(&cds_dump_java_thread, t, cur); | ||
return true; |
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.
I think this is not thread safe, as I got few intermittent asserts.
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.
Hmm, it should not really need to be thread safe, since it should only ever be called from a single thread. Do the asserts always happen in the one and only java thread?
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.
I found the problem. A new JavaThread
is created during VM exit. This thread is attached to the main OS thread:
Thread 2 "java" hit Breakpoint 3, JavaThread::JavaThread (this=0x7ffff002c4e0, is_attaching_via_jni=true) at javaThread.cpp:532
532 JavaThread::JavaThread(bool is_attaching_via_jni) : JavaThread() {
(gdb) where
#0 JavaThread::JavaThread (this=0x7ffff002c4e0, is_attaching_via_jni=true)
#1 attach_current_thread (vm=0x7ffff7c283d8 <main_vm>, penv=0x7ffff5358d50, _args=0x7ffff5358d60, daemon=false)
#2 jni_AttachCurrentThread (vm=0x7ffff7c283d8 <main_vm>, penv=0x7ffff5358d50, _args=0x7ffff5358d60)
#3 JavaVM_::AttachCurrentThread (this=0x7ffff7c283d8 <main_vm>, penv=0x7ffff5358d50, args=0x7ffff5358d60)
#4 jni_DestroyJavaVM_inner (vm=0x7ffff7c283d8 <main_vm>)
#5 jni_DestroyJavaVM (vm=0x7ffff7c283d8 <main_vm>)
#6 JavaMain ()
#7 ThreadJavaMain ()
#8 start_thread (arg=<optimized out>)
#9 clone3 ()
This thread can execute Java code (shutdown hooks) which can call Object::identityHashcode()
.
@iklam I decided to remove the assert. I briefly thought about adding a condition to time (don't assert after dump finished) but decided against more complexity. |
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.
I'll wait for @calvinccheung 's re-approval, then push. |
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.
The latest version looks good. Thanks!
Cool, thanks @calvinccheung ! /integrate |
Going to push as commit 9f43ce5.
Your commit was automatically rebased without conflicts. |
/backport jdk22u |
/backport jdk21u-dev |
@tstuefe Could not automatically backport
Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk22u. Note: these commands are just some suggestions and you can use other equivalent commands you know.
Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk22u with the title Below you can find a suggestion for the pull request body:
|
@tstuefe Could not automatically backport
Please fetch the appropriate branch/commit and manually resolve these conflicts by using the following commands in your personal fork of openjdk/jdk21u-dev. Note: these commands are just some suggestions and you can use other equivalent commands you know.
Once you have resolved the conflicts as explained above continue with creating a pull request towards the openjdk/jdk21u-dev with the title Below you can find a suggestion for the pull request body:
|
CDS archive contains archived objects with identity hashes.
These hashes are deliberately preserved or even generated during dumping. They are generated based on a seed that is initialized randomly on a per-thread basis. These generations precede CDS dump initialization, so they are not affected by the init_random call there, nor would they be affected by JDK-8323900.
A random seed will not work for dumping archives since it prevents reproducible archive generation. Therefore, when dumping, these seeds must be initiated in a reproducible way.
--- Update
After discussions with Ioi, and several redos, we settled on:
--- Update Update
The final version I plan to push does not have above mentioned assert, since it turned out to be too tricky and complex to get right, not worth the trouble.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18735/head:pull/18735
$ git checkout pull/18735
Update a local copy of the PR:
$ git checkout pull/18735
$ git pull https://git.openjdk.org/jdk.git pull/18735/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18735
View PR using the GUI difftool:
$ git pr show -t 18735
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18735.diff
Webrev
Link to Webrev Comment