Use idle task to perform incremental marking steps.

Use idle task to perform incremental marking steps.

This moves incremental marking steps from gc-idle-time-handler and heap to the new incremental marking task.

BUG=chromium:490559
LOG=NO

Committed: https://crrev.com/057514d3fa79af4732e136f5daad4b805a65e569
Cr-Commit-Position: refs/heads/master@{#30641}

[ulan, 2015-08-05 11:11:58]

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

[ulan, 2015-08-05 11:11:59]

PTAL

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
File src/heap/gc-idle-time-handler.h (left):

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.h:76: intptr_t parameter;
This is gone because we estimate the step size outside of the idle time handler.

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
File src/heap/gc-idle-time-handler.h (right):

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.h:16: DO_INCREMENTAL_STEP,
DO_INCREMENTAL_MARKING and DO_FINALIZE_SWEEPING are now combined into
DO_INCREMENTAL_STEP.

https://codereview.chromium.org/1265423002/diff/40001/test/unittests/heap/gc-...
File test/unittests/heap/gc-idle-time-handler-unittest.cc (left):

https://codereview.chromium.org/1265423002/diff/40001/test/unittests/heap/gc-...
test/unittests/heap/gc-idle-time-handler-unittest.cc:281:
TEST_F(GCIdleTimeHandlerTest, IncrementalMarking2) {
This test and two tests below are redundant now.

https://codereview.chromium.org/1265423002/diff/40001/test/unittests/heap/gc-...
test/unittests/heap/gc-idle-time-handler-unittest.cc:395:
TEST_F(GCIdleTimeHandlerTest, DoneIfNotMakingProgressOnSweeping) {
This is a real change. We will keep reposting idle tasks until sweeping is
finished. I don't think this is a problem because sweeping should finish
quickly.

[Hannes Payer (out of office), 2015-08-05 12:35:44]

LGTM, a few comments

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

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:249: if
(heap_state.incremental_marking_stopped) return GCIdleTimeAction::Done();
!FLAG_incremental_marking ||

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
File src/heap/gc-idle-time-handler.h (left):

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.h:76: intptr_t parameter;
On 2015/08/05 11:11:59, ulan wrote:
> This is gone because we estimate the step size outside of the idle time
handler.

ack

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
File src/heap/gc-idle-time-handler.h (right):

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.h:16: DO_INCREMENTAL_STEP,
On 2015/08/05 11:11:59, ulan wrote:
> DO_INCREMENTAL_MARKING and DO_FINALIZE_SWEEPING are now combined into
> DO_INCREMENTAL_STEP.

SGTM

https://codereview.chromium.org/1265423002/diff/40001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1265423002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:1064: }
A follow-up CL could check if there is still time after waiting for sweeper
threads and perform incremental marking steps.

https://codereview.chromium.org/1265423002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:1067: const double remaining_idle_time_in_ms =
heap->AdvanceIncrementalMarking(
AdvanceIncrementalMarking is always called with step size 0, let's remove this
parameter

[ulan, 2015-08-05 12:45:40]

Thanks!

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

https://codereview.chromium.org/1265423002/diff/40001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:249: if
(heap_state.incremental_marking_stopped) return GCIdleTimeAction::Done();
On 2015/08/05 12:35:44, Hannes Payer wrote:
> !FLAG_incremental_marking ||

Done.

https://codereview.chromium.org/1265423002/diff/40001/src/heap/incremental-ma...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/1265423002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:1064: }
On 2015/08/05 12:35:44, Hannes Payer wrote:
> A follow-up CL could check if there is still time after waiting for sweeper
> threads and perform incremental marking steps.

Acknowledged.

https://codereview.chromium.org/1265423002/diff/40001/src/heap/incremental-ma...
src/heap/incremental-marking.cc:1067: const double remaining_idle_time_in_ms =
heap->AdvanceIncrementalMarking(
On 2015/08/05 12:35:44, Hannes Payer wrote:
> AdvanceIncrementalMarking is always called with step size 0, let's remove this
> parameter

I'll do it in a separate CL to avoid conflicts with Michael's CL.

[ulan, 2015-08-05 12:55:26]

The CQ bit was checked by ulan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-05 12:55:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265423002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265423002/60001

[commit-bot: I haz the power, 2015-08-05 13:20:16]

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

[commit-bot: I haz the power, 2015-08-05 13:20:17]

Dry run: This issue passed the CQ dry run.

[ulan, 2015-09-08 09:47:59]

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

[ulan, 2015-09-08 09:48:00]

Michael, could please take a look at incremental-marking-job.* files?

[Michael Lippautz, 2015-09-08 10:16:27]

incremental-marking-job.* lgtm with one fix needed

Can you add a test using your MockPlatform in cctest/?

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
File src/heap/incremental-marking-job.cc (right):

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
src/heap/incremental-marking-job.cc:27: made_progress_since_last_delayed_task_ =
false;
This should be true, I guess. Otherwise the DelayedTask is forced to do work all
the time (?)

Also, maybe call it NotifyIdleTaskProgress() as it only indicates progress in
idle tasks and not overall (including delayed) tasks.

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
src/heap/incremental-marking-job.cc:45: const int kDelayInSeconds = 5;
nit: Can we make this a constant in IncrementalMarkingJob?

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
File src/heap/incremental-marking-job.h (right):

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
src/heap/incremental-marking-job.h:23: // which can happen for background tab in
Chrome.
nit: s/tab/tabs/

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
src/heap/incremental-marking-job.h:62: void Start(Heap* heap) {
IncrementalMarkingJob is reused as far as I see. We should reset the member
fields on start, as we might end up with made_progress_since_last_delayed_task_
= true from the last round. (After fixing NotifyProgress())

[ulan, 2015-09-08 11:34:53]

Thank you!

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
File src/heap/incremental-marking-job.cc (right):

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
src/heap/incremental-marking-job.cc:27: made_progress_since_last_delayed_task_ =
false;
On 2015/09/08 10:16:26, Michael Lippautz wrote:
> This should be true, I guess. Otherwise the DelayedTask is forced to do work
all
> the time (?)
> 
> Also, maybe call it NotifyIdleTaskProgress() as it only indicates progress in
> idle tasks and not overall (including delayed) tasks.

Good catch! This slipped through during last minute refactoring.

Fixed and renamed.

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
src/heap/incremental-marking-job.cc:45: const int kDelayInSeconds = 5;
On 2015/09/08 10:16:26, Michael Lippautz wrote:
> nit: Can we make this a constant in IncrementalMarkingJob?

Done.

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
File src/heap/incremental-marking-job.h (right):

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
src/heap/incremental-marking-job.h:23: // which can happen for background tab in
Chrome.
On 2015/09/08 10:16:26, Michael Lippautz wrote:
> nit: s/tab/tabs/

Done.

https://codereview.chromium.org/1265423002/diff/140001/src/heap/incremental-m...
src/heap/incremental-marking-job.h:62: void Start(Heap* heap) {
On 2015/09/08 10:16:26, Michael Lippautz wrote:
> IncrementalMarkingJob is reused as far as I see. We should reset the member
> fields on start, as we might end up with
made_progress_since_last_delayed_task_
> = true from the last round. (After fixing NotifyProgress())

Added a comment explaining why we shouldn't reset the fields.

[ulan, 2015-09-08 11:36:07]

> Can you add a test using your MockPlatform in cctest/?
There were test for idle tasks. I extended them with delayed tasks.

[ulan, 2015-09-08 12:00:45]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-09-08 12:00:45]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/1265423002/#ps180001
(title: "Add check to start")

[commit-bot: I haz the power, 2015-09-08 12:00:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265423002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265423002/180001

[commit-bot: I haz the power, 2015-09-08 13:03:11]

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

[commit-bot: I haz the power, 2015-09-08 13:03:12]

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

[ulan, 2015-09-08 13:32:58]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-09-08 13:32:58]

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

[commit-bot: I haz the power, 2015-09-08 13:33:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265423002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265423002/200001

[commit-bot: I haz the power, 2015-09-08 14:05:29]

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

[commit-bot: I haz the power, 2015-09-08 14:05:30]

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-09-08 14:31:56]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-09-08 14:31:56]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/1265423002/#ps220001
(title: "Delete tasks in the test")

[commit-bot: I haz the power, 2015-09-08 14:32:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265423002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265423002/220001

[commit-bot: I haz the power, 2015-09-08 15:05:11]

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

[commit-bot: I haz the power, 2015-09-08 15:05:12]

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-09-08 15:24:34]

The CQ bit was checked by ulan@chromium.org

[ulan, 2015-09-08 15:24:35]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
mlippautz@chromium.org
Link to the patchset: https://codereview.chromium.org/1265423002/#ps240001
(title: "More test fixes")

[commit-bot: I haz the power, 2015-09-08 15:24:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1265423002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1265423002/240001

[commit-bot: I haz the power, 2015-09-08 15:54:34]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-09-08 15:54:48]

Patchset 13 (id:??) landed as
https://crrev.com/057514d3fa79af4732e136f5daad4b805a65e569
Cr-Commit-Position: refs/heads/master@{#30641}