Limit rate of full garbage collections triggered by idle notification.

Limit rate of full garbage collections triggered by idle notification.

Idle notification can trigger a bunch of full garbage collections, especially with the larger idle times of 50ms. We have to limit the rate of these events, since there is no need to get rid of garbage on a high rate.

BUG=chromium:468554, chromium:455713
LOG=y

[Hannes Payer (out of office), 2015-03-20 12:35:19]

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

[jochen (gone - plz use gerrit), 2015-03-20 12:41:53]

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

https://codereview.chromium.org/1023153002/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.cc:160: last_mark_compact_time +
kMinMillisecondsInBetweenMarkCompact;
you're adding seconds + milliseconds here as far as I can tell

https://codereview.chromium.org/1023153002/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.cc:213: printf("start idle round, contexts
disposed\n");
debug code?

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

https://codereview.chromium.org/1023153002/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.h:141: static const double
kMinMillisecondsInBetweenMarkCompact;
MinTime...InMs would be more consistent with the other constants

https://codereview.chromium.org/1023153002/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.h:194: double last_mark_comact_time,
idle_time_in_ms is a size_t value. Can last_mark_com*p*act_time be a size_t as
well? And what's the time unit?

[Hannes Payer (out of office), 2015-03-20 14:20:39]

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

https://codereview.chromium.org/1023153002/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.cc:160: last_mark_compact_time +
kMinMillisecondsInBetweenMarkCompact;
On 2015/03/20 12:41:52, jochen (slow) wrote:
> you're adding seconds + milliseconds here as far as I can tell

yeah, found it when I run the experiment. already removed.

https://codereview.chromium.org/1023153002/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.cc:213: printf("start idle round, contexts
disposed\n");
On 2015/03/20 12:41:52, jochen (slow) wrote:
> debug code?

Done.

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

https://codereview.chromium.org/1023153002/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.h:141: static const double
kMinMillisecondsInBetweenMarkCompact;
On 2015/03/20 12:41:52, jochen (slow) wrote:
> MinTime...InMs would be more consistent with the other constants

Done.

https://codereview.chromium.org/1023153002/diff/1/src/heap/gc-idle-time-handl...
src/heap/gc-idle-time-handler.h:194: double last_mark_comact_time,
On 2015/03/20 12:41:52, jochen (slow) wrote:
> idle_time_in_ms is a size_t value. Can last_mark_com*p*act_time be a size_t as
> well? And what's the time unit?

Done.

https://codereview.chromium.org/1023153002/diff/80001/test/cctest/test-api.cc
File test/cctest/test-api.cc (left):

https://codereview.chromium.org/1023153002/diff/80001/test/cctest/test-api.cc...
test/cctest/test-api.cc:15745: // Test that idle notification can be handled and
eventually collects garbage.
These test (both identical) don't provide any value anymore. From the API, just
idle times of 1000ms are interesting. The rest is handled in the idle handler
unit tests.

[jochen (gone - plz use gerrit), 2015-03-20 14:52:10]

lgtm

just to clarify, if the system is fully idle, we'll attempt a MC every second,
right?

[rmcilroy, 2015-03-20 14:56:08]

looks good overall - couple of questions though.

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

https://codereview.chromium.org/1023153002/diff/80001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:162: EstimateMarkCompactTime(size_of_objects,
this is looking pretty complex condition now - could you pull each of these out
into seperate well-named variables like mark_compact_rate_high

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

https://codereview.chromium.org/1023153002/diff/80001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.h:146: static const size_t
kMinTimeInBetweenMarkCompactsInMs = 1000;
Is 1000ms a guesstimation or is there any reason to think that pages shouldn't
really need full GCs within 1s?

https://codereview.chromium.org/1023153002/diff/80001/src/heap/heap.cc
File src/heap/heap.cc (right):

https://codereview.chromium.org/1023153002/diff/80001/src/heap/heap.cc#newcod...
src/heap/heap.cc:4629: heap_state.current_time =
static_cast<size_t>(current_time);
I'm slightly hesitent at setting current_time as a field in heap_state, because
it might lead it to be used at a much later point in time (when it doesn't
represent the actual current time). I realize making this a separate variable
won't change too much but might make this more obvious, WDYT?

[Erik Corry, 2015-03-20 15:21:27]

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

[Erik Corry, 2015-03-20 15:21:28]

In terms of testability it's not great to add more time-based decision making. 
It would be good to get to a place where the test harness could call the idle
API and there was no other time based decision making.

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

https://codereview.chromium.org/1023153002/diff/80001/src/heap/gc-idle-time-h...
src/heap/gc-idle-time-handler.cc:240: if
(heap_state.incremental_marking_stopped) {
It feels like we are piling more and more heuristics on.  Does anyone understand
the current system?

It looks like the bit above ("IsMarkCompactIdleRoundFinished") will run 7 GCs in
the idle time, and then not start another until we have allocated enough garbage
or there has been a context disposal.  That would seem to be plenty of GCs.  Why
do we need more than that?

[Erik Corry, 2015-03-20 15:45:45]

I think we'll take another look at it Monday.

One idea is to add up the unutilized idle times and use those to measure how
much time has gone.  That way any test that controls the idle notifications is
still deterministic on varying devices and system loads.

I'd like to get rid of the idea of idle rounds.  Doing 7 GCs is too much from a
battery point of view.  Really the only reason to do more than one is to clear
out the data that ages slowly, but that is totally artificial, we can
lightening-age it if we want.

The other reason to do multiple GCs is if we wanted to compact more memory, but
had to limit the amount of compaction due to pause time worries.

So suggestion:
* If idle GCs are not blocked, we do idle GCs until there is no more compaction
work to be done.  Often this means just one idle GC.
* We now ignore idle notifications: the only thing we do is count up the
milliseconds we were given, but decided not to use.  This simulates GC rounds
2-6, but uses no power.
* When we have counted up enough idle notifications, we do a fast-ageing GC that
deletes as much as possible as if everything had aged.  This will have the same
effect as the 5 GCs we skipped (esp. when the phantom handles are used
throughout the API)
* After that we block idle GCs until a certain amount of memory has been
allocated (30% growth of old space, or old space gets 50% closer to the limit)
or until a context is disposed.  We back off the non-idle GC triggers so they
trigger later than this.

Related: https://codereview.chromium.org/677043002/

[Hannes Payer (out of office), 2015-03-20 20:02:45]

I agree, this is getting too complicated. We should have something simple
(modulo the unfortunate hack).

The system should serve two purposes:
1) Perform garbage collection during idle time when we are close to a full new
or old generation.
2) Reduce memory footprint.

1) is easy and very natural.
2) is right now really arbitrary and leads to the mess we are having. Right now
we assume that we are always in memory reduction situation which is wrong. We
should just be in this mode when it is really needed. Let's find a good signal
for that.