Bug 85411 - Opportunistic GC should give up if the Heap is paged out
Summary: Opportunistic GC should give up if the Heap is paged out
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-02 13:10 PDT by Mark Hahnenberg
Modified: 2012-05-03 15:37 PDT (History)
2 users (show)

See Also:


Attachments
Patch (10.38 KB, patch)
2012-05-02 14:02 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (12.82 KB, patch)
2012-05-02 16:44 PDT, Mark Hahnenberg
fpizlo: 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-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>