[webkit-reviews] review granted: [Bug 128261] GC timer should intelligently choose between EdenCollections and FullCollections : [Attachment 226600] straw man 2

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 128261: GC timer should intelligently choose between EdenCollections and
FullCollections
https://bugs.webkit.org/show_bug.cgi?id=128261

Attachment 226600: straw man 2
https://bugs.webkit.org/attachment.cgi?id=226600&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226600&action=review


r=me

Please test PLT before landing this.

Plus a few fixups below.

> Source/JavaScriptCore/heap/GCActivityCallback.cpp:-97
>      if (heap->isPagedOut(startTime + pagingTimeOut)) {
> -	   heap->activityCallback()->cancel();

EdenCollection probably should not do the isPagedOut check.

I think isPagedOut touches the whole heap, so that's not great for Eden.

Also, EdenCollection is more likely to find free space than FullCollection, so
I think it should be less conservative about whether collection is "worth it".

> Source/JavaScriptCore/heap/GCActivityCallback.cpp:112
> +    heap->setShouldDoFullCollection(true);
>      heap->collect();

I think it would be a slightly better abstraction to pass a collection type to
collect(), rather than setting a data member. Perhaps the Heap implements
things internally in terms of a data member, but clients need not know that
detail.

> Source/JavaScriptCore/heap/Heap.cpp:281
> +    , m_fullActivityCallback(GCActivityCallback::createFullTimer(this))
> +    , m_edenActivityCallback(GCActivityCallback::createEdenTimer(this))

If GenGC is disabled, both pointers should point to the same callback, and it
should be a full callback. That way, we can still A/B test GenGC on vs off.

> Source/JavaScriptCore/heap/Heap.cpp:340
> -    if (m_activityCallback)
> -	   m_activityCallback->didAllocate(m_bytesAllocatedThisCycle +
m_bytesAbandonedThisCycle);
> +    if (m_fullActivityCallback)
> +	   m_fullActivityCallback->didAllocate(m_sizeAfterLastCollect +
m_bytesAbandonedThisCycle);

This is pretty aggro. The first abandoned object graph reports that it
abandoned the whole heap! I think you want this instead:

    m_sizeAfterLastCollect - m_sizeAfterLastFullCollect +
m_bytesAllocatedThisCycle + m_bytesAbandonedThisCycle

That way, we only report our growth since the last full collect. 

Also: You should rename m_bytesAbandonedThisCycle to
m_bytesAbandonedSinceLastFullCollect, and change it so that it only resets to 0
during a full collect. Our theory of abandoned object graphs is that they
should hasten full collections, so we should not reset the abandoned counter
during eden collections.

For example, if I navigate between three websites, and do three eden
collections, I still want to do a full collection soon.