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. http://codereview.chromium.org/3327021/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
