Attempt to collect more garbage before panicking with out of memory.

Attempt to collect more garbage before panicking with out of memory.

Currently weak handles retain an object for another GC round (oftem times,
major GC round.)  Instrumenting Chromium shows that navigation leaves
many global objects which are only collected in next go.  Let's
attempt to collect more garbage when approacing OOM condition.

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

[antonm, 2010-09-13 14:14:44]

Slava,

may you have a look?

[Vyacheslav Egorov (Chromium), 2010-09-13 16:07:10]

Nice idea Anton. 

LGTM.

http://codereview.chromium.org/3327021/diff/1/3
File src/global-handles.h (right):

http://codereview.chromium.org/3327021/diff/1/3#newcode99
src/global-handles.h:99: static bool PostGarbageCollectionProcessing();
Document a return value here.

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

http://codereview.chromium.org/3327021/diff/1/5#newcode701
src/heap.cc:701: // Note: as weak callbacks can execute arbitrary code, we
cannot
Please write a line about purpose of the loop below. Otherwise it's a bit
confusing.

http://codereview.chromium.org/3327021/diff/1/5#newcode705
src/heap.cc:705: int attempts = 7;
Magic unnamed constants in the code. Consider giving name to 7 and moving it
somewhere (e.g. into Heap class)

http://codereview.chromium.org/3327021/diff/1/5#newcode722
src/heap.cc:722: if (collector == MARK_COMPACTOR) {
Consider updating old gen limits inside loop as handles processing can trigger
allocation and garbage collection.

http://codereview.chromium.org/3327021/diff/1/6
File src/heap.h (right):

http://codereview.chromium.org/3327021/diff/1/6#newcode693
src/heap.h:693: bool recollect_weakly_reachable = false);
Consider using enumeration instead of boolean to make callsites more readable.

[antonm, 2010-09-13 18:26:06]

Thanks a lot for comments, Slava.  May you have second look?

http://codereview.chromium.org/3327021/diff/1/3
File src/global-handles.h (right):

http://codereview.chromium.org/3327021/diff/1/3#newcode99
src/global-handles.h:99: static bool PostGarbageCollectionProcessing();
On 2010/09/13 16:07:10, Vyacheslav Egorov wrote:
> Document a return value here.

Done.

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

http://codereview.chromium.org/3327021/diff/1/5#newcode701
src/heap.cc:701: // Note: as weak callbacks can execute arbitrary code, we
cannot
On 2010/09/13 16:07:10, Vyacheslav Egorov wrote:
> Please write a line about purpose of the loop below. Otherwise it's a bit
> confusing.

Done.  I would appreciate the second look.

http://codereview.chromium.org/3327021/diff/1/5#newcode705
src/heap.cc:705: int attempts = 7;
On 2010/09/13 16:07:10, Vyacheslav Egorov wrote:
> Magic unnamed constants in the code. Consider giving name to 7 and moving it
> somewhere (e.g. into Heap class)

Almost done: I left the constant in the method itself---I don't think we should
extend the scope of this constant right now.

http://codereview.chromium.org/3327021/diff/1/5#newcode722
src/heap.cc:722: if (collector == MARK_COMPACTOR) {
On 2010/09/13 16:07:10, Vyacheslav Egorov wrote:
> Consider updating old gen limits inside loop as handles processing can trigger
> allocation and garbage collection.

Good catch, thanks.  Done.

http://codereview.chromium.org/3327021/diff/1/6
File src/heap.h (right):

http://codereview.chromium.org/3327021/diff/1/6#newcode693
src/heap.h:693: bool recollect_weakly_reachable = false);
On 2010/09/13 16:07:10, Vyacheslav Egorov wrote:
> Consider using enumeration instead of boolean to make callsites more readable.

Done.  I don't quite like the term "aggression", if you have any other ideas,
they are most appreciated.

[Vyacheslav Egorov (Chromium), 2010-09-14 08:31:45]

Still LGTM except for the piece with retry &= ...

http://codereview.chromium.org/3327021/diff/7001/8004
File src/heap.cc (right):

http://codereview.chromium.org/3327021/diff/7001/8004#newcode700
src/heap.cc:700: // handle callback, we reran major GC to release objects which
become
next go => next major GC
reran => rerun

