Summary: | GC timer should intelligently choose between EdenCollections and FullCollections | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Mark Hahnenberg <mhahnenberg> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | aestes, bunhere, commit-queue, gyuyoung.kim, rakuco, sergio, webkit-ews | ||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Bug Depends on: | 130488 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
Mark Hahnenberg
2014-02-05 10:58:03 PST
Created attachment 226599 [details]
straw man
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.
Created attachment 226600 [details]
straw man 2
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. Created attachment 227087 [details]
Patch
Created attachment 227107 [details]
Patch
Created attachment 227114 [details]
Patch
Created attachment 227117 [details]
Patch
Created attachment 227125 [details]
Patch
Created attachment 227128 [details]
Patch
Created attachment 227138 [details]
Patch
Created attachment 227142 [details]
Patch
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. Committed r165926: <http://trac.webkit.org/changeset/165926> Re-opened since this is blocked by bug 130488 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) Committed r165940: <http://trac.webkit.org/changeset/165940> |