Bug 224155 – [GPUP] Purge renderer resources after 2D canvas and media features become inactive

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: New Bugs

[Peng Liu]

[GPUP] Purge renderer resources after 2D canvas and media features become inactive

[Peng Liu]

Created attachment 425102 [details]
WIP patch

[Peng Liu]

Created attachment 425103 [details]
WIP patch

[Radar WebKit Bug Importer]

<rdar://problem/76205859>

[Jon Lee]

rdar://76166141

[Peng Liu]

Created attachment 425180 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 425180 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/ScopedRenderingResourcesRequestCocoa.mm:73
> +    [[NSNotificationCenter defaultCenter] postNotificationName:UIApplicationDidEnterBackgroundNotification object:nil userInfo:nil];

Rather than softlink UIKIt, you can use use the NSString* version of these: @"UIApplicationDidEnterBackgroundNotification" (please check the actual string).

> Source/WebKit/GPUProcess/ios/GPUProcessIOS.mm:54
> +    int token;
> +    notify_register_dispatch("org.WebKit.GPUProcessPurgeUnusedResources", &token, dispatch_get_main_queue(), ^(int) {
> +        [[NSNotificationCenter defaultCenter] postNotificationName:UIApplicationDidEnterBackgroundNotification object:nil userInfo:nil];
> +        }
> +    );
> +
> +    [[NSNotificationCenter defaultCenter] postNotificationName:UIApplicationDidFinishLaunchingNotification object:nil userInfo:nil];

Do we want this? Should we instead respond to "org.WebKit.lowMemory" which the tests already send?

[Peng Liu]

Comment on attachment 425180 [details]
Patch

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

>> Source/WebKit/GPUProcess/ios/GPUProcessIOS.mm:54
>> +    [[NSNotificationCenter defaultCenter] postNotificationName:UIApplicationDidFinishLaunchingNotification object:nil userInfo:nil];
> 
> Do we want this? Should we instead respond to "org.WebKit.lowMemory" which the tests already send?

I suppose you referred to the line of `notify_register_dispatch`? Yes, sounds a good idea to do it in response of "org.WebKit.lowMemory".

[Peng Liu]

Created attachment 425191 [details]
Patch

[Peng Liu]

Comment on attachment 425180 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/ScopedRenderingResourcesRequestCocoa.mm:73
>> +    [[NSNotificationCenter defaultCenter] postNotificationName:UIApplicationDidEnterBackgroundNotification object:nil userInfo:nil];
> 
> Rather than softlink UIKIt, you can use use the NSString* version of these: @"UIApplicationDidEnterBackgroundNotification" (please check the actual string).

Fixed.

[Kimmo Kinnunen]

Comment on attachment 425191 [details]
Patch

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

I think there's couple of checks that still need taken care of? Ping me offline unless you know the cases and have taken care of them.

> Source/WTF/wtf/cocoa/MemoryPressureHandlerCocoa.mm:127
> +        [[NSNotificationCenter defaultCenter] postNotificationName:@"UIApplicationDidEnterBackgroundNotification" object:nil userInfo:nil];

This causes wrong counts, e.g. the receivers think the app is in the background even though it might be in the foreground.
How/where do we undo the action done here?

> Source/WebKit/GPUProcess/graphics/ScopedRenderingResourcesRequest.h:76
> +        if (!s_requests)

I don't think this sort of thing is thread-safe?

> Source/WebKit/GPUProcess/graphics/ScopedRenderingResourcesRequestCocoa.mm:72
> +    [[NSNotificationCenter defaultCenter] postNotificationName:@"UIApplicationDidEnterBackgroundNotification" object:nil userInfo:nil];

I wonder if nsnotificationchenter posting is thread safe?

[Peng Liu]

Some Core Animation states need to be taken care of properly. Over to Kimmo. :-)

[Kimmo Kinnunen]

Created attachment 425375 [details]
Patch, not tested

[Kimmo Kinnunen]

Created attachment 425376 [details]
Patch, not tested

[Kimmo Kinnunen]

Created attachment 425380 [details]
Patch, not tested

[Darin Adler]

Do you want someone to review your untested patch? Normally we’d want to review it after it’s tested.

[Chris Dumez]

(In reply to Darin Adler from comment #15)
> Do you want someone to review your untested patch? Normally we’d want to
> review it after it’s tested.

I think the decision during the last GPUProcess sync-up was to A/B test this patch after I landed the GPUProcess idle exit change. My patch has landed now. Kimmo, do you need help scheduling the A/B test?