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

8256938: Improve remembered set sampling task scheduling #1428

Closed

Conversation

kstefanj
Copy link
Contributor

@kstefanj kstefanj commented Nov 25, 2020

Please review this change to the remembered set sampling task scheduling.

The sampling tasks joins the suspendible thread set (STS) and if it needs to yield during the sampling it aborts and reschedules. This is the expected behavior and will lead to the sampling happening roughly G1ConcRefinementServiceIntervalMillis after the pause ended. But in the case where the task is started during a GC (can happen since the whole service thread isn't joined to the STS) the sampling will start right after the pause has finished. This enhancement changes this by checking when the last pause occurred and reschedules the task to take place G1ConcRefinementServiceIntervalMillis after the GC ended.

Testing
Tier 1-3 and manual verification looking at logs.


Progress

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

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build (3/6 failed) (2/2 failed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed)

Failed test tasks

Issue

  • JDK-8256938: Improve remembered set sampling task scheduling

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1428/head:pull/1428
$ git checkout pull/1428

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 25, 2020

👋 Welcome back sjohanss! 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 openjdk bot added the rfr label Nov 25, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2020

@kstefanj 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.

@openjdk openjdk bot added the hotspot-gc label Nov 25, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 25, 2020

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Nov 25, 2020

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

8256938: Improve remembered set sampling task scheduling

Reviewed-by: tschatzl, ayang

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

  • b823ad9: 8257072: ZGC: Rename roots iterators
  • 973255c: 8196100: javax/swing/text/JTextComponent/5074573/bug5074573.java fails
  • a8e3eab: 8245026: PsAdaptiveSizePolicy::_old_gen_policy_is_ready is unused
  • b1d1499: 8256956: RegisterImpl::max_slots_per_register is incorrect on AMD64
  • 20020d1: 8254360: Re-examine use of CodeBuffer::verify_section_allocation
  • e56a8df: 8257042: [aix] Disable os.release_one_mapping_multi_commits_vm gtest
  • 9d7121c: 8256713: SwingSet2 : Slider leaves tracks in uiScale=2
  • 434b98f: 8257077: ZGC: Remove ZWorkers::run_serial()
  • f3fc0e0: 8257079: ZGC: Fold ZMark::prepare_mark() into ZMark::start()
  • a14f02d: 8256267: Relax compiler/floatingpoint/NaNTest.java for x86_32 and lower -XX:+UseSSE
  • ... and 16 more: https://git.openjdk.java.net/jdk/compare/1b7a61ff4a01608da64bdeb96b1936f876219425...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 Nov 25, 2020
// Reschedule if a GC happened too recently.
if (should_reschedule()) {
// Calculate the delay given the last GC and the interval.
schedule(reschedule_delay_ms());
return;
}
Copy link
Member

@albertnetymk albertnetymk Nov 25, 2020

Choose a reason for hiding this comment

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

I would prefer not having should_reschedule and reuse the return value of reschedule_delay_ms, but up to you.

delay_ms = reschedule_delay_ms();
// only reschedule when ...
if (delay_ms > 1) {
  schedule(delay_ms);
  return;
}

Second point, why 1 in reschedule_delay_ms() > 1, but not 0?

Copy link
Contributor Author

@kstefanj kstefanj Nov 25, 2020

Choose a reason for hiding this comment

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

I tried to cover the why in the comment for should_reschedule():

  // To avoid extensive rescheduling if the task is executed a bit early. The task is
  // only rescheduled if the expected time is more than 1ms away.
  bool should_reschedule() {
    return reschedule_delay_ms() > 1;
  }

But maybe this comment needs to be clearer. The problem is that when calculating the reschedule_delay_ms() the since_last_gc.milliseconds() call might round down to 299ms (when the interval is 300ms), even if we are just a microsecond away. And since there are different timestamps used to decide if the task is ready to run, we can end up in this situation.

Having this comment is one of the reasons I used two functions even if it meant calculating the delay two times. Makes sense? Would you like more info in the comment?

Copy link
Member

@albertnetymk albertnetymk Nov 25, 2020

Choose a reason for hiding this comment

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

might round down to 299ms (when the interval is 300ms), even if we are just a microsecond away.

In that case, using > 0 will just delay the start of the task for ~1ms, right? Doesn't sound like a serious problem. Similarly, > 1 could cause the task to be run ~1ms earlier than its intended 300ms interval, right? Either way is fine, but I think >0 is less surprising.

Copy link
Contributor Author

@kstefanj kstefanj Nov 26, 2020

Choose a reason for hiding this comment

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

One would think :) But the rounding comes back the other way around, if we reschedule to 1ms in the future, the service thread will look at the timestamp a bit later and then the time left will be rounded down to 0ms. So the effect will be that the task is rescheduled and run instantly over and over until the time since GC is over the interval.

Copy link
Member

@albertnetymk albertnetymk Nov 26, 2020

Choose a reason for hiding this comment

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

Resolved my confusion via private chat. Thank you for the explanation. This PR is good to go.

@kstefanj
Copy link
Contributor Author

@kstefanj kstefanj commented Nov 26, 2020

/reviewer credit @albertnetymk

@openjdk
Copy link

@openjdk openjdk bot commented Nov 26, 2020

@kstefanj Reviewer ayang has already made an authenticated review of this PR, and does not need to be credited manually.

@kstefanj
Copy link
Contributor Author

@kstefanj kstefanj commented Nov 26, 2020

/integrate

@openjdk openjdk bot closed this Nov 26, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 26, 2020
@kstefanj kstefanj deleted the 8256938-remset-task-schedule branch Nov 26, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 26, 2020

@kstefanj Since your change was applied there have been 26 commits pushed to the master branch:

  • b823ad9: 8257072: ZGC: Rename roots iterators
  • 973255c: 8196100: javax/swing/text/JTextComponent/5074573/bug5074573.java fails
  • a8e3eab: 8245026: PsAdaptiveSizePolicy::_old_gen_policy_is_ready is unused
  • b1d1499: 8256956: RegisterImpl::max_slots_per_register is incorrect on AMD64
  • 20020d1: 8254360: Re-examine use of CodeBuffer::verify_section_allocation
  • e56a8df: 8257042: [aix] Disable os.release_one_mapping_multi_commits_vm gtest
  • 9d7121c: 8256713: SwingSet2 : Slider leaves tracks in uiScale=2
  • 434b98f: 8257077: ZGC: Remove ZWorkers::run_serial()
  • f3fc0e0: 8257079: ZGC: Fold ZMark::prepare_mark() into ZMark::start()
  • a14f02d: 8256267: Relax compiler/floatingpoint/NaNTest.java for x86_32 and lower -XX:+UseSSE
  • ... and 16 more: https://git.openjdk.java.net/jdk/compare/1b7a61ff4a01608da64bdeb96b1936f876219425...master

Your commit was automatically rebased without conflicts.

Pushed as commit f6d6a07.

💡 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-gc integrated
3 participants