8360540: nmethod entry barriers of new nmethods should be disarmed

[TheRealMDoerr]

trafficstars

Adding disarm calls as ZGC and ShenandoahGC already have (see JBS issue for more details). Tier1-4 tests have passed.

I will need reviews from GC experts to check for potentially problematic side effects. Note that rebuild_code_roots() disarms all nmethods during G1FullCollector::complete_collection, now.

Some stats (nmethod entry barrier hits in jvm98 with/without patch):

GC	before	after
G1	3339	127
Parallel	3223	1141
Serial	3206	1142

---

Progress

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

Warning

 ⚠️ Found leading lowercase letter in issue title for 8360540: nmethod entry barriers of new nmethods should be disarmed

Issue

• JDK-8360540: nmethod entry barriers of new nmethods should be disarmed (Enhancement - P3)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25982

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper[bot]]

:wave: Welcome back mdoerr! 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[bot]]

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

8360540: nmethod entry barriers of new nmethods should be disarmed

Reviewed-by: eosterlund, tschatzl

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

• f0498c2aed761d4023917bc9cd1f852a02ce977a: 8364764: java/nio/channels/vthread/BlockingChannelOps.java subtests timed out
• 8e4485699235caff0074c4d25ee78539e57da63a: 8365180: Remove sun.awt.windows.WInputMethod.finalize()
• 558d06399c7a13b247ee3d0f36f4fe6118004c55: 8361536: [s390x] Saving return_pc at wrong offset
• ... and 688 more: https://git.openjdk.org/jdk/compare/f2ef809719cbb14f90a0a5f673e10e7c74fa0f45...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[bot]]

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

• hotspot-gc

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.

[mlbridge[bot]]

Webrevs

• 00: Full (6461a288)

[TheRealMDoerr]

Thanks for the review! I hope we can find somebody for a 2nd review.

[albertnetymk]

Note that rebuild_code_roots() disarms all nmethods during G1FullCollector::complete_collection, now.

Would that interfere with the coldness estimate?

  // 2. Used at the end of (stw/concurrent) marking so that nmethod::_gc_epoch
  //    is up-to-date, which provides more accurate estimate of
  //    nmethod::is_cold.
  static void arm_all_nmethods();

[TheRealMDoerr]

G1FullCollector::complete_collection

complete_collection happens after G1CollectedHeap::finish_codecache_marking_cycle(), so updating all nmethods sounds good to me. But this is actually what I'd like to have double-checked.

[albertnetymk]

My understanding on how arming-at-marking-end works is that java threads will hit those armed nmethods after full-gc-pause-end, run nmethod-entry-barrier, and mark them as "hot" (mark_as_maybe_on_stack), then in the next full-gc, unloading logic will identify those "hot" nmethods and not unload them. IOW, we use the btw-full-gc window to measure nmethod-temperature.

[tschatzl]

My understanding on how arming-at-marking-end works is that java threads will hit those armed nmethods after full-gc-pause-end, run nmethod-entry-barrier, and mark them as "hot" (mark_as_maybe_on_stack), then in the next full-gc, unloading logic will identify those "hot" nmethods and not unload them. IOW, we use the btw-full-gc window to measure nmethod-temperature.

This has also been my concern when looking at this, however if ZGC/Shenandoah do not care, then (not directed at @albertnetymk , just in general) why should the others? It would be nice to be uniform here (another random musing), either way is fine. Maybe the ZGC/Shenandoah devs can comment on this.

[bridgekeeper[bot]]

@TheRealMDoerr This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[TheRealMDoerr]

How can we proceed? Should we handle the related issues separately or is anyone intending to work on a bigger change which solves all related issues?

[TheRealMDoerr]

Thanks! Do we need follow-up RFEs? /integrate

[openjdk[bot]]

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

• f0498c2aed761d4023917bc9cd1f852a02ce977a: 8364764: java/nio/channels/vthread/BlockingChannelOps.java subtests timed out
• 8e4485699235caff0074c4d25ee78539e57da63a: 8365180: Remove sun.awt.windows.WInputMethod.finalize()
• 558d06399c7a13b247ee3d0f36f4fe6118004c55: 8361536: [s390x] Saving return_pc at wrong offset
• ... and 688 more: https://git.openjdk.org/jdk/compare/f2ef809719cbb14f90a0a5f673e10e7c74fa0f45...master

Your commit was automatically rebased without conflicts.

[openjdk[bot]]

@TheRealMDoerr Pushed as commit e1c58f858a64853c2d454fd00a84455ca6700055.

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

[tschatzl]

We should probably just call mark_as_maybe_on_stack() in the RebuildCodeRootClosure::do_nmethod() to keep the hotness calculation as before.

[tschatzl]

(I was too slow :) )

[tschatzl]

Filed https://bugs.openjdk.org/browse/JDK-8365976, working on it.

[tschatzl]

Actually there is already a call at end of G1 full GC to arm all (remaining) nmethods, in G1FullCollector::collect(), G1CollectedHeap::finish_codecache_marking_cycle()does that. So everything should be fine already.