Simplified garbage collection idle handler.

Simplified garbage collection idle handler.

The current GC idle time handling heuristics are getting too complicated. Moreover, with longer idle time we are getting more full garbage collections. This CL shrinks the idle round window and reduces complexity in the case where we cause a full garbage collection.

BUG=chromium:468554
LOG=n

Committed: https://crrev.com/c293448f3e548a0c8e1e5daa3ba6601c0c380574
Cr-Commit-Position: refs/heads/master@{#27493}

[Hannes Payer (out of office), 2015-03-23 10:55:25]

hpayer@chromium.org changed reviewers:
+ erikcorry@chromium.org, jochen@chromium.org, rmcilroy@chromium.org,
ulan@chromium.org

[Hannes Payer (out of office), 2015-03-23 10:55:26]

https://codereview.chromium.org/1024043003/diff/1/src/heap/gc-idle-time-handl...
File src/heap/gc-idle-time-handler.cc (right):

https://codereview.chromium.org/1024043003/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.cc:227: if (IsMarkCompactIdleRoundFinished()) {
I am still not happy about the idle round concept. This one triggers
aggressively full garbage collections. Maybe it should be *perform garbage
collection if there was no garbage collection for a long time*.

https://codereview.chromium.org/1024043003/diff/1/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/1024043003/diff/1/test/cctest/test-api.cc#old...
test/cctest/test-api.cc:15763: 
These tests are identical and do not test anything.

[jochen (gone - plz use gerrit), 2015-03-23 13:10:14]

lgtm

[ulan, 2015-03-23 13:21:14]

lgtm

[Erik Corry, 2015-03-23 14:38:38]

erik.corry@gmail.com changed reviewers:
+ erik.corry@gmail.com

[Erik Corry, 2015-03-23 14:38:39]

It's a good simplification.

We have 3 triggers for a GC when this lands:

IncrementalMarking: kActivationThreshold: 8Mbytes promoted
GCIdleTimeHandler: KIdleScavengeThreshold: 5 new-space GCs
Heap::OldGenerationAllocationLimit: Growth of 1.1x to 4x depending on handles
freed at last GC.

LGTM

[rmcilroy, 2015-03-24 07:26:35]

On 2015/03/23 14:38:39, Erik Corry wrote:
> It's a good simplification.
> 
> We have 3 triggers for a GC when this lands:
> 
> IncrementalMarking: kActivationThreshold: 8Mbytes promoted
> GCIdleTimeHandler: KIdleScavengeThreshold: 5 new-space GCs
> Heap::OldGenerationAllocationLimit: Growth of 1.1x to 4x depending on handles
> freed at last GC.
> 
> LGTM

Lgtm

[Hannes Payer (out of office), 2015-03-24 08:14:07]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2015-03-24 08:14:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1024043003/1

[commit-bot: I haz the power, 2015-03-24 08:39:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-24 08:39:07]

Failed to apply patch for test/cctest/test-api.cc:
While running git apply --index -3 -p1;
  error: patch failed: test/cctest/test-api.cc:15742
  Falling back to three-way merge...
  Applied patch to 'test/cctest/test-api.cc' with conflicts.
  U test/cctest/test-api.cc

Patch:       test/cctest/test-api.cc
Index: test/cctest/test-api.cc
diff --git a/test/cctest/test-api.cc b/test/cctest/test-api.cc
index
dcb11be19d1c84a377b2381a7d3929aff4bbe3a1..2288f44c6d82a92b255bfbbe40b14955b099b3fc
100644
--- a/test/cctest/test-api.cc
+++ b/test/cctest/test-api.cc
@@ -15742,46 +15742,6 @@ TEST(IdleNotification) {
 }
 
 
-// Test that idle notification can be handled and eventually collects garbage.
-TEST(IdleNotificationWithSmallHint) {
-  const intptr_t MB = 1024 * 1024;
-  const int IdlePauseInMs = 900;
-  LocalContext env;
-  v8::HandleScope scope(env->GetIsolate());
-  intptr_t initial_size = CcTest::heap()->SizeOfObjects();
-  CreateGarbageInOldSpace();
-  intptr_t size_with_garbage = CcTest::heap()->SizeOfObjects();
-  CHECK_GT(size_with_garbage, initial_size + MB);
-  bool finished = false;
-  for (int i = 0; i < 200 && !finished; i++) {
-    finished = env->GetIsolate()->IdleNotification(IdlePauseInMs);
-  }
-  intptr_t final_size = CcTest::heap()->SizeOfObjects();
-  CHECK(finished);
-  CHECK_LT(final_size, initial_size + 1);
-}
-
-
-// Test that idle notification can be handled and eventually collects garbage.
-TEST(IdleNotificationWithLargeHint) {
-  const intptr_t MB = 1024 * 1024;
-  const int IdlePauseInMs = 900;
-  LocalContext env;
-  v8::HandleScope scope(env->GetIsolate());
-  intptr_t initial_size = CcTest::heap()->SizeOfObjects();
-  CreateGarbageInOldSpace();
-  intptr_t size_with_garbage = CcTest::heap()->SizeOfObjects();
-  CHECK_GT(size_with_garbage, initial_size + MB);
-  bool finished = false;
-  for (int i = 0; i < 200 && !finished; i++) {
-    finished = env->GetIsolate()->IdleNotification(IdlePauseInMs);
-  }
-  intptr_t final_size = CcTest::heap()->SizeOfObjects();
-  CHECK(finished);
-  CHECK_LT(final_size, initial_size + 1);
-}
-
-
 TEST(Regress2333) {
   LocalContext env;
   for (int i = 0; i < 3; i++) {

[commit-bot: I haz the power, 2015-03-24 08:39:09]

The CQ bit was unchecked by commit-bot@chromium.org

[Hannes Payer (out of office), 2015-03-27 07:29:30]

The CQ bit was checked by hpayer@chromium.org

[Hannes Payer (out of office), 2015-03-27 07:29:31]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
jochen@chromium.org, rmcilroy@chromium.org, erik.corry@gmail.com
Link to the patchset: https://codereview.chromium.org/1024043003/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-03-27 07:29:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1024043003/20001

[commit-bot: I haz the power, 2015-03-27 08:00:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-27 08:01:00]

Patchset 2 (id:??) landed as
https://crrev.com/c293448f3e548a0c8e1e5daa3ba6601c0c380574
Cr-Commit-Position: refs/heads/master@{#27493}