http://codereview.chromium.org/3327021/diff/7001/8004#newcode714
src/heap.cc:714: retry &= GlobalHandles::PostGarbageCollectionProcessing();
I see no reason to make |retry| semantics this complicated. 

I would rewrite the whole do { } while (); into something like:

const int cycles = (aggression == HIGH) ? kNumberOfCyclesInAggressiveCollection
: 1;
for (int cycle = 0; cycle < cycles; cycle++) {
  MarkCompact(tracer);
  UpdateOldSpaceLimits();
  { DisableAssertNoAllocation allow_allocation;
    GCTracer::Scope scope(tracer, GCTracer::Scope::EXTERNAL);
    if (!GlobalHandles::PostGarbageCollectionProcessing()) {
      break;
    }
  }
}

But this is just my _thought_ not a call for immediate action. Feel free to chat
with me about this.

http://codereview.chromium.org/3327021/diff/7001/8004#newcode732
src/heap.cc:732: UpdateOldSpaceLimits();
It is not needed here. 

Also from correctness standpoint you should move the whole survival trend
updating after MarkCompact call. Especially 

old_gen_exhausted_ = false;

thingy.

http://codereview.chromium.org/3327021/diff/7001/8005
File src/heap.h (right):

http://codereview.chromium.org/3327021/diff/7001/8005#newcode689
src/heap.h:689: enum Aggression { NORMAL, HIGH };
I agree that aggression might not be the best choice. 

I can't think about something better than:

enum WeaklyReachableReclamationMode { TRY_RECLAIM_WEAKLY_REACHABLE,
LEAVE_WEAKLY_REACHABLE_FOR_NEXT_GC };

But this is awfully long.

[antonm, 2010-09-14 16:48:04]

hopefully we're getting closer

http://codereview.chromium.org/3327021/diff/7001/8004
File src/heap.cc (right):

http://codereview.chromium.org/3327021/diff/7001/8004#newcode700
src/heap.cc:700: // handle callback, we reran major GC to release objects which
become
On 2010/09/14 08:31:45, Vyacheslav Egorov wrote:
> next go => next major GC
> reran => rerun
> 

Done.

http://codereview.chromium.org/3327021/diff/7001/8004#newcode714
src/heap.cc:714: retry &= GlobalHandles::PostGarbageCollectionProcessing();
On 2010/09/14 08:31:45, Vyacheslav Egorov wrote:
> I see no reason to make |retry| semantics this complicated. 
> 
> I would rewrite the whole do { } while (); into something like:
> 
> const int cycles = (aggression == HIGH) ?
kNumberOfCyclesInAggressiveCollection
> : 1;
> for (int cycle = 0; cycle < cycles; cycle++) {
>   MarkCompact(tracer);
>   UpdateOldSpaceLimits();
>   { DisableAssertNoAllocation allow_allocation;
>     GCTracer::Scope scope(tracer, GCTracer::Scope::EXTERNAL);
>     if (!GlobalHandles::PostGarbageCollectionProcessing()) {
>       break;
>     }
>   }
> }
> 
> But this is just my _thought_ not a call for immediate action. Feel free to
chat
> with me about this.

Done.

http://codereview.chromium.org/3327021/diff/7001/8004#newcode732
src/heap.cc:732: UpdateOldSpaceLimits();
On 2010/09/14 08:31:45, Vyacheslav Egorov wrote:
> It is not needed here. 
> 
> Also from correctness standpoint you should move the whole survival trend
> updating after MarkCompact call. Especially 
> 
> old_gen_exhausted_ = false;
> 
> thingy. 

Thanks a lot for pointing that out and for offline discussion.

[Vyacheslav Egorov (Chromium), 2010-09-14 17:07:03]

LGTM!

[Vyacheslav Egorov (Chromium), 2010-09-14 17:08:42]

small remark

http://codereview.chromium.org/3327021/diff/14001/15004
File src/heap.cc (right):

http://codereview.chromium.org/3327021/diff/14001/15004#newcode738
src/heap.cc:738: { DisableAssertNoAllocation allow_allocation;
Empty line before this one might help readability.