Make idle notification cleanup less aggressive. Do not clean up on...

Make idle notification cleanup less aggressive. Do not clean up on
idle notifications after the one that causes the mark-compact
collection unless four or more garbage collections (scavenges) have
occurred.

The embedder should stop sending idle notifications once V8 returns
true from the IdleNotification call. This change is being defensive so
it will not hurt as badly if embedders continue to send idle
notifications.


Committed: http://code.google.com/p/v8/source/detail?r=5981

[Kasper Lund, 2010-12-13 11:45:44]

http://codereview.chromium.org/5726005/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/5726005/diff/1/src/heap.cc#newcode3783
src/heap.cc:3783: if (number_idle_notifications > kIdlesBeforeMarkCompact)
return true;
Have you consider getting rid of this code and just falling into the
contexts_disposed_ case? Somehow, I would be more comfortable with still getting
GCs on idle if we're actively disposing contexts. Maybe the call to
UncommitFromSpace isn't a bad idea either.

Maybe something like:

  } else if (contexts_disposed_ > 0) {
    ...
  } else {
    finished = true;
  }

would be better.

[Mads Ager (chromium), 2010-12-13 11:58:06]

http://codereview.chromium.org/5726005/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/5726005/diff/1/src/heap.cc#newcode3783
src/heap.cc:3783: if (number_idle_notifications > kIdlesBeforeMarkCompact)
return true;
On 2010/12/13 11:45:44, Kasper Lund wrote:
> Have you consider getting rid of this code and just falling into the
> contexts_disposed_ case? Somehow, I would be more comfortable with still
getting
> GCs on idle if we're actively disposing contexts. Maybe the call to
> UncommitFromSpace isn't a bad idea either.
> 
> Maybe something like:
> 
>   } else if (contexts_disposed_ > 0) {
>     ...
>   } else {
>     finished = true;
>   }
> 
> would be better.

Good idea. I have moved this to an else-if part that sets finished to true so it
fits the current model better.

[antonm, 2010-12-13 12:06:20]

LGTM

http://codereview.chromium.org/5726005/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/5726005/diff/1/src/heap.cc#newcode3760
src/heap.cc:3760: static const int kMaxIdleCount = 9;
do we need it at all?  maybe use something like kIdlesBeforeMarkCompact (+1)
instead?

[Mads Ager (chromium), 2010-12-13 12:12:58]

http://codereview.chromium.org/5726005/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/5726005/diff/1/src/heap.cc#newcode3760
src/heap.cc:3760: static const int kMaxIdleCount = 9;
On 2010/12/13 12:06:20, antonm wrote:
> do we need it at all?  maybe use something like kIdlesBeforeMarkCompact (+1)
> instead?

I'm just being paranoid to avoid overflows. I'll change it to
kIdlesBeforeMarkCompact + 1.

Thanks.