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.

Sign up for GitHub

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
wants to merge 1 commit into from
+29 −4
Closed

8256938: Improve remembered set sampling task scheduling #1428

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 29 additions & 4 deletions src/hotspot/share/gc/g1/g1RemSet.cpp
View file
Original file line number Diff line number Diff line change
@@ -548,13 +548,12 @@ class G1RemSetSamplingTask : public G1ServiceTask {
// reevaluates the prediction for the remembered set scanning costs, and potentially
// G1Policy resizes the young gen. This may do a premature GC or even
// increase the young gen size to keep pause time length goal.
void sample_young_list_rs_length(){
SuspendibleThreadSetJoiner sts;
void sample_young_list_rs_length(SuspendibleThreadSetJoiner* sts){
G1CollectedHeap* g1h = G1CollectedHeap::heap();
G1Policy* policy = g1h->policy();

if (policy->use_adaptive_young_list_length()) {
G1YoungRemSetSamplingClosure cl(&sts);
G1YoungRemSetSamplingClosure cl(sts);

G1CollectionSet* g1cs = g1h->collection_set();
g1cs->iterate(&cl);
@@ -565,10 +564,36 @@ class G1RemSetSamplingTask : public G1ServiceTask {
}
}

// 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;
}

// There is no reason to do the sampling if a GC occurred recently. We use the
// G1ConcRefinementServiceIntervalMillis as the metric for recently and calculate
// the diff to the last GC. If the last GC occurred longer ago than the interval
// 0 is returned.
jlong reschedule_delay_ms() {
Tickspan since_last_gc = G1CollectedHeap::heap()->time_since_last_collection();
jlong delay = (jlong) (G1ConcRefinementServiceIntervalMillis - since_last_gc.milliseconds());
return MAX2<jlong>(0L, delay);
}

public:
G1RemSetSamplingTask(const char* name) : G1ServiceTask(name) { }
virtual void execute() {
sample_young_list_rs_length();
SuspendibleThreadSetJoiner sts;

// 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;
}
Comment on lines +588 to +593
Copy link
Member

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

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

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

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

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.


// Do the actual sampling.
sample_young_list_rs_length(&sts);
schedule(G1ConcRefinementServiceIntervalMillis);
}
};