Move idle notification handling to GCIdleTimeHandler.

Move idle notification handling to GCIdleTimeHandler.

BUG=
R=hpayer@chromium.org

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

[ulan, 2014-08-20 13:41:49]

PTAL

[Hannes Payer (out of office), 2014-08-20 18:24:11]

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

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.cc:61: intptr_t speed =
gc_tracer->MaxMarkSweepSpeedInBytesPerMillisecond();
Why are we taking the maximum?

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

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:21: struct GCIdleTimeAction {
According to the style guide, this should be a class.

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:74: void NotifyIdleFullGC() {
In the gc tracer we refer to the event as mark compact event. Let's make sure we
use the same terminology here.
NotifyIdleFullGC -> NotifyIdleMarkCompact

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:100: static const int kMaxMarkSweepsInIdleRound
= 7;
kMaxMarkSweepsInIdleRound -> kMaxMarkCompactsInIdleRound

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:102: int mark_sweeps_since_idle_round_started_;
mark_sweeps_since_idle_round_started_ -> mark_compacts_since_idle_round_started_

https://codereview.chromium.org/492763002/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/492763002/diff/40001/src/heap/heap.cc#newcode...
src/heap/heap.cc:4300: if (incremental_marking()->IsStopped()) {
I think this check should be in the Compute function.

https://codereview.chromium.org/492763002/diff/40001/src/heap/heap.cc#newcode...
src/heap/heap.cc:4325: mark_compact_collector()->sweeping_in_progress()) {
Should we do a SWEEP action., which is selected when the "hint" is large enough
and sweeping is in progress. WDYT?

[ulan, 2014-08-21 08:45:19]

Thanks for comments. PTAL.

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

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.cc:61: intptr_t speed =
gc_tracer->MaxMarkSweepSpeedInBytesPerMillisecond();
On 2014/08/20 18:24:11, Hannes Payer wrote:
> Why are we taking the maximum?

Good catch, it should be either minimum or average. Made average to be
consistent with other estimates.

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

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:21: struct GCIdleTimeAction {
On 2014/08/20 18:24:11, Hannes Payer wrote:
> According to the style guide, this should be a class.

Done.

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:74: void NotifyIdleFullGC() {
On 2014/08/20 18:24:11, Hannes Payer wrote:
> In the gc tracer we refer to the event as mark compact event. Let's make sure
we
> use the same terminology here.
> NotifyIdleFullGC -> NotifyIdleMarkCompact

Done.

https://codereview.chromium.org/492763002/diff/40001/src/heap/gc-idle-time-ha...
src/heap/gc-idle-time-handler.h:100: static const int kMaxMarkSweepsInIdleRound
= 7;
On 2014/08/20 18:24:11, Hannes Payer wrote:
> kMaxMarkSweepsInIdleRound -> kMaxMarkCompactsInIdleRound

Done.

https://codereview.chromium.org/492763002/diff/40001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/492763002/diff/40001/src/heap/heap.cc#newcode...
src/heap/heap.cc:4300: if (incremental_marking()->IsStopped()) {
On 2014/08/20 18:24:11, Hannes Payer wrote:
> I think this check should be in the Compute function.

Acknowledged.

https://codereview.chromium.org/492763002/diff/40001/src/heap/heap.cc#newcode...
src/heap/heap.cc:4325: mark_compact_collector()->sweeping_in_progress()) {
On 2014/08/20 18:24:11, Hannes Payer wrote:
> Should we do a SWEEP action., which is selected when the "hint" is large
enough
> and sweeping is in progress. WDYT?

Agreed, but let's do this in a separate CL to reduce risk of reverting CL.

[Hannes Payer (out of office), 2014-08-21 09:47:53]

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

https://codereview.chromium.org/492763002/diff/100001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:38: intptr_t size_of_objects, intptr_t
mark_sweep_speed_in_bytes_per_ms) {
Can we make the parameters size_t, c.f. EstimateMarkingStepSize.

https://codereview.chromium.org/492763002/diff/100001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:40: const int kMaxTimeInMs = 1000000;
I think these constant should be in GCIdleTImeHandler, otherwise it will be hard
to test this function. Btw. can we have unit tests, similar to the
EstimateMarkingStepSize tests.

https://codereview.chromium.org/492763002/diff/100001/src/heap/gc-tracer.cc
File src/heap/gc-tracer.cc (right):

https://codereview.chromium.org/492763002/diff/100001/src/heap/gc-tracer.cc#n...
src/heap/gc-tracer.cc:423: intptr_t
GCTracer::MarkCompactSpeedInBytesPerMillisecond() const {
The GCTracer functions should also operate on size_t. We should fix that in a
separate cl.

https://codereview.chromium.org/492763002/diff/100001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/492763002/diff/100001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4323: const int kMinHintForFullGC = 100;
Please add a todo here for sweeping events.

[ulan, 2014-08-21 11:44:01]

Added tests and addressed comments.

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

https://codereview.chromium.org/492763002/diff/100001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:38: intptr_t size_of_objects, intptr_t
mark_sweep_speed_in_bytes_per_ms) {
On 2014/08/21 09:47:53, Hannes Payer wrote:
> Can we make the parameters size_t, c.f. EstimateMarkingStepSize.

Done.

https://codereview.chromium.org/492763002/diff/100001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:40: const int kMaxTimeInMs = 1000000;
On 2014/08/21 09:47:53, Hannes Payer wrote:
> I think these constant should be in GCIdleTImeHandler, otherwise it will be
hard
> to test this function. Btw. can we have unit tests, similar to the
> EstimateMarkingStepSize tests.

Done.

https://codereview.chromium.org/492763002/diff/100001/src/heap/gc-tracer.cc
File src/heap/gc-tracer.cc (right):

https://codereview.chromium.org/492763002/diff/100001/src/heap/gc-tracer.cc#n...
src/heap/gc-tracer.cc:423: intptr_t
GCTracer::MarkCompactSpeedInBytesPerMillisecond() const {
On 2014/08/21 09:47:53, Hannes Payer wrote:
> The GCTracer functions should also operate on size_t. We should fix that in a
> separate cl.

Acknowledged.

https://codereview.chromium.org/492763002/diff/100001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/492763002/diff/100001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4323: const int kMinHintForFullGC = 100;
On 2014/08/21 09:47:53, Hannes Payer wrote:
> Please add a todo here for sweeping events.

Done.

[Hannes Payer (out of office), 2014-08-21 14:01:21]

lgtm

[ulan, 2014-08-21 14:42:31]

Committed patchset #9 manually as 23282 (presubmit successful).