Check if there is still time before finalizing an incremental collection.

Check if there is still time before finalizing an incremental collection.

BUG=
R=erik.corry@gmail.com, ulan@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24567

[Hannes Payer (out of office), 2014-10-06 12:10:23]

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

[Hannes Payer (out of office), 2014-10-06 12:10:24]

adding tests...

[ulan, 2014-10-06 12:36:01]

Looks good, will review again after tests are added.

https://codereview.chromium.org/629903003/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/629903003/diff/60001/src/heap/heap.cc#newcode...
src/heap/heap.cc:4348: true, false);
enum instead of bool would be more readable here.

[Erik Corry Chromium.org, 2014-10-06 14:56:47]

erikcorry@chromium.org changed reviewers:
+ erikcorry@chromium.org

[Erik Corry Chromium.org, 2014-10-06 14:56:47]

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

https://codereview.chromium.org/629903003/diff/60001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:158: static bool DoMarkCompact(size_t
idle_time_in_ms, size_t size_of_objects,
I would prefer "ShouldDoMarkCompact" to make it clearer that this checks whether
we should do a mark-compact GC, rather than actually doing it.

https://codereview.chromium.org/629903003/diff/60001/src/heap/incremental-mar...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/629903003/diff/60001/src/heap/incremental-mar...
src/heap/incremental-marking.cc:947: IncrementIdleMarkingDelayCounter();
If we increment this every time we do a step, then I think it will always be
above 3 when we check it.  What am I missing?

[Hannes Payer (out of office), 2014-10-07 13:41:11]

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

https://codereview.chromium.org/629903003/diff/60001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:158: static bool DoMarkCompact(size_t
idle_time_in_ms, size_t size_of_objects,
On 2014/10/06 14:56:47, Erik Corry Chromium.org wrote:
> I would prefer "ShouldDoMarkCompact" to make it clearer that this checks
whether
> we should do a mark-compact GC, rather than actually doing it.

Done.

https://codereview.chromium.org/629903003/diff/60001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/629903003/diff/60001/src/heap/heap.cc#newcode...
src/heap/heap.cc:4348: true, false);
On 2014/10/06 12:36:01, ulan wrote:
> enum instead of bool would be more readable here.

Done.

https://codereview.chromium.org/629903003/diff/60001/src/heap/incremental-mar...
File src/heap/incremental-marking.cc (right):

https://codereview.chromium.org/629903003/diff/60001/src/heap/incremental-mar...
src/heap/incremental-marking.cc:947: IncrementIdleMarkingDelayCounter();
On 2014/10/06 14:56:47, Erik Corry Chromium.org wrote:
> If we increment this every time we do a step, then I think it will always be
> above 3 when we check it.  What am I missing?

Uff, good catch. We should just increment the counter when we finish marking but
do not complete. Done.

[Erik Corry Chromium.org, 2014-10-08 10:17:14]

I like the change, but I have some objections.

https://codereview.chromium.org/629903003/diff/80001/src/heap/incremental-mar...
File src/heap/incremental-marking.h (right):

https://codereview.chromium.org/629903003/diff/80001/src/heap/incremental-mar...
src/heap/incremental-marking.h:92: static const size_t
kMaxIdleMarkingDelayCounter = 3;
This is formulated in terms of a number of calls to IdleNotification. This means
a change on the embedder side that inserts extra 0.1ms calls to IdleNotification
can cause the finalization of the GC to happen much earlier.  I think that's
unfortunate: I want the scheduler to be able to say "I found a few moments of
time, use them if you can, but don't perturb your heuristics too much, based on
this".  Or perhaps "We are waiting for an important network packet, you can have
as many 1ms snippets as you want until the packet arrives, but we don't want to
delay the processing that is waiting for it when it arrives".

Perhaps this should only count idle notifications that are above a certain
threshold, or perhaps it should add up the ms and use that as the limit, rather
than the invocation count.

https://codereview.chromium.org/629903003/diff/80001/test/unittests/heap/gc-i...
File test/unittests/heap/gc-idle-time-handler-unittest.cc (right):

https://codereview.chromium.org/629903003/diff/80001/test/unittests/heap/gc-i...
test/unittests/heap/gc-idle-time-handler-unittest.cc:179: }
I think if you changed kMaxIdleMarkingDelayCounter from 3 to 1000 then this test
would still pass.  Can we test that the constant is working as intended?

[Hannes Payer (out of office), 2014-10-13 13:25:45]

https://codereview.chromium.org/629903003/diff/80001/src/heap/incremental-mar...
File src/heap/incremental-marking.h (right):

https://codereview.chromium.org/629903003/diff/80001/src/heap/incremental-mar...
src/heap/incremental-marking.h:92: static const size_t
kMaxIdleMarkingDelayCounter = 3;
On 2014/10/08 10:17:14, Erik Corry Chromium.org wrote:
> This is formulated in terms of a number of calls to IdleNotification. This
means
> a change on the embedder side that inserts extra 0.1ms calls to
IdleNotification
> can cause the finalization of the GC to happen much earlier.  I think that's
> unfortunate: I want the scheduler to be able to say "I found a few moments of
> time, use them if you can, but don't perturb your heuristics too much, based
on
> this".  Or perhaps "We are waiting for an important network packet, you can
have
> as many 1ms snippets as you want until the packet arrives, but we don't want
to
> delay the processing that is waiting for it when it arrives".
> 
> Perhaps this should only count idle notifications that are above a certain
> threshold, or perhaps it should add up the ms and use that as the limit,
rather
> than the invocation count.

