Skip to content
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

8285739: disable EscapeBarrier deopt for virtual threads #8589

Closed
wants to merge 2 commits into from

Conversation

lmesnik
Copy link
Member

@lmesnik lmesnik commented May 9, 2022

The fix disables EscapeBarrier and EscapeAnalysis when certain JVMTI capabilities are enabled and --enable-preview.

It restores the same behavior as it was before https://bugs.openjdk.java.net/browse/JDK-8227745 "Enable Escape Analysis for Better Performance in the Presence of JVMTI Agents" is implemented when Continuations are enabled.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

  • JDK-8285739: disable EscapeBarrier deopt for virtual threads

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8589/head:pull/8589
$ git checkout pull/8589

Update a local copy of the PR:
$ git checkout pull/8589
$ git pull https://git.openjdk.java.net/jdk pull/8589/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8589

View PR using the GUI difftool:
$ git pr show -t 8589

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8589.diff

@bridgekeeper
Copy link

bridgekeeper bot commented May 9, 2022

👋 Welcome back lmesnik! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented May 9, 2022

@lmesnik The following labels will be automatically applied to this pull request:

  • hotspot
  • serviceability

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 /label pull request command.

@openjdk openjdk bot added serviceability hotspot labels May 9, 2022
@lmesnik lmesnik marked this pull request as ready for review May 9, 2022
@openjdk openjdk bot added the rfr label May 9, 2022
@mlbridge
Copy link

mlbridge bot commented May 9, 2022

Webrevs

@lmesnik lmesnik closed this May 9, 2022
@lmesnik lmesnik deleted the 8285739 branch May 9, 2022
@lmesnik lmesnik restored the 8285739 branch May 9, 2022
@lmesnik lmesnik reopened this May 9, 2022
Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.

@openjdk
Copy link

openjdk bot commented May 9, 2022

@lmesnik 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:

8285739: disable EscapeBarrier deopt for virtual threads

Reviewed-by: kvn, rrich, sspitsyn

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 245 new commits pushed to the master branch:

  • 8040aa0: 8286908: ECDSA signature should not return parameters
  • 689f80c: 8287153: Whitespace typos in HeapRegion class
  • c906591: 8286391: Address possibly lossy conversions in jdk.compiler
  • 88018c4: 8287150: Remove HeapRegion::block_start_const declaration without definition
  • 81f128b: 8287154: java/nio/channels/FileChannel/LargeMapTest.java does not compile
  • 7becf13: 8286825: Linker naming cleanup
  • a0042de: 8276549: Improve documentation about ContainerPtr encoding
  • 89a1d05: 8286715: Generalize MemorySegment::ofBuffer
  • cb08b4e: 8287024: G1: Improve the API boundary between HeapRegionRemSet and G1CardSet
  • 01916e1: 8287053: Avoid redundant HashMap.containsKey calls in ZoneInfoFile.getZoneInfo0
  • ... and 235 more: https://git.openjdk.java.net/jdk/compare/9583e3657e43cc1c6f2101a64534564db2a9bd84...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 9, 2022
@reinrich
Copy link
Contributor

reinrich commented May 9, 2022

Hi Leonid,

have you done some testing? JDK-8227745 came with a bunch of tests. I wonder how they behave with your change.

Thanks, Richard.

@lmesnik
Copy link
Member Author

lmesnik commented May 9, 2022

Hi Leonid,

have you done some testing? JDK-8227745 came with a bunch of tests. I wonder how they behave with your change.

Thanks, Richard.

Thank you for looking on this.

The EA-related tests passed and still pass when are executed without any additional options. There is a special mode when the test runs main() method in the virtual thread. Test IterateHeapWithEscapeAnalysisEnabled.java failed before the fix and start passing after my fix. It failed because before the fix EscapeBarrier was disabled while EA was enabled and we got incorrect data.
Unfortunately, I was unable to run EATests.java in this mode even before the fix. (Not all tests are compatible with this "wrapper" mode)

The goal is to restore the same behavior as it was before JDK-8227745 is implemented when virtual threads are enabled.

@lmesnik
Copy link
Member Author

lmesnik commented May 9, 2022

Also I run all our testing on the linux-x64-debug to ensure that there are no regressions.

@reinrich
Copy link
Contributor

reinrich commented May 9, 2022

Ok, thanks for letting me know.

Copy link
Contributor

@reinrich reinrich left a comment

Hi Leonid,

your changes look good to me.

Thanks, Richard.

@lmesnik
Copy link
Member Author

lmesnik commented May 20, 2022

After discussion with @sspitsyn and more testing, I realized that it is not enough to disable EA with some capabilities. The IterateHeap requires a capability which is enabled by default. Moreover, code might be compiled before the agent requests any caps. So the new proposed fix is to apply EscapeBarriers for platform threads only. For virtual threads, the behavior is the same as before JDK-8227745. This should be fixed by https://bugs.openjdk.java.net/browse/JDK-8264699

