Bug 84404

Summary: We're collecting pathologically due to small allocations
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch ggaren: review+

Description Mark Hahnenberg 2012-04-19 16:39:29 PDT
On google.com we're constantly running full collections due to the fact that after a collection, the first allocation will always hit the slow path because there are no free lists in any of the blocks. This causes the GC timer to set itself and fire very quickly, causing another full collection and the resetting of all the free lists. We should not be collecting so aggressively. 

A way to fix this is to change Heap so that instead of using a water mark to gauge whether or not a collection should be run, we track the number of bytes we've allocated since the last collection. When that hits a certain threshold, the Heap will recommend a collection be run. The Heap also notifies the GC activity timer of how many bytes have been allocated since the last collection, and the timer will ignore these notifications until the amount allocated exceeds a certain preset amount (currently 128K), after which it will schedule a timer to run a collection like it currently does. This prevents a sort of runaway timer that is rescheduled too eagerly.

This setup also puts the policy of future collection scheduling more squarely into the hands of the Heap and the GC activity timer, which will allow us to have more robust policies in the future.
Comment 1 Mark Hahnenberg 2012-04-19 16:53:27 PDT
Created attachment 138008 [details]
Patch
Comment 2 Geoffrey Garen 2012-04-19 16:57:05 PDT
Comment on attachment 138008 [details]
Patch

r=me, but please put your performance results in the ChangeLog.
Comment 3 Mark Hahnenberg 2012-04-19 17:05:50 PDT
Committed r114698: <http://trac.webkit.org/changeset/114698>
Comment 4 Yong Li 2012-06-11 14:26:38 PDT
I'm bisecting a sunspider regression. Now it appears to me this one makes a big impact. I'm testing on QNX armv7 with no DFG, no LLINT, no GGC, no GCActivityCallback implemented. Is it theoretically possible this commit can affect performance very much?
Comment 5 Yong Li 2012-06-12 13:02:19 PDT
It seems the m_bytesAllocatedLimit (previously m_highWaterMark) is no longer twice of the new size.

    size_t newSize = size();
    if (fullGC) {
        m_lastFullGCSize = newSize;
        m_bytesAllocatedLimit = max(newSize, m_minBytesPerCycle);

After I make it like this:
m_bytesAllocatedLimit = max(newSize * 2, m_minBytesPerCycle);

the regression disappears. I will do more testing to confirm this.