Bug 223885 – RemoteRenderingBackend should clean up CG resources after last Canvas is destroyed in Gpu process

• Component: Canvas
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

RemoteRenderingBackend should clean up CG resources after last Canvas is destroyed in Gpu process

[Kimmo Kinnunen]

<rdar://problem/75637356>

[Kimmo Kinnunen]

Created attachment 424549 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 424549 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsUtilitiesCG.h:32
> +WEBCORE_EXPORT void releaseUnusedCoreGraphicsResources();

These are not "core graphics" resources per se. I would call them "rendering resources" or something.

> Source/WebCore/platform/graphics/cg/GraphicsUtilitiesCG.mm:47
> +    auto devices = adoptNS(MTLCopyAllDevices());

Do we know that this isn't the first time MTLCopyAllDevices() is called, which would possibly trigger allocation of more resources?

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:70
> +static constexpr Seconds releaseAllResourcesIfUnusedTimeout = 0.2_s;

That seems really short.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:111
> +        --remoteRenderingBackendCount;

You should ASSERT(remoteRenderingBackendCount > 0) before this.

[Radar WebKit Bug Importer]

<rdar://problem/75963350>

[Dean Jackson]

Comment on attachment 424549 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/GraphicsUtilitiesCG.mm:47
>> +    auto devices = adoptNS(MTLCopyAllDevices());
> 
> Do we know that this isn't the first time MTLCopyAllDevices() is called, which would possibly trigger allocation of more resources?

I think we call this on process start in order to pre-warm.

>> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:70
>> +static constexpr Seconds releaseAllResourcesIfUnusedTimeout = 0.2_s;
> 
> That seems really short.

Yeah. Isn't there a chance that this could happen on a page navigation, if the incoming page takes a while to load and initialize WebGL? Maybe 10s or so?

[Kimmo Kinnunen]

(In reply to Simon Fraser (smfr) from comment #3)
> > Source/WebCore/platform/graphics/cg/GraphicsUtilitiesCG.mm:47
> > +    auto devices = adoptNS(MTLCopyAllDevices());
> 
> Do we know that this isn't the first time MTLCopyAllDevices() is called,
> which would possibly trigger allocation of more resources?

We only release the resources if we have had a canvas. Currently we don't track if the canvas drew, but it's probably fairly accurate estimate of whether the function has already been called (by CG)..


> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:70
> > +static constexpr Seconds releaseAllResourcesIfUnusedTimeout = 0.2_s;
> 
> That seems really short.

It's what WebGL uses. How long do our page switches take?

[Simon Fraser (smfr)]

Comment on attachment 424549 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/GraphicsUtilitiesCG.mm:47
>>> +    auto devices = adoptNS(MTLCopyAllDevices());
>> 
>> Do we know that this isn't the first time MTLCopyAllDevices() is called, which would possibly trigger allocation of more resources?
> 
> I think we call this on process start in order to pre-warm.

Only in the web process, I think?

[Simon Fraser (smfr)]

Comment on attachment 424549 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:114
> +        if (!remoteRenderingBackendCount)
> +            RunLoop::main().dispatchAfter(releaseAllResourcesIfUnusedTimeout, releaseAllResourcesIfUnused);
> +    });

Maybe use HysteresisActivity

[Kimmo Kinnunen]

Created attachment 424558 [details]
Patch

[Kimmo Kinnunen]

>Maybe use HysteresisActivity

What benefits would this bring?

Would cause all sorts of threading issues to be worked around.
The implementation of delayed dispatch seems subset of HysterisisActivity, e.g. it has strictly smaller footprint.

[Kimmo Kinnunen]

Created attachment 424887 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 424887 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/ScopedRenderingResourcesRequest.h:60
> +    static void releaseRenderingResources()

I would avoid use of the term "release" here because it sounds like this function itself frees the resources. I would use incrementRequestCount and decrementRequestCount which each modify s_requests, for symmetry.