As discussed offline, we just increment the delay counter when we were not able
to complete incremental marking. For now I guess it is good enough to just
guarantee that we complete eventually. Follow up cls can improve that heuristic.

https://codereview.chromium.org/629903003/diff/80001/test/unittests/heap/gc-i...
File test/unittests/heap/gc-idle-time-handler-unittest.cc (right):

https://codereview.chromium.org/629903003/diff/80001/test/unittests/heap/gc-i...
test/unittests/heap/gc-idle-time-handler-unittest.cc:179: }
On 2014/10/08 10:17:14, Erik Corry Chromium.org wrote:
> I think if you changed kMaxIdleMarkingDelayCounter from 3 to 1000 then this
test
> would still pass.  Can we test that the constant is working as intended?

I added an integration test that test the constant. Unfortunately, we cannot
have a nice unit test since it lives in heap and depends on global state.

[Hannes Payer (out of office), 2014-10-13 13:25:46]

https://codereview.chromium.org/629903003/diff/80001/src/heap/incremental-mar...
File src/heap/incremental-marking.h (right):

https://codereview.chromium.org/629903003/diff/80001/src/heap/incremental-mar...
src/heap/incremental-marking.h:92: static const size_t
kMaxIdleMarkingDelayCounter = 3;
On 2014/10/08 10:17:14, Erik Corry Chromium.org wrote:
> This is formulated in terms of a number of calls to IdleNotification. This
means
> a change on the embedder side that inserts extra 0.1ms calls to
IdleNotification
> can cause the finalization of the GC to happen much earlier.  I think that's
> unfortunate: I want the scheduler to be able to say "I found a few moments of
> time, use them if you can, but don't perturb your heuristics too much, based
on
> this".  Or perhaps "We are waiting for an important network packet, you can
have
> as many 1ms snippets as you want until the packet arrives, but we don't want
to
> delay the processing that is waiting for it when it arrives".
> 
> Perhaps this should only count idle notifications that are above a certain
> threshold, or perhaps it should add up the ms and use that as the limit,
rather
> than the invocation count.

As discussed offline, we just increment the delay counter when we were not able
to complete incremental marking. For now I guess it is good enough to just
guarantee that we complete eventually. Follow up cls can improve that heuristic.

https://codereview.chromium.org/629903003/diff/80001/test/unittests/heap/gc-i...
File test/unittests/heap/gc-idle-time-handler-unittest.cc (right):

https://codereview.chromium.org/629903003/diff/80001/test/unittests/heap/gc-i...
test/unittests/heap/gc-idle-time-handler-unittest.cc:179: }
On 2014/10/08 10:17:14, Erik Corry Chromium.org wrote:
> I think if you changed kMaxIdleMarkingDelayCounter from 3 to 1000 then this
test
> would still pass.  Can we test that the constant is working as intended?

I added an integration test that test the constant. Unfortunately, we cannot
have a nice unit test since it lives in heap and depends on global state.

[ulan, 2014-10-13 13:40:04]

lgtm

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2201: marking->Step(100 * MB,
IncrementalMarking::NO_GC_VIA_STACK_GUARD,
"100 * MB" looks fragile, but I guess it is sufficient. One possible solution
would be to make Step() return number of marked bytes and do "while (Step() >
0)";

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2220: CHECK_EQ(CcTest::heap()->gc_count(),
kShortIdleTimeInMs);
Did you mean CHECK_EQ(CcTest::heap()->gc_count(), 1) ?

[Hannes Payer (out of office), 2014-10-13 13:53:32]

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2201: marking->Step(100 * MB,
IncrementalMarking::NO_GC_VIA_STACK_GUARD,
On 2014/10/13 13:40:04, ulan wrote:
> "100 * MB" looks fragile, but I guess it is sufficient. One possible solution
> would be to make Step() return number of marked bytes and do "while (Step() >
> 0)";

Done.

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2220: CHECK_EQ(CcTest::heap()->gc_count(),
kShortIdleTimeInMs);
On 2014/10/13 13:40:04, ulan wrote:
> Did you mean CHECK_EQ(CcTest::heap()->gc_count(), 1) ?

lol, yes. Done.

[Hannes Payer (out of office), 2014-10-13 13:53:32]

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2201: marking->Step(100 * MB,
IncrementalMarking::NO_GC_VIA_STACK_GUARD,
On 2014/10/13 13:40:04, ulan wrote:
> "100 * MB" looks fragile, but I guess it is sufficient. One possible solution
> would be to make Step() return number of marked bytes and do "while (Step() >
> 0)";

Done.

https://codereview.chromium.org/629903003/diff/100001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2220: CHECK_EQ(CcTest::heap()->gc_count(),
kShortIdleTimeInMs);
On 2014/10/13 13:40:04, ulan wrote:
> Did you mean CHECK_EQ(CcTest::heap()->gc_count(), 1) ?

lol, yes. Done.

[Erik Corry, 2014-10-13 13:54:09]

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

[Erik Corry, 2014-10-13 13:54:10]

lgtm

[Hannes Payer (out of office), 2014-10-13 16:28:11]

Committed patchset #8 (id:260001) manually as 24567 (presubmit successful).