-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8365407: Race condition in MethodTrainingData::verify() #26866
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
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back iveresov! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
/label add hotspot-compiler |
@veresov |
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.
Just a few drive-by comments as I'm not familiar with this "training" stuff.
while (!CompileBroker::is_compilation_disabled_forever()) { | ||
InstanceKlass* ik = _training_replay_queue.pop(TrainingReplayQueue_lock, THREAD); | ||
void CompilationPolicy::replay_training_at_init_loop(JavaThread* current) { | ||
while (!CompileBroker::is_compilation_disabled_forever() || AOTVerifyTrainingData) { |
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.
Will it loop forever with + AOTVerifyTrainingData
?
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.
Yes, it runs in a dedicated thread. It doesn't need to terminate.
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.
Add comment about this.
This change fixes multiple issue with training data verification. While the current state of things in the mainline will not cause any issues (because of the absence of the call to
TD::verify()
during the shutdown) it does problems in the leyden repo. This change strengthens verification in the mainline (by adding the shutdown verify call), and fixes the problems that prevent it from working reliably.Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26866/head:pull/26866
$ git checkout pull/26866
Update a local copy of the PR:
$ git checkout pull/26866
$ git pull https://git.openjdk.org/jdk.git pull/26866/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26866
View PR using the GUI difftool:
$ git pr show -t 26866
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26866.diff
Using Webrev
Link to Webrev Comment