-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8227745: Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents #119
Conversation
…e Presence of JVMTI Agents
👋 Welcome back rrich! A progress list of the required criteria for merging this PR into |
/issue add JDK-8233915 |
@reinrich |
/label add hotspot |
@reinrich |
Webrevs
|
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.
Hi Richard,
I had reviewed an older webrev. Your newest improvements look good.
@reinrich 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 28 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 |
Thanks Martin! |
The commit message looks odd to me. Should I remove ", 8233915" from the title of this request? I used the old scheme "JBS-Id[, JBS-Id]*" and this might be wrong. |
The commit message is correct according to JEP 357 |
Sorry, now I see. Yes, please remove |
@reinrich The following labels will be automatically applied to this pull request: When this pull request is ready to be reviewed, an RFR email will be sent to the corresponding mailing lists. If you would like to change these labels, use the |
Thanks for helping. The commit message does look better now. |
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 can't review this in-depth as I am not familiar with too much of the code. I have looked at some of the key threading related parts and have made a number of comments.
Mailing list message from David Holmes on hotspot-dev: On 14/09/2020 6:18 pm, Richard Reingruber wrote:
Doh! My bad. David |
Reviewed in the good old hg times :) |
Hi Richard, Could you consider to place the classes EscapeBarrier and JvmtiDeferredUpdates into theyr own .hpp/.cpp files? The class JvmtiDeferredUpdates would be better to put into the folder 'prims' then. src/hotspot/share/opto/macro.cpp:
I wonder if the comment is still correct after you removed the check for JvmtiExport::can_pop_frame(). src/hotspot/share/runtime/deoptimization.hpp:
Nit: would better to make the parameter deoptee_thread to be the 3rd to better mach the seconf constructor.
I'd suggest to put comments in a line before function definitions as it is done for other declarations/definitions. |
src/hotspot/share/runtime/deoptimization.cpp:
It is not clear why the 'list' is not deleted anymore. If it is intentional then could you, please, add a comment with an explanation? If you are okay to separate the EscapeBarrier class into its own hpp/cpp files then the class EscapeBarrierSuspendHandshake is better to be colocated with it. The below functions EscapeBarrier::sync_and_suspend_one() and do_thread() make a call to the set_obj_deopt_flag() which seems to be a duplication. At least, it is not clear why this duplication exist and so, needs to be explained in a comment.
/src/hotspot/share/prims/jvmtiImpl.cpp:
I think, false has to be passed to the constructors of non-object getters instead of expression: |
EscapeBarrier::sync_and_suspend_all(): - Set suspend flags before handshake because then the setting will be visible after leaving the _thread_blocked state in JavaThread::wait_for_object_deoptimization() JavaThread::wait_for_object_deoptimization() - Reuse SpinYield instead of new custom spinning code. - Do safepoint check after the loop. This is possible because the set_obj_deopt_flag is done before the handshake. - Don't set_suspend_equivalent() anymore for more simplicity. It's just an optimization (that won't pay off here). Added/updated source code comments. Additional smaller enhancements.
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.
Compiler changes seems fine.
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 tried to run testing with latest changes and latest JDK and build failed:
src/hotspot/share/runtime/escapeBarrier.cpp:310:35: error: no matching function for call to 'StackFrameStream::StackFrameStream(JavaThread*&)'
310 | StackFrameStream fst(deoptee);
Thank you for looking again at this. |
I noticed this too. I wanted to test with ZGC before pushing the small
In the test case So far I do not have an indication that the failure is caused by this change but I need to look more into it. Wish I was a ZGC expert :) Anyway I pushed the build fix. Tests succeed with default GC. |
The crash described above happens after JDK-8253180 (b9873e1) when executing
My understanding of JDK-8253180 (and ZGC) is rather vague. To me it looks as if stackwalks outside of a safepoint/handshake on suspended threads are currently not supported. It would be my understanding that
I will ask Erik Österlund about this. |
The issues with ZGC concurrent thread stack processing will be resolved with #627 |
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.
Good.
Thanks for the review, Vladimir (@vnkozlov)! |
…heck from ~ThreadInVMForHandshake() With JDK-8254263 the special_runtime_exit_condition() check was removed from ~ThreadInVMForHandshake() because now a thread never becomes unsafe when processing its own handshakes. EscapeBarrier uses handshakes to sync with the target thread for object deoptimization so we add a check for object deoptimization to ThreadSafepointState::handle_polling_page_exception(). In JavaThread::wait_for_object_deoptimization() we must check is_obj_deopt_suspend() again after handshake/safepoint processing because a handshake for obj. deopt suspend could have been processed.
…timization _before_ async ex. check.
… See JDK-8254264.
Thanks once more @TheRealMDoerr, @GoeLin, @dholmes-ora, @sspitsyn, @vnkozlov, @robehn, @pchilano for feedback and reviews. |
/label remove core-libs |
@AlanBateman |
/integrate |
@reinrich Since your change was applied there have been 32 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 40f847e. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
* IDEA-283934 Top panel (toolbar, Editor tabs) hides under the Mac menu in full-screen mode * IDEA-283934 Top panel (toolbar, Editor tabs) hides under the Mac menu in full-screen mode * JBR-4305 IDEA-283934 Top panel (toolbar, Editor tabs) hides under the Mac menu in full-screen mode * JBR-4305 IDEA-283934 Top panel (toolbar, Editor tabs) hides under the Mac menu in full-screen mode
Hi,
this is the continuation of the review of the implementation for:
https://bugs.openjdk.java.net/browse/JDK-8227745
https://bugs.openjdk.java.net/browse/JDK-8233915
It allows for JIT optimizations based on escape analysis even if JVMTI agents acquire capabilities to access references to objects that are subject to such optimizations, e.g. scalar replacement.
The implementation reverts such optimizations just before access very much as when switching from JIT compiled execution to the interpreter, aka "deoptimization".
Webrev.8 was the last one before before the transition to Git/Github:
http://cr.openjdk.java.net/~rrich/webrevs/8227745/webrev.8/
Thanks, Richard.
Progress
Testing
Issues
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/119/head:pull/119
$ git checkout pull/119