Bug 167544 – [GTK] Do not release OpenGL resource immediately when leaving accelerated compositing mode

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

Sometimes the conditions to be in AC mode or not change quickly, and then we leave AC mode just enter it again after a very short period of time. In those cases we are dropping all the GL resources and the compositor thread, and creating it again. We could keep the layer tree host alive for a while when exiting AC mode, and reuse it if we enter AC mode before the previous one has been discarded. While the previous layer tree host is alive we still need to keep it up to date, for example if the web view is resized or contents size change, and synchronize with the threaded compositor when it becomes the layer tree host again.

[Carlos Garcia Campos]

Created attachment 300011 [details]
Patch

I'm using a 5 seconds delay for now and seems work fine. We should probably hook the memory pressure handler to discard the previous layer tree host on memory pressure too, but we can do that in a follow up

[Carlos Garcia Campos]

Doesn't apply because it depends on #167494

[Michael Catanzaro]

Comment on attachment 300011 [details]
Patch

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

Nice!

Now I know all this recent work to bring back on-demand accelerated composting is going to have a dramatic memory reduction benefit, but these changes seem risky and I don't think we should backport them to 2.14 now that we're nearly to the end of the release cycle. My recommendation is to let it bake in trunk at this point. Not just this commit, but the whole set. It's your call, of course.

> Source/WebKit2/ChangeLog:24
> +        starting a timer of 5 secons to discard it if not reused.

seconds

> Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:382
> +    m_previousLayerTreeHost = WTFMove(m_layerTreeHost);

If something ever goes wrong, better to have a null pointer dereference than a use after free, so I think you should set m_layerTreeHost to nullptr here, even if it's not strictly necessary. And it looks like it *is* necessary, since you have checks in several places to only use m_previousLayerTreeHost if m_layerTreeHost is null. If it's really not needed, please explain why.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:229
> +    m_isDiscardable = discardable;
> +    if (m_isDiscardable) {
> +        m_discardableSyncActions = OptionSet<DiscardableSyncActions>();
> +        return;
> +    }

Personally, I would split the rest of this function below this point into a new function named ThreadedCoordinatedLayerTreeHost::applyDiscardableSyncActions.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:111
> +    enum class DiscardableSyncActions { UpdateSize = 1 << 1, UpdateViewport = 1 << 2, UpdateScale = 1 << 3, UpdateBackground = 1 << 4 };

I would also prefer not to write enums on one line like this.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 300011 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=300011&action=review
> 
> Nice!
> 
> Now I know all this recent work to bring back on-demand accelerated
> composting is going to have a dramatic memory reduction benefit, but these
> changes seem risky and I don't think we should backport them to 2.14 now
> that we're nearly to the end of the release cycle. My recommendation is to
> let it bake in trunk at this point. Not just this commit, but the whole set.
> It's your call, of course.

All this work is to make 2.14 usable again for many people. so all these commits expect the one adding new API will be backported to 2.14, but not right now. My plan is to make a new 2.15 release as soon as possible next week, once the tests are in better shape. Then we test the new release for a while, and continue fixing issues that might show up in the bots, ro reported by Andrés. And then, with all the fixes I'll backport everything to 2.14 and make a new release.

> > Source/WebKit2/ChangeLog:24
> > +        starting a timer of 5 secons to discard it if not reused.
> 
> seconds
> 
> > Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:382
> > +    m_previousLayerTreeHost = WTFMove(m_layerTreeHost);
> 
> If something ever goes wrong, better to have a null pointer dereference than
> a use after free, so I think you should set m_layerTreeHost to nullptr here,
> even if it's not strictly necessary. And it looks like it *is* necessary,
> since you have checks in several places to only use m_previousLayerTreeHost
> if m_layerTreeHost is null. If it's really not needed, please explain why.

Not it's not needed at all. Because m_layerTreeHost is a RefPtr, so !m_layerTreeHost checks the internal pointer and the move assignment already sets the internal pointer to nullptr.

> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:229
> > +    m_isDiscardable = discardable;
> > +    if (m_isDiscardable) {
> > +        m_discardableSyncActions = OptionSet<DiscardableSyncActions>();
> > +        return;
> > +    }
> 
> Personally, I would split the rest of this function below this point into a
> new function named
> ThreadedCoordinatedLayerTreeHost::applyDiscardableSyncActions.
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.h:111
> > +    enum class DiscardableSyncActions { UpdateSize = 1 << 1, UpdateViewport = 1 << 2, UpdateScale = 1 << 3, UpdateBackground = 1 << 4 };
> 
> I would also prefer not to write enums on one line like this.

[Carlos Garcia Campos]

Committed r211365: <http://trac.webkit.org/changeset/211365>