Replace reduce-memory mode in idle notification with delayed clean-up GC.

Replace reduce-memory mode in idle notification with delayed clean-up GC.

BUG=490559
LOG=NO

Committed: https://crrev.com/0ecd9e1bd2c6b519d4e7285f46cb7e844bc2235c
Cr-Commit-Position: refs/heads/master@{#29451}

[ulan, 2015-06-30 11:01:15]

ulan@chromium.org changed reviewers:
+ hpayer@chromium.org

[ulan, 2015-06-30 11:01:35]

PTAL

[Hannes Payer (out of office), 2015-07-01 12:03:17]

First round. Really nice cleanup, already looking good. Just a few minor
comments (so far).

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.cc
File src/heap/cleanup-gc.cc (right):

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.cc...
src/heap/cleanup-gc.cc:132: void CleanupGC::ScheduleTimer(double delay_ms) {
Can we DCHECK that delay_ms is always positive.

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.cc...
src/heap/cleanup-gc.cc:137: isolate, new CleanupGC::TimerTask(this), (delay_ms +
kSlackMs) / 1000.0);
where is the delete?

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.h
File src/heap/cleanup-gc.h (right):

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.h#...
src/heap/cleanup-gc.h:17: // The goal of the CleanupGC class is to detect
transition of the mutator
Hmm... I am not a big fan of the name CleanupGC. What about MemoryController? I
am open to other alternatives...

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

https://codereview.chromium.org/1218863002/diff/180001/src/heap/gc-idle-time-...
src/heap/gc-idle-time-handler.cc:211: // (2) If the context disposal rate is
high and we cannot perform a full GC,
please update this comment

https://codereview.chromium.org/1218863002/diff/180001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1218863002/diff/180001/src/heap/heap.cc#newco...
src/heap/heap.cc:4883: CollectAllGarbage(kReduceMemoryFootprintMask,
Could this call end an ongoing incremental marking cycle?

[ulan, 2015-07-01 12:51:48]

Thanks. Uploaded new PS.

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.cc
File src/heap/cleanup-gc.cc (right):

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.cc...
src/heap/cleanup-gc.cc:132: void CleanupGC::ScheduleTimer(double delay_ms) {
On 2015/07/01 12:03:16, Hannes Payer wrote:
> Can we DCHECK that delay_ms is always positive.

Done.

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.cc...
src/heap/cleanup-gc.cc:137: isolate, new CleanupGC::TimerTask(this), (delay_ms +
kSlackMs) / 1000.0);
On 2015/07/01 12:03:16, Hannes Payer wrote:
> where is the delete?

Platform will delete it.

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.h
File src/heap/cleanup-gc.h (right):

https://codereview.chromium.org/1218863002/diff/180001/src/heap/cleanup-gc.h#...
src/heap/cleanup-gc.h:17: // The goal of the CleanupGC class is to detect
transition of the mutator
On 2015/07/01 12:03:16, Hannes Payer wrote:
> Hmm... I am not a big fan of the name CleanupGC. What about MemoryController?
I
> am open to other alternatives... 

Renamed to MemoryReducer as discussed.

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

https://codereview.chromium.org/1218863002/diff/180001/src/heap/gc-idle-time-...
src/heap/gc-idle-time-handler.cc:211: // (2) If the context disposal rate is
high and we cannot perform a full GC,
On 2015/07/01 12:03:17, Hannes Payer wrote:
> please update this comment

Done.

https://codereview.chromium.org/1218863002/diff/180001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1218863002/diff/180001/src/heap/heap.cc#newco...
src/heap/heap.cc:4883: CollectAllGarbage(kReduceMemoryFootprintMask,
On 2015/07/01 12:03:17, Hannes Payer wrote:
> Could this call end an ongoing incremental marking cycle?

I removed this completely and added DCHECK(context_disposed_ > 0).

[Hannes Payer (out of office), 2015-07-02 13:19:06]

LGTM, if memory and latency did not regression. Super nice cleanup! Just a few
nits left.

https://codereview.chromium.org/1218863002/diff/240001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1218863002/diff/240001/BUILD.gn#newcode832
BUILD.gn:832: "src/heap/memory-reducer.cc",
alphabetic order

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
File src/heap/memory-reducer.cc (right):

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
src/heap/memory-reducer.cc:1: // Copyright 2014 the V8 project authors. All
rights reserved.
2015

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
src/heap/memory-reducer.cc:41: PrintIsolate(heap()->isolate(), "Clean-up GC:
started #%d\n",
Please don't use the term Cleanup-gc, also below.

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reducer.h
File src/heap/memory-reducer.h (right):

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
src/heap/memory-reducer.h:1: // Copyright 2014 the V8 project authors. All
rights reserved.
2015

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
src/heap/memory-reducer.h:72: explicit State(Action action, int started_gcs,
double next_gc_start_ms)
I think the code style says that this should be a method called Initialize.

[ulan, 2015-07-02 13:30:16]

Thanks!

https://codereview.chromium.org/1218863002/diff/240001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/1218863002/diff/240001/BUILD.gn#newcode832
BUILD.gn:832: "src/heap/memory-reducer.cc",
On 2015/07/02 13:19:06, Hannes Payer wrote:
> alphabetic order

Done.

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
File src/heap/memory-reducer.cc (right):

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
src/heap/memory-reducer.cc:1: // Copyright 2014 the V8 project authors. All
rights reserved.
On 2015/07/02 13:19:06, Hannes Payer wrote:
> 2015

Done.

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
src/heap/memory-reducer.cc:41: PrintIsolate(heap()->isolate(), "Clean-up GC:
started #%d\n",
On 2015/07/02 13:19:06, Hannes Payer wrote:
> Please don't use the term Cleanup-gc, also below.

Done.

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reducer.h
File src/heap/memory-reducer.h (right):

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
src/heap/memory-reducer.h:1: // Copyright 2014 the V8 project authors. All
rights reserved.
On 2015/07/02 13:19:06, Hannes Payer wrote:
> 2015

Done.

https://codereview.chromium.org/1218863002/diff/240001/src/heap/memory-reduce...
src/heap/memory-reducer.h:72: explicit State(Action action, int started_gcs,
double next_gc_start_ms)
On 2015/07/02 13:19:06, Hannes Payer wrote:
> I think the code style says that this should be a method called Initialize.

As discussed offline: I removed "explicit" and leaving the constructor since it
makes code cleaner.

[ulan, 2015-07-02 13:30:44]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-07-02 13:30:45]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1218863002/#ps260001
(title: "Address comments")

[commit-bot: I haz the power, 2015-07-02 13:30:52]

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

[commit-bot: I haz the power, 2015-07-02 13:38:01]

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

[commit-bot: I haz the power, 2015-07-02 13:38:02]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)

[ulan, 2015-07-02 13:42:58]

The CQ bit was unchecked by ulan@chromium.org

[ulan, 2015-07-02 14:39:38]

I removed the scavenge event, since the check for allocation throughput should
subsume it. I also remove the check for high fragmentation to keep it simple for
now.

[ulan, 2015-07-02 15:06:03]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-07-02 15:06:03]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org
Link to the patchset: https://codereview.chromium.org/1218863002/#ps320001
(title: "Fix test")

[commit-bot: I haz the power, 2015-07-02 15:06:18]

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

[commit-bot: I haz the power, 2015-07-02 15:41:33]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-07-02 15:41:46]

Patchset 17 (id:??) landed as
https://crrev.com/0ecd9e1bd2c6b519d4e7285f46cb7e844bc2235c
Cr-Commit-Position: refs/heads/master@{#29451}

[Michael Achenbach, 2015-07-05 17:49:12]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1226703002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Looks like it blocks the roll (bisected).
Speculative revert.
https://codereview.chromium.org/1210293003/.