8280917: Simplify G1ConcurrentRefineThread activation #7282

kimbarrett · 2022-01-31T00:08:10Z

Please review this change to to G1ConcurrentRefineThreads to simplify the
activation mechanism.

We split the class into two subclasses, one for the "primary" refinement
thread and another class for the remaining refinement threads.

The secondary threads are changed to use Monitor-based waits and notifications
with a supporting request flag.

The primary thread uses an atomic activation threshold that also serves as "is
running" state. This activation threshold is used by the write barrier
support to determine whether the thread's semaphore needs to be signaled.
That threshold is now in the primary refinement thread rather than in the
dirty card queue set to simplify threshold management and somewhat reduce
coupling.

Testing:
mach5 tier1-3

Manual tests with refinement thread logging enabled to verify expected
activations occur.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8280917: Simplify G1ConcurrentRefineThread activation

Reviewers

Ivan Walulya (@walulyai - Reviewer) ⚠️ Review applies to 070e92a
Stefan Johansson (@kstefanj - Reviewer) ⚠️ Review applies to 070e92a

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7282

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

Using diff file

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

bridgekeeper · 2022-01-31T00:09:14Z

👋 Welcome back kbarrett! 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 · 2022-01-31T00:11:24Z

@kimbarrett 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 · 2022-01-31T00:14:25Z

Webrevs

kimbarrett · 2022-02-02T00:22:50Z

src/hotspot/share/gc/g1/g1ConcurrentRefineThread.cpp

+    // the primary thread if needed.  Non-GC safepoints are expected to
+    // rarely (if ever) dirty cards, so defer activation to a post-safepoint
+    // notification.
+    if (SafepointSynchronize::is_at_safepoint()) {


@walulyai noticed that this test was backward. Difference in testing is often surprisingly subtle, I suspect because there's enough other wonkiness in the refinement thread control to often mask the effect. This change as a whole is intended as a step toward fixing that. I'm sure glad Ivan spotted this now though.

I re-ran tests and ran some additional with this mistake fixed. No problems encountered. I also did some better performance testing. specjbb2015 with options that will trigger the need for more concurrent refinement was unchanged between baseline and with the above fixed. Having the test backward produced a 6-7% regression. Thanks again Ivan.

walulyai

Lgtm!

openjdk · 2022-02-03T09:42:40Z

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

8280917: Simplify G1ConcurrentRefineThread activation

Reviewed-by: iwalulya, sjohanss

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 1 new commit pushed to the master branch:

f2a9627: 8279329: Remove hardcoded IPv4 available policy on Windows

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

kstefanj

Looks good, just a couple of small comments.

kstefanj · 2022-02-07T10:40:36Z

src/hotspot/share/gc/g1/g1ConcurrentRefineThread.hpp


  // Called when no refinement work found for this thread.
  // Returns true if should deactivate.
-  bool maybe_deactivate(bool more_work);
+  // precondition: this is the current thread.
+  virtual bool maybe_deactivate() = 0;


I know the name is pre-existing, but I've always found the naming it a bit odd. Or we usually call thing like this should_deactivate(). If you prefer keeping the old name I'm fine with it, just wanted to mention it since we are changing the code.

I'm not especially fond that name, but a should_ prefix would incorrectly suggest it's "just" a predicate, but it has side effects. I have some further changes coming that might do things a bit differently. I'll keep the naming in mind.

kstefanj · 2022-02-07T11:08:44Z

src/hotspot/share/gc/g1/g1ConcurrentRefineThread.hpp

+class G1PrimaryConcurrentRefineThread final : public G1ConcurrentRefineThread {
+  Semaphore _notifier;
+  DEFINE_PAD_MINUS_SIZE(0, DEFAULT_CACHE_LINE_SIZE, 0);
+  volatile size_t _threshold;


Might make sense to add some additional comment for the threshold, similar to the description you have in the PR summary:

The primary thread uses an atomic activation threshold that also serves as "is running" state. This activation threshold is used by the write barrier support to determine whether the thread's semaphore needs to be signaled.

I meant to do that, and then forgot. Thanks for point it out. I'll add it before pushing.

kimbarrett · 2022-02-08T09:01:28Z

Thanks @walulyai and @kstefanj for reviews.

kimbarrett · 2022-02-08T09:01:42Z

/integrate

openjdk · 2022-02-08T09:03:02Z

Going to push as commit 861f279.
Since your change was applied there has been 1 commit pushed to the master branch:

f2a9627: 8279329: Remove hardcoded IPv4 available policy on Windows

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-08T09:03:17Z

@kimbarrett Pushed as commit 861f279.

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

refactor refinement threads

b72cf4f

openjdk bot added the rfr Pull request is ready for review label Jan 31, 2022

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Jan 31, 2022

kimbarrett added 2 commits February 1, 2022 02:08

update copyrights

ba3f9a4

fix primary thread notification safepoint check

070e92a

kimbarrett commented Feb 2, 2022

View reviewed changes

Merge branch 'master' into split-crt

342e1a7

walulyai approved these changes Feb 3, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 3, 2022

kstefanj approved these changes Feb 7, 2022

View reviewed changes

kimbarrett added 2 commits February 8, 2022 03:18

add some description comments

5192502

Merge branch 'master' into split-crt

4051881

openjdk bot added the integrated Pull request has been integrated label Feb 8, 2022

openjdk bot closed this Feb 8, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 8, 2022

kimbarrett deleted the split-crt branch February 8, 2022 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8280917: Simplify G1ConcurrentRefineThread activation #7282

8280917: Simplify G1ConcurrentRefineThread activation #7282

kimbarrett commented Jan 31, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 31, 2022

openjdk bot commented Jan 31, 2022

mlbridge bot commented Jan 31, 2022 •

edited

Loading

kimbarrett Feb 2, 2022

kimbarrett Feb 5, 2022

walulyai left a comment

openjdk bot commented Feb 3, 2022 •

edited

Loading

kstefanj left a comment

kstefanj Feb 7, 2022

kimbarrett Feb 8, 2022

kstefanj Feb 7, 2022

kimbarrett Feb 8, 2022

kimbarrett commented Feb 8, 2022

kimbarrett commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

8280917: Simplify G1ConcurrentRefineThread activation #7282

8280917: Simplify G1ConcurrentRefineThread activation #7282

Conversation

kimbarrett commented Jan 31, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 31, 2022

openjdk bot commented Jan 31, 2022

mlbridge bot commented Jan 31, 2022 • edited Loading

Webrevs

kimbarrett Feb 2, 2022

Choose a reason for hiding this comment

kimbarrett Feb 5, 2022

Choose a reason for hiding this comment

walulyai left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 3, 2022 • edited Loading

kstefanj left a comment

Choose a reason for hiding this comment

kstefanj Feb 7, 2022

Choose a reason for hiding this comment

kimbarrett Feb 8, 2022

Choose a reason for hiding this comment

kstefanj Feb 7, 2022

Choose a reason for hiding this comment

kimbarrett Feb 8, 2022

Choose a reason for hiding this comment

kimbarrett commented Feb 8, 2022

kimbarrett commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

openjdk bot commented Feb 8, 2022

kimbarrett commented Jan 31, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 31, 2022 •

edited

Loading

openjdk bot commented Feb 3, 2022 •

edited

Loading