> Source/WebKit/GPUProcess/graphics/ScopedRenderingResourcesRequestCocoa.mm:70
> +}

This function will also send the background notification soon.

> Source/WebKit/GPUProcess/graphics/ScopedWebGLRenderingResourcesRequest.cpp:43
> +void ScopedWebGLRenderingResourcesRequest::scheduleFreeWebGLRenderingResources()
> +{
> +
> +}
> +
> +void ScopedWebGLRenderingResourcesRequest::freeWebGLRenderingResources()
> +{
> +
> +}

Remove the empty lines.

> Source/WebKit/GPUProcess/graphics/ScopedWebGLRenderingResourcesRequest.h:34
> +class ScopedWebGLRenderingResourcesRequest {

It's crazy to repeat all this code again. There should be a base class that does the atomic counting stuff (I'm surprised we don't have one already).

> Source/WebKit/GPUProcess/graphics/ScopedWebGLRenderingResourcesRequest.h:80
> +    ScopedRenderingResourcesRequest m_renderingResourcesRequest;

It's slightly odd that this class holds a ScopedRenderingResourcesRequest but then duplicates its functionality. Maybe the WebGL code should just hold a ScopedRenderingResourcesRequest too?

[Kimmo Kinnunen]

Created attachment 424918 [details]
Patch

[Kimmo Kinnunen]

(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 424887 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424887&action=review
> 
> > Source/WebKit/GPUProcess/graphics/ScopedRenderingResourcesRequest.h:60
> > +    static void releaseRenderingResources()
> 
> I would avoid use of the term "release" here because it sounds like this
> function itself frees the resources. I would use incrementRequestCount and
> decrementRequestCount which each modify s_requests, for symmetry.

How about relating it with other scoped apparatus like unique_ptr and their (public) reset()?
I'm hesitant to add incrementRequestCount() { ++s_requests } that is only called once, seems confusing to me.


 
> > Source/WebKit/GPUProcess/graphics/ScopedWebGLRenderingResourcesRequest.h:34
> > +class ScopedWebGLRenderingResourcesRequest {
> 
> It's crazy to repeat all this code again. There should be a base class that
> does the atomic counting stuff (I'm surprised we don't have one already).

Well, I thought about it but as far as I can see using curiously recurring template pattern you probably write the same lines with much more complex result.
With non-template, you'd use vfuncs and still write roughly the same lines.

Extracting to static instance "static MyCountingRequest s_requests" reduces to nothing more than "using MyCountingRequest = std::atomic<unsigned>" with ++ and -- reduced. And you still write the same lines.

It is literally 5 lines of code, with couple of ++/--, compare and a function call.
I know this can be abstracted in C++, I just don't think it can be *usefully* abstracted in C++. All the approaches that I simulated in my head would produce just pointless structure without benefit to the reader.

> > Source/WebKit/GPUProcess/graphics/ScopedWebGLRenderingResourcesRequest.h:80
> > +    ScopedRenderingResourcesRequest m_renderingResourcesRequest;
> 
> It's slightly odd that this class holds a ScopedRenderingResourcesRequest
> but then duplicates its functionality. Maybe the WebGL code should just hold
> a ScopedRenderingResourcesRequest too?

What's odd in that? Like refcounted class holding a unique_ptr?

But what would your proposal concretely solve? There's no complexity in holding it in WebGL? There's a concrete intent: you cannot purge the rendering resources while intending to not purge webgl resources. Wouldn't that be a _cause_ of duplication. There's no reduced duplication by moving it out, only increased possibility bugs and duplication. Starting from the fact that it would be suboptimal to decouple then, since then the reader would ask "what's the point of providing the user the possibility of shooting themselves to foot by purging the metal while holding an intent to use the metal through webgl". I mean, even if there was no bug?

[Simon Fraser (smfr)]

Comment on attachment 424918 [details]
Patch

r+ to get this in but I think it could be a bit cleaner.

[EWS]

Committed r275403: <https://commits.webkit.org/r275403>

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