Bug 224556 – Exit the GPUProcess when under memory pressure if it is not being used

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Exit the GPUProcess when under memory pressure if it is not being used. This will help us save memory, especially until we are able to enable "DOM Rendering in GPUProcess".

[Chris Dumez]

Created attachment 425995 [details]
Patch

[Chris Dumez]

I am currently perf-testing this patch on PLT5 and PLUM3.

[Geoffrey Garen]

EWS seems angry:

    TestWebKitAPI.GPUProcess.ExitsUnderMemoryPressureCanvasCase
        Child process terminated with signal 6: Abort trap
    TestWebKitAPI.GPUProcess.ExitsUnderMemoryPressureVideoCase
        Failed to find media engine.
        Failed to find media engine.
        Child process terminated with signal 6: Abort trap
    TestWebKitAPI.GPUProcess.ExitsUnderMemoryPressureWebAudioCase
        Child process terminated with signal 6: Abort trap

[Chris Dumez]

(In reply to Geoffrey Garen from comment #3)
> EWS seems angry:
> 
>     TestWebKitAPI.GPUProcess.ExitsUnderMemoryPressureCanvasCase
>         Child process terminated with signal 6: Abort trap
>     TestWebKitAPI.GPUProcess.ExitsUnderMemoryPressureVideoCase
>         Failed to find media engine.
>         Failed to find media engine.
>         Child process terminated with signal 6: Abort trap
>     TestWebKitAPI.GPUProcess.ExitsUnderMemoryPressureWebAudioCase
>         Child process terminated with signal 6: Abort trap

Yes, investigating.

[Ryosuke Niwa]

Comment on attachment 425995 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUProcess.cpp:133
> +    }
> +    return true;

Please rate limit this with time too although it could be done in a separate patch.
We probably shouldn't killing GPU process no more than once every 5-10s.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 425995 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425995&action=review
> 
> > Source/WebKit/GPUProcess/GPUProcess.cpp:133
> > +    }
> > +    return true;
> 
> Please rate limit this with time too although it could be done in a separate
> patch.
> We probably shouldn't killing GPU process no more than once every 5-10s.

Yes, I understand the concern. I am not doing this yet because it is going to be tricky to do this right and avoid making the tests flaky or causing the process not to exit in PLUM3. I am still pondering whether to do this in this patch or in a follow-up. I definitely want to do initial perf testing without this though.

[Chris Dumez]

Created attachment 426031 [details]
Patch

[Chris Dumez]

Created attachment 426033 [details]
Patch

[Chris Dumez]

Created attachment 426043 [details]
Patch

[Chris Dumez]

Created attachment 426153 [details]
Patch

[Chris Dumez]

The perf results look promising (Nice PLUM3 progression, no PLT5 regression). However, I am getting flaky GPUProcess crashes on ios-EWS (with no crash trace :/). I will investigate locally.

[Chris Dumez]

Looks like the iOS EWS crashes are not really true. It is true to this code in WKTR:
void TestController::gpuProcessDidCrash(WKProcessID processID)
{
    fprintf(stderr, "#CRASHED - GPUProcess (pid %ld)\n", static_cast<long>(processID));
    if (m_shouldExitWhenWebProcessCrashes)
        exit(1);
}


Every time the GPUProcess exits on memory pressure while running the tests, WKTR reports it as a crash even though it is a clean exit.

[Chris Dumez]

A/B Testing results:

PLUM3-iPhone - Arithmetic mean
Comparision by Mean	5.26% better (significant with 98% probability)
Comparision by Individual	5.26% better (significant with 98% probability)

PLUM3-iPad - Arithmetic mean
Comparision by Mean	2.55% better (significant with 98% probability)
Comparision by Individual	2.55% better (significant with 98% probability)

PLT5-iPad - null
Comparision by Mean	0.63% worse (insignificant)
Comparision by Individual	0.63% worse (insignificant)

PLT5-iPhone - null
Comparision by Mean	0.03% better (insignificant)
Comparision by Individual	0.03% better (insignificant)

Looks good!

I know how to address the fake crash reports on ios-EWS (We need to tell the gpuProcessCrashed client delegate only in the case of a true crash, not a clean idle exit). I will try and finish the patch tomorrow.

[Chris Dumez]

Created attachment 426230 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #14)
> Created attachment 426230 [details]
> Patch

Should take care of those iOS-EWS failures.

[Darin Adler]

Comment on attachment 426230 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:293
> +#if HAVE(AVASSETWRITERDELEGATE)

Would you be willing to consider just writing this out as:

    #if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM) && HAVE(AVASSETWRITERDELEGATE)

instead of nesting it?

[Chris Dumez]

(In reply to Darin Adler from comment #16)
> Comment on attachment 426230 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426230&action=review
> 
> > Source/WebKit/GPUProcess/GPUConnectionToWebProcess.cpp:293
> > +#if HAVE(AVASSETWRITERDELEGATE)
> 
> Would you be willing to consider just writing this out as:
> 
>     #if PLATFORM(COCOA) && ENABLE(MEDIA_STREAM) &&
> HAVE(AVASSETWRITERDELEGATE)
> 
> instead of nesting it?

Sure. I was just following the pattern in the header. I'll update.

[Chris Dumez]

Created attachment 426243 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/76768542>

[EWS]

Committed r276148 (236641@main): <https://commits.webkit.org/236641@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 426243 [details].

[Chris Dumez]

There is still code that (re-)launches the GPUProcess too eagerly. I will follow-up and fix them via related bugs.