Reviewers: Hannes Payer, rmcilroy,
Message:
PTAL
Description:
Restore NothingOrDone action in idle time handler.
This also adjusts transitioning between modes so that crbug.com/460090
remains
fixed.
BUG=chromium:489323, chromium:460090
LOG=NO
Please review this at https://codereview.chromium.org/1141393002/
Base URL: https://chromium.googlesource.com/v8/v8.git@master
Affected files (+63, -24 lines):
M src/heap/gc-idle-time-handler.h
M src/heap/gc-idle-time-handler.cc
M test/unittests/heap/gc-idle-time-handler-unittest.cc
Index: src/heap/gc-idle-time-handler.cc
diff --git a/src/heap/gc-idle-time-handler.cc
b/src/heap/gc-idle-time-handler.cc
index
40c2eff3f96c56ff8e63cc815029a13704545b82..352593e6c42605b3194e8eb22717614bf1399b48
100644
--- a/src/heap/gc-idle-time-handler.cc
+++ b/src/heap/gc-idle-time-handler.cc
@@ -192,6 +192,17 @@ bool
GCIdleTimeHandler::ShouldDoOverApproximateWeakClosure(
}
+GCIdleTimeAction GCIdleTimeHandler::NothingOrDone() {
+ if (idle_times_which_made_no_progress_per_mode_ >=
+ kMaxNoProgressIdleTimesPerMode) {
+ return GCIdleTimeAction::Done();
+ } else {
+ idle_times_which_made_no_progress_per_mode_++;
+ return GCIdleTimeAction::Nothing();
+ }
+}
+
+
// The idle time handler has three modes and transitions between them
// as shown in the diagram:
//
@@ -306,13 +317,13 @@ GCIdleTimeAction GCIdleTimeHandler::Action(double
idle_time_in_ms,
if (heap_state.sweeping_completed) {
return GCIdleTimeAction::FinalizeSweeping();
} else {
- return GCIdleTimeAction::Nothing();
+ return NothingOrDone();
}
}
if (heap_state.incremental_marking_stopped &&
!heap_state.can_start_incremental_marking && !reduce_memory) {
- return GCIdleTimeAction::Nothing();
+ return NothingOrDone();
}
size_t step_size = EstimateMarkingStepSize(
@@ -324,19 +335,20 @@ GCIdleTimeAction GCIdleTimeHandler::Action(double
idle_time_in_ms,
void GCIdleTimeHandler::UpdateCounters(double idle_time_in_ms) {
if (mode_ == kReduceLatency) {
- int mutator_gcs = scavenges_ + mark_compacts_ - idle_mark_compacts_;
- if (mutator_gcs > 0) {
+ int gcs = scavenges_ + mark_compacts_;
+ if (gcs > 0) {
// There was a mutator GC since the last notification.
long_idle_notifications_ = 0;
}
idle_mark_compacts_ = 0;
mark_compacts_ = 0;
scavenges_ = 0;
+ // Go to reduce memory mode after two large idle notifications.
+ const int kLargeIdleIncrement =
+ kLongIdleNotificationsBeforeMutatorIsIdle / 2;
if (idle_time_in_ms >= kMinLongIdleTime) {
long_idle_notifications_ +=
- (idle_time_in_ms >= kLargeLongIdleTime)
- ? kLongIdleNotificationsBeforeMutatorIsIdle
- : 1;
+ (idle_time_in_ms >= kLargeLongIdleTime) ? kLargeIdleIncrement :
1;
}
}
}
@@ -347,11 +359,14 @@ void GCIdleTimeHandler::ResetCounters() {
idle_mark_compacts_ = 0;
mark_compacts_ = 0;
scavenges_ = 0;
+ idle_times_which_made_no_progress_per_mode_ = 0;
}
-bool GCIdleTimeHandler::IsMutatorActive(int contexts_disposed, int gcs) {
- return contexts_disposed > 0 || gcs >= kGCsBeforeMutatorIsActive;
+bool GCIdleTimeHandler::IsMutatorActive(int contexts_disposed,
+ int mark_compacts) {
+ return contexts_disposed > 0 ||
+ mark_compacts >= kMarkCompactsBeforeMutatorIsActive;
}
@@ -368,7 +383,7 @@ GCIdleTimeHandler::Mode GCIdleTimeHandler::NextMode(
switch (mode_) {
case kDone:
DCHECK(idle_mark_compacts_ == 0);
- if (IsMutatorActive(heap_state.contexts_disposed, mutator_gcs)) {
+ if (IsMutatorActive(heap_state.contexts_disposed, mark_compacts_)) {
return kReduceLatency;
}
break;
Index: src/heap/gc-idle-time-handler.h
diff --git a/src/heap/gc-idle-time-handler.h
b/src/heap/gc-idle-time-handler.h
index
e76178b7e676b547dcddf4d4b0e5b0d83f23cde4..137621f78995b7ec48ab724df443168b8cf2f00b
100644
--- a/src/heap/gc-idle-time-handler.h
+++ b/src/heap/gc-idle-time-handler.h
@@ -151,8 +151,9 @@ class GCIdleTimeHandler {
// the kDone mode.
static const int kMaxIdleMarkCompacts = 3;
- // The number of mutator GCs before transitioning to the kReduceLatency
mode.
- static const int kGCsBeforeMutatorIsActive = 7;
+ // The number of mutator MarkCompact GCs before transitioning to the
+ // kReduceLatency mode.
+ static const int kMarkCompactsBeforeMutatorIsActive = 1;
// Mutator is considered idle if
// 1) there is an idle notification with time >= kLargeLongIdleTime,
@@ -161,8 +162,11 @@ class GCIdleTimeHandler {
// with time >= kMinLongIdleTime and without any mutator GC in
between.
static const int kMinLongIdleTime = kMaxFrameRenderingIdleTime + 1;
static const int kLargeLongIdleTime = 900;
- static const int kLongIdleNotificationsBeforeMutatorIsIdle = 600;
-
+ static const int kLongIdleNotificationsBeforeMutatorIsIdle = 50;
+ // Number of times we will return a Nothing action in the current mode
+ // despite having idle time available before we returning a Done action
to
+ // ensure we don't keep scheduling idle tasks and making no progress.
+ static const int kMaxNoProgressIdleTimesPerMode = 10;
class HeapState {
public:
@@ -189,6 +193,7 @@ class GCIdleTimeHandler {
mark_compacts_(0),
scavenges_(0),
long_idle_notifications_(0),
+ idle_times_which_made_no_progress_per_mode_(0),
mode_(kReduceLatency) {}
GCIdleTimeAction Compute(double idle_time_in_ms, HeapState heap_state);
@@ -238,6 +243,7 @@ class GCIdleTimeHandler {
Mode NextMode(const HeapState& heap_state);
GCIdleTimeAction Action(double idle_time_in_ms, const HeapState&
heap_state,
bool reduce_memory);
+ GCIdleTimeAction NothingOrDone();
int idle_mark_compacts_;
int mark_compacts_;
@@ -245,6 +251,8 @@ class GCIdleTimeHandler {
// The number of long idle notifications with no mutator GC happening
// between the notifications.
int long_idle_notifications_;
+ // Idle notifications with no progress in the current mode.
+ int idle_times_which_made_no_progress_per_mode_;
Mode mode_;
Index: test/unittests/heap/gc-idle-time-handler-unittest.cc
diff --git a/test/unittests/heap/gc-idle-time-handler-unittest.cc
b/test/unittests/heap/gc-idle-time-handler-unittest.cc
index
357b08f88108117d8c74d91ddbf6c42df215708c..2a1aeb44959792199aae6b1e6d9f82ca70b3dad9
100644
--- a/test/unittests/heap/gc-idle-time-handler-unittest.cc
+++ b/test/unittests/heap/gc-idle-time-handler-unittest.cc
@@ -48,7 +48,11 @@ class GCIdleTimeHandlerTest : public ::testing::Test {
heap_state.can_start_incremental_marking;
for (int i = 0; i < limit; i++) {
GCIdleTimeAction action = handler()->Compute(idle_time_ms,
heap_state);
- EXPECT_EQ(incremental ? DO_INCREMENTAL_MARKING : DO_NOTHING,
action.type);
+ if (incremental) {
+ EXPECT_EQ(DO_INCREMENTAL_MARKING, action.type);
+ } else {
+ EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type);
+ }
}
handler()->Compute(idle_time_ms, heap_state);
EXPECT_EQ(GCIdleTimeHandler::kReduceMemory, handler()->mode());
@@ -73,16 +77,12 @@ class GCIdleTimeHandlerTest : public ::testing::Test {
void TransitionToReduceLatencyMode(
const GCIdleTimeHandler::HeapState& heap_state) {
EXPECT_EQ(GCIdleTimeHandler::kDone, handler()->mode());
- int limit = GCIdleTimeHandler::kGCsBeforeMutatorIsActive;
+ int limit = GCIdleTimeHandler::kMarkCompactsBeforeMutatorIsActive;
double idle_time_ms = GCIdleTimeHandler::kMinLongIdleTime;
for (int i = 0; i < limit; i++) {
GCIdleTimeAction action = handler()->Compute(idle_time_ms,
heap_state);
EXPECT_EQ(DONE, action.type);
- if (i % 2 == 0) {
- handler()->NotifyScavenge();
- } else {
- handler()->NotifyMarkCompact();
- }
+ handler()->NotifyMarkCompact();
}
handler()->Compute(idle_time_ms, heap_state);
EXPECT_EQ(GCIdleTimeHandler::kReduceLatency, handler()->mode());
@@ -550,7 +550,7 @@ TEST_F(GCIdleTimeHandlerTest, SmallIdleTimeNothingToDo)
{
heap_state.can_start_incremental_marking = false;
for (int i = 0; i < kMaxNotifications; i++) {
GCIdleTimeAction action = handler()->Compute(10, heap_state);
- EXPECT_EQ(DO_NOTHING, action.type);
+ EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type);
}
}
@@ -563,7 +563,7 @@ TEST_F(GCIdleTimeHandlerTest,
StayInReduceLatencyModeBecauseOfScavenges) {
int limit = GCIdleTimeHandler::kLongIdleNotificationsBeforeMutatorIsIdle;
for (int i = 0; i < kMaxNotifications; i++) {
GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
- EXPECT_EQ(DO_NOTHING, action.type);
+ EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type);
if ((i + 1) % limit == 0) handler()->NotifyScavenge();
EXPECT_EQ(GCIdleTimeHandler::kReduceLatency, handler()->mode());
}
@@ -578,7 +578,7 @@ TEST_F(GCIdleTimeHandlerTest,
StayInReduceLatencyModeBecauseOfMarkCompacts) {
int limit = GCIdleTimeHandler::kLongIdleNotificationsBeforeMutatorIsIdle;
for (int i = 0; i < kMaxNotifications; i++) {
GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
- EXPECT_EQ(DO_NOTHING, action.type);
+ EXPECT_TRUE(DO_NOTHING == action.type || DONE == action.type);
if ((i + 1) % limit == 0) handler()->NotifyMarkCompact();
EXPECT_EQ(GCIdleTimeHandler::kReduceLatency, handler()->mode());
}
@@ -643,5 +643,21 @@ TEST_F(GCIdleTimeHandlerTest, ReduceMemoryToDone) {
}
+TEST_F(GCIdleTimeHandlerTest, Regress489323) {
+ GCIdleTimeHandler::HeapState heap_state = DefaultHeapState();
+ // Simulate incremental marking stopped and not eligible to start.
+ heap_state.incremental_marking_stopped = true;
+ heap_state.can_start_incremental_marking = false;
+ double idle_time_ms = 10.0;
+ for (int i = 0; i < GCIdleTimeHandler::kMaxNoProgressIdleTimesPerMode;
i++) {
+ GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
+ EXPECT_EQ(DO_NOTHING, action.type);
+ }
+ // We should return DONE after not making progress for some time.
+ GCIdleTimeAction action = handler()->Compute(idle_time_ms, heap_state);
+ EXPECT_EQ(DONE, action.type);
+}
+
+
} // namespace internal
} // namespace v8
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