@lmesnik
Copy link
Member Author

lmesnik commented May 20, 2022

The fix has been pushed into loom repo already and it is a backport:
openjdk/loom@6520b71

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Update is good.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Hi Leonid,
Thank you a lot for taking care about this!
Looks good to me (according to our private agreement).
My understanding is that this changeset fixes a test regression.
Could you, please, list this test being fixed in the bug report?
Thanks,
Serguei

@lmesnik
Copy link
Member Author

lmesnik commented May 21, 2022

This change fixes regression of platform threads behavior with --enabled-preview enabled. However, the info for virtual threads might be incorrect if EA is enabled.
The issue JDK-8264699 Re-examine deopt of frames to allow EA be enabled with JVMTI agents should cover it.

@lmesnik
Copy link
Member Author

lmesnik commented May 21, 2022

Affected tests are:
serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysisEnabled.java
com/sun/jdi/EATests.java#id0

The pass in jdk/jdk because --enable-preview is not enabled but failed before fix in fibers branch where --enable-preview is enabled by default.

The should pass with --enabled-preview after fix. The are removed from problemlist in fibers branch in the fix:
openjdk/loom@6520b71

@reinrich
Copy link
Contributor

reinrich commented May 21, 2022

Hi Leonid,
if EscapeAnalysis is not disabled, then local objects cannot be read per JVMTI if they are scalarized in compiled frames on the heap, right? This would be a problem I'd think.
Thanks, Richard.

@lmesnik
Copy link
Member Author

lmesnik commented May 21, 2022

Hi Leonid, if EscapeAnalysis is not disabled, then local objects cannot be read per JVMTI if they are scalarized in compiled frames on the heap, right? This would be a problem I'd think. Thanks, Richard.

Yes, the fix restores correct behavior for platform threads if --enable-preview is on. The JDK-8285739 should fix this for virtual threads.

And as test serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysisEnabled.java demonstrates there are might be methods which compiled with EA before JVMTI agent require capabilities.

I incorrectly tested my previous fix, it really doesn't pass serviceability/jvmti/Heap/IterateHeapWithEscapeAnalysisEnabled.java test with --enable-preview.

@reinrich
Copy link
Contributor

reinrich commented May 23, 2022

Hi Leonid, if EscapeAnalysis is not disabled, then local objects cannot be read per JVMTI if they are scalarized in compiled frames on the heap, right? This would be a problem I'd think. Thanks, Richard.

Yes, the fix restores correct behavior for platform threads if --enable-preview is on. The JDK-8285739 should fix this for virtual threads.

Could it be that you mean JDK-8264699?

I'm ok with this version of your fix. I'd suggest to change title/synopsis of the bug report to better match it though.

@lmesnik lmesnik changed the title 8285739: disable EA when both JVMTI and Preview are enabled 8285739: disable EscapeBarrier deopt for virtual threads May 23, 2022
@lmesnik
Copy link
Member Author

lmesnik commented May 23, 2022

Could it be that you mean JDK-8264699?

Yes, you are right.

I'm ok with this version of your fix. I'd suggest to change title/synopsis of the bug report to better match it though.

Thank you. I've renamed the summary

@lmesnik
Copy link
Member Author

lmesnik commented May 23, 2022

/integrate

@openjdk
Copy link

openjdk bot commented May 23, 2022

Going to push as commit 940e94f.
Since your change was applied there have been 246 commits pushed to the master branch:

  • 110d906: 8287103: java/lang/management/ThreadMXBean/VirtualThreadDeadlocks.java fails with Xcomp
  • 8040aa0: 8286908: ECDSA signature should not return parameters
  • 689f80c: 8287153: Whitespace typos in HeapRegion class
  • c906591: 8286391: Address possibly lossy conversions in jdk.compiler
  • 88018c4: 8287150: Remove HeapRegion::block_start_const declaration without definition
  • 81f128b: 8287154: java/nio/channels/FileChannel/LargeMapTest.java does not compile
  • 7becf13: 8286825: Linker naming cleanup
  • a0042de: 8276549: Improve documentation about ContainerPtr encoding
  • 89a1d05: 8286715: Generalize MemorySegment::ofBuffer
  • cb08b4e: 8287024: G1: Improve the API boundary between HeapRegionRemSet and G1CardSet
  • ... and 236 more: https://git.openjdk.java.net/jdk/compare/9583e3657e43cc1c6f2101a64534564db2a9bd84...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label May 23, 2022
@openjdk openjdk bot closed this May 23, 2022
@openjdk openjdk bot removed ready rfr labels May 23, 2022
@openjdk
Copy link

openjdk bot commented May 23, 2022

@lmesnik Pushed as commit 940e94f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated serviceability
4 participants