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

8325613: CTW: Stale method cleanup requires GC after Sweeper removal #18249

Closed
wants to merge 1 commit into from

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Mar 12, 2024

See more details in the bug. There is a double-whammy from two issues: a) Sweeper was removed, and now the cleanup work is done during GC, which does not really happen as CTW barely allocates anything; b) CTW calls for explicit deoptimization often, at which point CTW threads get mostly busy at spin-waiting-yielding for deopt epoch to move (that is why you see lots of sys%). (a) leads to stale methods buildup, which makes (b) progressively worse.

This PR adds explicit GC calls to CTW runner. Since CTW allocates and retains a little, those GCs are quite fast. I chose the threshold by running some CTW tests on my machines. I think we are pretty flat in 25..100 region, so I chose the higher threshold for additional safety.

This patch improves both CPU and wall times for CTW testing dramatically, as you can see from the logs below. It still does not recuperate completely to JDK 17 levels, but it least it is not regressing as badly.

--- x86_64 EC2, applications/ctw/modules CTW

jdk17u-dev:            4511.54s user  169.43s system 1209% cpu  6:27.07 total
current mainline:     11678.13s user 8687.06s system 2299% cpu 14:45.62 total

GC every 25 methods:   5050.83s user  670.38s system 1629% cpu  5:51.04 total
GC every 50 methods:   4965.41s user  709.64s system 1670% cpu  5:39.77 total
GC every 100 methods:  4997.34s user  782.12s system 1680% cpu  5:43.99 total
GC every 200 methods:  5237.76s user  943.51s system 1788% cpu  5:45.59 total
GC every 400 methods:  5851.24s user 1443.16s system 1914% cpu  6:20.99 total
GC every 800 methods:  7010.06s user 2649.35s system 2079% cpu  7:44.48 total
GC every 1600 methods: 9361.12s user 5616.84s system 2409% cpu 10:21.68 total

--- Mac M1, applications/ctw/modules/java.base CTW

jdk17u-dev:             171.93s user   25.33s system  157% cpu  2:05.34 total
current mainline:      1128.69s user  349.46s system  249% cpu  9:52.51 total

GC every 25 methods:    252.31s user   29.98s system  172% cpu  2:43.68 total 
GC every 50 methods:    232.53s user   28.49s system  170% cpu  2:32.69 total
GC every 100 methods:   237.38s user   34.53s system  169% cpu  2:40.54 total 
GC every 200 methods:   251.70s user   39.60s system  172% cpu  2:48.40 total
GC every 400 methods:   271.50s user   42.55s system  185% cpu  2:49.66 total
GC every 800 methods:   389.51s user   69.41s system  204% cpu  3:44.01 total
GC every 1600 methods:  660.98s user  169.97s system  229% cpu  6:01.78 total

Progress

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

Issue

  • JDK-8325613: CTW: Stale method cleanup requires GC after Sweeper removal (Bug - P4)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18249/head:pull/18249
$ git checkout pull/18249

Update a local copy of the PR:
$ git checkout pull/18249
$ git pull https://git.openjdk.org/jdk.git pull/18249/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18249

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18249.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 12, 2024

👋 Welcome back shade! 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 Mar 12, 2024

@shipilev The following label will be automatically applied to this pull request:

  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Mar 12, 2024
@shipilev shipilev marked this pull request as ready for review March 12, 2024 19:34
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 12, 2024
@mlbridge
Copy link

mlbridge bot commented Mar 12, 2024

Webrevs

Copy link
Contributor

@rwestrel rwestrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me.

@openjdk
Copy link

openjdk bot commented Mar 13, 2024

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

8325613: CTW: Stale method cleanup requires GC after Sweeper removal

Reviewed-by: roland, chagedorn

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

  • 49ce85f: 8327874: Convert javax/swing/JTree/4314199/bug4314199.java applet test to main
  • 481c866: 8327468: Do not restart close if errno is EINTR [macOS/linux]
  • 44aef38: 8327045: Consolidate -fvisibility=hidden as a basic flag for all compilation
  • fcf746d: 8328106: COMPARE_BUILD improvements
  • fadc4b1: 8327423: C2 remove_main_post_loops: check if main-loop belongs to pre-loop, not just assert
  • cff0747: 8326204: yield statements doesn't allow cast expressions with more than 1 type arguments
  • 6f2676d: 8328064: Remove obsolete comments in constantPool and metadataFactory
  • 7502dc9: 8328030: Convert javax/swing/text/GlyphView/4984669/bug4984669.java applet test to main
  • 357c912: 8325897: Parallel: Remove PSYoungGen::is_maximal_no_gc
  • 98e4b75: 8327755: Convert javax/swing/JScrollBar/8039464/Test8039464.java applet to main
  • ... and 62 more: https://git.openjdk.org/jdk/compare/63dd6d1ac5b367e9c475779349581506d5c81d16...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 Pull request is ready to be integrated label Mar 13, 2024
@shipilev
Copy link
Member Author

Thanks! Any additional reviews, maybe @TobiHartmann or @chhagedorn ?

Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me, too.

@shipilev
Copy link
Member Author

All right, thanks! I checked that both fastdebug and release binaries work well with java.base tests too.

It also improves large CTW run times significantly. We are able to CTW 130K JARs in 24 hours now, about 3x improvement.

/integrate

@openjdk
Copy link

openjdk bot commented Mar 14, 2024

Going to push as commit 1281e18.
Since your change was applied there have been 72 commits pushed to the master branch:

  • 49ce85f: 8327874: Convert javax/swing/JTree/4314199/bug4314199.java applet test to main
  • 481c866: 8327468: Do not restart close if errno is EINTR [macOS/linux]
  • 44aef38: 8327045: Consolidate -fvisibility=hidden as a basic flag for all compilation
  • fcf746d: 8328106: COMPARE_BUILD improvements
  • fadc4b1: 8327423: C2 remove_main_post_loops: check if main-loop belongs to pre-loop, not just assert
  • cff0747: 8326204: yield statements doesn't allow cast expressions with more than 1 type arguments
  • 6f2676d: 8328064: Remove obsolete comments in constantPool and metadataFactory
  • 7502dc9: 8328030: Convert javax/swing/text/GlyphView/4984669/bug4984669.java applet test to main
  • 357c912: 8325897: Parallel: Remove PSYoungGen::is_maximal_no_gc
  • 98e4b75: 8327755: Convert javax/swing/JScrollBar/8039464/Test8039464.java applet to main
  • ... and 62 more: https://git.openjdk.org/jdk/compare/63dd6d1ac5b367e9c475779349581506d5c81d16...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 14, 2024
@openjdk openjdk bot closed this Mar 14, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 14, 2024
@openjdk
Copy link

openjdk bot commented Mar 14, 2024

@shipilev Pushed as commit 1281e18.

💡 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-compiler hotspot-compiler-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

3 participants