Bug 85411

Summary: Opportunistic GC should give up if the Heap is paged out
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch fpizlo: review+

Description Mark Hahnenberg 2012-05-02 13:10:37 PDT
Opportunistic GC is punishing us severely in limited memory situations because its assumptions about how much time a collection will take are way out of whack when the Heap has been paged out by the OS. We should add a simple detection function to the Heap that detects if its is paged out. It will do this by iterating each block of both the MarkedSpace and CopiedSpace. If that operation takes longer than a fixed amount of time (e.g. 100ms), the function returns true. This function will only be run prior to an opportunistic collection (i.e. it will not run during our normal allocation-triggered collections).
Comment 1 Mark Hahnenberg 2012-05-02 14:02:39 PDT
Created attachment 139882 [details]
Patch
Comment 2 Geoffrey Garen 2012-05-02 16:06:34 PDT
Comment on attachment 139882 [details]
Patch

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

Looks good, but needs refinement.

Please file a bug about the incremental sweep issue you mentioned, and the "calling didAllocate" issue you mentioned in person.

> Source/JavaScriptCore/ChangeLog:14
> +        and CopiedSpace. If that operation takes longer than a fixed amount of time (e.g. 100ms), 
> +        the function returns true. This function will only be run prior to an opportunistic 
> +        collection (i.e. it will not run during our normal allocation-triggered collections).

Bug # or it didn't happen ;).

> Source/JavaScriptCore/heap/CopiedSpace.cpp:291
> +        if (itersSinceLastTimeCheck >= 16) {

16 should be a static const at the top of the file.

> Source/JavaScriptCore/heap/CopiedSpace.cpp:293
> +            if (currentTime > timeOut)

timeOut implies an amount of time, not a deadline in absolute time. How about using the word "deadline"?

> Source/JavaScriptCore/heap/MarkedAllocator.cpp:17
> +        if (itersSinceLastTimeCheck >= 16) {

Shared constant, please.

> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:68
> +    double startTime = WTF::monotonicallyIncreasingTime();
> +    if (heap->isPagedOut(startTime + pagingTimeOut)) {

This should only happen on systems that page, so !PLATFORM(IOS).

> Source/JavaScriptCore/runtime/GCActivityCallbackCF.cpp:69
> +        heap->activityCallback()->willCollect();

I think it would be better just to cancel the timer, rather than calling this API duplicitously.
Comment 3 Mark Hahnenberg 2012-05-02 16:44:00 PDT
Created attachment 139914 [details]
Patch
Comment 4 Filip Pizlo 2012-05-02 17:00:07 PDT
Comment on attachment 139914 [details]
Patch

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

R=me with one fix for that comment.

> Source/JavaScriptCore/heap/CopiedSpace.cpp:295
> +        if (itersSinceLastTimeCheck >= Heap::s_timeCheckResolution) {
> +            double currentTime = WTF::monotonicallyIncreasingTime();
> +            if (currentTime > deadline)
> +                return true;
> +        }

Shouldn't this be setting itersSinceLastTimeCheck to 0 if we don't return true?
Comment 5 Mark Hahnenberg 2012-05-02 17:13:14 PDT
Committed r115915: <http://trac.webkit.org/changeset/115915>
Comment 6 Mark Hahnenberg 2012-05-03 15:37:34 PDT
<rdar://problem/11294420>