Use smaller heap growing factor in idle notification to start incremental marking when there is idl…

Use smaller heap growing factor in idle notification to start incremental marking when there is idle time >16ms.

BUG=chromium:477323
LOG=y

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

[Hannes Payer (out of office), 2015-04-16 13:33:27]

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

[Erik Corry Chromium.org, 2015-04-16 14:16:44]

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

https://codereview.chromium.org/1090963002/diff/20001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.h:126: static const size_t
kMaxFrameRenderingIdleTime = 16;
It's 16 and two thirds.  I think you should go with 17 because rounding errors.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:5235: // may be close.
But it may not be close.

I'm not very happy with setting the limit to Min(halfway_ttm, x * factor +
stuff) and then later changing the limit by dividing by the factor and
multiplying by another factor.  It would be cleaner to have two limits,
calculated with the two factors and set them both in this routine.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1637: double min_old_generation_growing_factor_;
This never changes.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1643: double idle_old_generation_growing_factor_;
This never changes.

I think it keeps things simpler to have these as genuine constants.

[ulan, 2015-04-16 14:17:22]

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4593: limit =
Instead of storing current_old_generation_growing_factor_ and reversing the
limit, can we just compute and store the idle_limit at the same time as we
compute the old_generation_allocation_limit?

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:5236: current_old_generation_growing_factor_ = factor;
Since it is used for just for the idle limit computation, I would store the idle
limit directly here.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1228: inline bool NextGCIsLikelyToBeFull(intptr_t limit = 0) {
Always pass the limit (either the old_generation_allocation_limit_ or the
idle_limit) and get rid of the conditional below?

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1636: // The minimum factor used to grow the old generation.
Each additional state variable complicates GC scheduling. Let's minimize the
number of state variables.

One state variable is unavoidable: either current_old_generation_growing_factor_
or better idle_limit_.

Everything else should be a (local) constant I think.

[Hannes Payer (out of office), 2015-04-16 14:46:45]

Thanks for the comments, it's simpler now.

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

https://codereview.chromium.org/1090963002/diff/20001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.h:126: static const size_t
kMaxFrameRenderingIdleTime = 16;
On 2015/04/16 14:16:44, Erik Corry Chromium.org wrote:
> It's 16 and two thirds.  I think you should go with 17 because rounding
errors.

Done.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4593: limit =
On 2015/04/16 14:17:21, ulan wrote:
> Instead of storing current_old_generation_growing_factor_ and reversing the
> limit, can we just compute and store the idle_limit at the same time as we
> compute the old_generation_allocation_limit?

Done.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:5235: // may be close.
On 2015/04/16 14:16:44, Erik Corry Chromium.org wrote:
> But it may not be close.
> 
> I'm not very happy with setting the limit to Min(halfway_ttm, x * factor +
> stuff) and then later changing the limit by dividing by the factor and
> multiplying by another factor.  It would be cleaner to have two limits,
> calculated with the two factors and set them both in this routine.

Done.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.cc#newcod...
src/heap/heap.cc:5236: current_old_generation_growing_factor_ = factor;
On 2015/04/16 14:17:22, ulan wrote:
> Since it is used for just for the idle limit computation, I would store the
idle
> limit directly here.

Done.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h
File src/heap/heap.h (right):

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1228: inline bool NextGCIsLikelyToBeFull(intptr_t limit = 0) {
On 2015/04/16 14:17:22, ulan wrote:
> Always pass the limit (either the old_generation_allocation_limit_ or the
> idle_limit) and get rid of the conditional below?

Done.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1636: // The minimum factor used to grow the old generation.
On 2015/04/16 14:17:22, ulan wrote:
> Each additional state variable complicates GC scheduling. Let's minimize the
> number of state variables.
> 
> One state variable is unavoidable: either
current_old_generation_growing_factor_
> or better idle_limit_.
> 
> Everything else should be a (local) constant I think.
> 

Done.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1637: double min_old_generation_growing_factor_;
On 2015/04/16 14:16:44, Erik Corry Chromium.org wrote:
> This never changes.

Done.

https://codereview.chromium.org/1090963002/diff/20001/src/heap/heap.h#newcode...
src/heap/heap.h:1643: double idle_old_generation_growing_factor_;
On 2015/04/16 14:16:44, Erik Corry Chromium.org wrote:
> This never changes.
> 
> I think it keeps things simpler to have these as genuine constants.

Done.

[ulan, 2015-04-16 15:00:03]

lgtm

[rmcilroy, 2015-04-16 15:23:40]

On 2015/04/16 15:00:03, ulan wrote:
> lgtm

lgtm, thanks.

[Erik Corry, 2015-04-16 15:35:46]

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

[Erik Corry, 2015-04-16 15:35:46]

LGTM

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

https://codereview.chromium.org/1090963002/diff/20001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.h:126: static const size_t
kMaxFrameRenderingIdleTime = 16;
It's actually 16⅔.  Should we use 17 here?

[Hannes Payer (out of office), 2015-04-16 16:30:02]

The CQ bit was checked by hpayer@chromium.org

[commit-bot: I haz the power, 2015-04-16 16:30:11]

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

[commit-bot: I haz the power, 2015-04-16 16:31:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-16 16:31:56]

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