Bug 84404 - We're collecting pathologically due to small allocations
Summary: We're collecting pathologically due to small allocations
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
Depends on:
Reported: 2012-04-19 16:39 PDT by Mark Hahnenberg
Modified: 2012-06-12 13:02 PDT (History)
2 users (show)

See Also:

Patch (33.31 KB, patch)
2012-04-19 16:53 PDT, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Geoffrey Garen 2012-04-19 16:57:05 PDT
Comment on attachment 138008 [details]

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.