Bug 160238 – [Coordinated Graphics] Improve scheduling of tasks between threads in CoordinatedGraphicsScene

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

Avoid scheduling tasks to the main thread if the scene is detached. Do not take references when not actually sending tasks to another threads. Use Function instead of std::function on dispatch methods. Remove purgeBackingStores that is actually never called.

[Carlos Garcia Campos]

Created attachment 284684 [details]
Patch

[Michael Catanzaro]

Comment on attachment 284684 [details]
Patch

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

Questions

> Source/WebKit2/ChangeLog:12
> +         - Use Function instead of std::function on dispatch methods.

Why, just to use our thing instead of the standard one? Is there an advantage to WTF::Function?

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:44
> +    RunLoop::main().dispatch([protectedThis = makeRef(*this), function = WTFMove(function)] {

Why is this correct? We don't use |this| inside the lambda, so why the need to keep it alive? If the ref is really needed then it should be handled inside |function|, right?

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:56
> +    m_clientRunLoop.dispatch([protectedThis = makeRef(*this), function = WTFMove(function)] {

Ditto.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:158
> +    dispatchOnClientRunLoop([this] {

Why no protector?

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:622
> +    dispatchOnMainThread([this] {

Ditto.

> Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:689
> +    dispatchOnMainThread([this, layerID, offset] {

Ditto.

[Carlos Garcia Campos]

(In reply to comment #2)
> Comment on attachment 284684 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284684&action=review
> 
> Questions
> 
> > Source/WebKit2/ChangeLog:12
> > +         - Use Function instead of std::function on dispatch methods.
> 
> Why, just to use our thing instead of the standard one? Is there an
> advantage to WTF::Function?

This is WTF::Function.

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:44
> > +    RunLoop::main().dispatch([protectedThis = makeRef(*this), function = WTFMove(function)] {
> 
> Why is this correct? We don't use |this| inside the lambda, so why the need
> to keep it alive? If the ref is really needed then it should be handled
> inside |function|, right?

Just to ensure the scene is alive, that's why it's called protectedThis. Before this patch all caller do that themselves, and in some cases unnecessarily, because we only need it when scheduling to another thread. Now it's done here to simplify the callers.

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:56
> > +    m_clientRunLoop.dispatch([protectedThis = makeRef(*this), function = WTFMove(function)] {
> 
> Ditto.

Ditto.

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:158
> > +    dispatchOnClientRunLoop([this] {
> 
> Why no protector?

Because dispatchOnClientRunLoop is protecting this only when actually needed.

> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:622
> > +    dispatchOnMainThread([this] {
> 
> Ditto.
> 
> > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsScene.cpp:689
> > +    dispatchOnMainThread([this, layerID, offset] {
> 
> Ditto.

Ditto.

[Michael Catanzaro]

(In reply to comment #3) 
> This is WTF::Function.

Why is WTF::Function preferred to std::function?

[Carlos Garcia Campos]

(In reply to comment #4)
> (In reply to comment #3) 
> > This is WTF::Function.
> 
> Why is WTF::Function preferred to std::function?

WTF::Function is non-copyable, so it's safer to use between threads and allows to capture things like Ref<>

[Carlos Garcia Campos]

Committed r203776: <http://trac.webkit.org/changeset/203776>