http://codereview.chromium.org/8519002/diff/1/src/heap.cc File src/heap.cc (right):
http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode518 src/heap.cc:518: !idle_notification_will_schedule_next_gc()) { I think if the heuristics say we need to start a GC then we should start a GC. Expecting the idle notification to schedule a GC is not reliable, because the idle notifications may not arrive. http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4465 src/heap.cc:4465: bool Heap::IdleNotification() { A comment here to say what the return value means would be helpful. http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4475 src/heap.cc:4475: // wait until the mutator creates more garbage. "Creates more garbage" is rather vague. If this means that a certain number of scavenges have taken place then we should write that. http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4480 src/heap.cc:4480: // The counters from IdleGlobalGC are reused, but have different meaning: This seems unnecessarily complicated. Counters are not expensive, lets give them just one meaning. http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4481 src/heap.cc:4481: // - number_idle_notifications_ counts the GC cycles. I do not like the phrase "GC cycles". If we use the following phrases it is clearer: Scavenge Full GC Incremental GC (always full) Nonincremental GC (always full) http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4497 src/heap.cc:4497: // Speed up if idle notifications are rare. Time-based things make debugging difficult, and in this case I cannot see that it makes sense. We should not try to autodetect the version of Chrome based on the interval between the idle notifications, which is what this looks like. http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4503 src/heap.cc:4503: !old_pointer_space()->IsSweepingComplete()) { It is a little dangerous to name individual spaces here. If we later have another space that is swept lazily then we will have to remember to update this place. http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4520 src/heap.cc:4520: incremental_marking()->Step(step_size); It seems that we do a step without checking that we are currently in the middle of incremental marking? http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4526 src/heap.cc:4526: isolate_->compilation_cache()->Clear(); We seem to be duplicating a lot of the logic of incremental GC here. Clearing specific caches, but leaving others alone, and calling CollectAllGarbage in order to finish the incremental GC. Can we factor that out into a common routine with the other place(s) where we discover that incremental GC is nearly done? http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4540 src/heap.cc:4540: return !WorthStartingGCWhenIdle() && If it is worth starting, should we not just start it, then the next line would take care of this situation? http://codereview.chromium.org/8519002/diff/1/src/heap.cc#newcode4542 src/heap.cc:4542: old_data_space()->IsSweepingComplete() && Listing the lazily swept pages is fragile. http://codereview.chromium.org/8519002/diff/1/src/heap.h File src/heap.h (right): http://codereview.chromium.org/8519002/diff/1/src/heap.h#newcode1804 src/heap.h:1804: static const int kMaxIdleCount = kIdlesBeforeMarkCompact + 1; The name kMaxIdleCount no longer makes much sense. Can't we get any more idle notifications than this, and why not? http://codereview.chromium.org/8519002/diff/1/src/heap.h#newcode1805 src/heap.h:1805: static const unsigned int kGCsBetweenCleanup = 4; I suggest kScavengesBeforeFullGC for this one. http://codereview.chromium.org/8519002/diff/1/src/incremental-marking.h File src/incremental-marking.h (right): http://codereview.chromium.org/8519002/diff/1/src/incremental-marking.h#newcode103 src/incremental-marking.h:103: static const intptr_t kStepFakeAllocatedBytes = kAllocatedThreshold * 3; This is rather ugly. Instead of lying to the Step function about how much memory has been allocated, we should create a step function that takes a number of bytes to scan, and then we should call that from the old Step function. http://codereview.chromium.org/8519002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
