8255233: InterpreterRuntime::at_unwind should be a JRT_LEAF #828
Conversation
|
|
Webrevs
|
|
The delayed AArch64 test results are in: looking good as expected. However the github bot complained about x86 32 bit needing a cast, so hopefully fixed that now. I guess we will see what the bot has to say. Thank you bot for finding that. |
|
@fisk This change now passes all automated pre-integration checks. 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 77 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.
|
| JRT_LEAF(void, InterpreterRuntime::at_unwind(JavaThread* thread)) | ||
| // JRT_END does an implicit safepoint check, hence we are guaranteed to block | ||
| // if this is called during a safepoint |
There was a problem hiding this comment.
The comments are no longer valid. The implicit safepoint check came from the use of ThreadInVMfromJava as part of the JRT_ENTRY.
Also it is far from obvious that StackWatermarkSet::before_unwind meets all the requirements of a JRT_LEAF method. Please assure me it is. :)
There was a problem hiding this comment.
Nice catch. I removed the out of date comments.
Regarding the leafness of the operation: the operations performed in the stack watermark are indeed designed to run in such conditions. When the callbacks are executed, verification code is run, asserting we are not in a state ignored by the GC. It would never work to have code that transitions inside of such callbacks. Today, it merely applies some GC barriers, similar to performing an access API call.
Having said that, despite not transitioning in this code path, I do make the stack walkable when calling this method, so that the barrier can see what the last_Java_frame is, and figure out if action needs to be taken or not. But we do that for other leaf functions as well, and that is fine. The crucial thing is that there are zero transitions, which I can assure you of. In particular, the thread will be _in_java throughout the entire operation.
Thanks for the review!
Thanks for the review. |
|
/integrate |
openjdk
bot
added
integrated
|
@fisk Since your change was applied there have been 82 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit aaf4f69. |
InterpreterRuntime::at_unwind is called at the very beginning of remove_activation(), to notify concurrent stack processing that a frame is about to be unwound. It is currently a JRT_ENTRY, because it needs a last_Java_frame to see what frame is about to get unwound.
However, there are special return paths used by JVMTI pop frame, that checks if the caller frame is deoptimized, then calls a special path that removes the top activation, assuming that does not enter the deopt handler. The new JRT_ENTRY makes that reasoning invalid.
Therefore, we need this to be a JRT_LEAF, that sets a last Java frame, to make everyone happy. This patch performs that change.
I have run tier 1-5 testing, and manually tested:
while true; do make test JTREG="RETAIN=all" TEST=open/test/hotspot/jtreg/vmTestbase/nsk/jvmti/ForceEarlyReturn/ForceEarlyReturn002 TEST_OPTS_JAVA_OPTIONS="-XX:+UseZGC -Xmx2g -XX:ZCollectionInterval=0.0001 -XX:ZFragmentationLimit=0.01 -XX:+VerifyOops -XX:+ZVerifyViews" ; done
Before the fix it crashes ~1/15 runs with a bad oop. After the fix, it doesn't crash. I have run it more times than my tmux buffer fits (for a day), and it does not fail any more with this fix.
Unfortunately, my testing on AArch64 has been stalled for a day, so I have sent out this PR without the testing of those bits being finished. I won't push until I get the results back, of course. But I am expecting that to be fine, as there is nothing special going on there and it compiles. Will post a comment when the complete results have arrived.
Progress
Testing
Failed test task
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/828/head:pull/828$ git checkout pull/828