Bug 128261 – GC timer should intelligently choose between EdenCollections and FullCollections

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

Most of the GCs while browsing the web are due to the GC timer. Currently the GC timer always does FullCollections. To reduce the impact of the GC timer on the system we should add some logic to intelligently schedule both EdenCollections and FullCollections.

[Mark Hahnenberg]

Created attachment 226599 [details]
straw man

[Early Warning System Bot]

Attachment 226599 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Hahnenberg]

Created attachment 226600 [details]
straw man 2

[Geoffrey Garen]

Comment on attachment 226600 [details]
straw man 2

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.

[Mark Hahnenberg]

Created attachment 227087 [details]
Patch

[Mark Hahnenberg]

Created attachment 227107 [details]
Patch

[Mark Hahnenberg]

Created attachment 227114 [details]
Patch

[Mark Hahnenberg]

Created attachment 227117 [details]
Patch

[Mark Hahnenberg]

Created attachment 227125 [details]
Patch

[Mark Hahnenberg]

Created attachment 227128 [details]
Patch

[Mark Hahnenberg]

Created attachment 227138 [details]
Patch

[Mark Hahnenberg]

Created attachment 227142 [details]
Patch

[Geoffrey Garen]

Comment on attachment 227142 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227142&action=review

r=me

> Source/JavaScriptCore/heap/EdenGCActivityCallback.cpp:57
> +    return double(sizeBefore - sizeAfter) / double(sizeBefore);

static_cast<double> is more our style.

[Mark Hahnenberg]

Committed r165926: <http://trac.webkit.org/changeset/165926>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 130488

[Andy Estes]

This broke linking WebCore on iOS:

Undefined symbols for architecture x86_64:
  "__ZN3JSC22FullGCActivityCallback12doCollectionEv", referenced from:
      __ZTVN7WebCore29WebSafeFullGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC22FullGCActivityCallback11gcTimeSliceEm", referenced from:
      __ZTVN7WebCore29WebSafeFullGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC18GCActivityCallback11willCollectEv", referenced from:
      __ZTVN7WebCore29WebSafeEdenGCActivityCallbackE in JSDOMWindowBase.o
      __ZTVN7WebCore29WebSafeFullGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC22FullGCActivityCallback12lastGCLengthEv", referenced from:
      __ZTVN7WebCore29WebSafeFullGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC22FullGCActivityCallback9deathRateEv", referenced from:
      __ZTVN7WebCore29WebSafeFullGCActivityCallbackE in JSDOMWindowBase.o
  "_wkQueryDecoderAvailability", referenced from:
     -exported_symbol[s_list] command line option
  "__ZN3JSC18GCActivityCallback6doWorkEv", referenced from:
      __ZTVN7WebCore29WebSafeEdenGCActivityCallbackE in JSDOMWindowBase.o
      __ZTVN7WebCore29WebSafeFullGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC22EdenGCActivityCallback12doCollectionEv", referenced from:
      __ZTVN7WebCore29WebSafeEdenGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC18GCActivityCallback6cancelEv", referenced from:
      __ZTVN7WebCore29WebSafeEdenGCActivityCallbackE in JSDOMWindowBase.o
      __ZTVN7WebCore29WebSafeFullGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC22EdenGCActivityCallback9deathRateEv", referenced from:
      __ZTVN7WebCore29WebSafeEdenGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC18GCActivityCallback11didAllocateEm", referenced from:
      __ZTVN7WebCore29WebSafeEdenGCActivityCallbackE in JSDOMWindowBase.o
      __ZTVN7WebCore29WebSafeFullGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC22EdenGCActivityCallback12lastGCLengthEv", referenced from:
      __ZTVN7WebCore29WebSafeEdenGCActivityCallbackE in JSDOMWindowBase.o
  "__ZN3JSC22EdenGCActivityCallback11gcTimeSliceEm", referenced from:
      __ZTVN7WebCore29WebSafeEdenGCActivityCallbackE in JSDOMWindowBase.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[Mark Hahnenberg]

Committed r165940: <http://trac.webkit.org/changeset/165940>