Bug 72752 – [Chromium] Upload textures in parallel with painting contents

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[David Reveman]

Start uploading texture resources as soon as enough contents has been painted for one texture to be updated.

[David Reveman]

Created attachment 115869 [details]
Patch

[David Reveman]

Created attachment 115886 [details]
Patch

[David Reveman]

Created attachment 116005 [details]
Patch

[David Reveman]

Created attachment 117073 [details]
Patch

With changes from https://bugs.webkit.org/show_bug.cgi?id=72192 folded into this patch.

[David Reveman]

Created attachment 117218 [details]
Patch

Add a few comments about thread restrictions to LayerTextureUpdater::Texture functions and remove unnecessary semi-colon.

[David Reveman]

Created attachment 117624 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[David Levin]

Comment on attachment 117624 [details]
Patch

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

A few drive by comments.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:299
> +    CCProxy::implThread()->postTask(createCCThreadTask(this, &CCThreadProxy::updateTextureOnImplThread, AllowCrossThreadAccess(entry)));

AllowCrossThreadAccess is an out for when there is no other lifetime management possible and requires really careful thought.

It looks like entry should be a OwnPtr and this call should do a .release() on it instead of using AllowCrossThreadAccess.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:439
> +void CCThreadProxy::updateTextureOnImplThread(UpdateEntry* entry)

entry should be a PassOwnPtr.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1019
> +        explicit ContentLayerTextureUpdater(ContentLayerChromiumWithUpdateTracking *layer) : m_layer(layer) { }

* in wrong place.

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1024
> +    static PassRefPtr<ContentLayerChromiumWithUpdateTracking> create(CCLayerDelegate *delegate, bool needPrepareRect)

Ditto.

[David Reveman]

Created attachment 117646 [details]
Patch

[David Reveman]

(In reply to comment #8)
> (From update of attachment 117624 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117624&action=review
> 
> A few drive by comments.

Thanks! I updated the patch based on your comments.

[James Robinson]

I'm not sure why this patch is marked as blocking https://bugs.webkit.org/show_bug.cgi?id=71869. https://bugs.webkit.org/show_bug.cgi?id=71869 seems like it would work fine without this, right? Given my druthers, with my overloaded reviewer hat on, I'd rather spend time getting that patch figured out and landed first since it's conceptually much simpler and will provide us a much bigger bang for our buck in the short term.

[David Reveman]

Created attachment 118058 [details]
Patch

[David Reveman]

(In reply to comment #11)
> I'm not sure why this patch is marked as blocking https://bugs.webkit.org/show_bug.cgi?id=71869. https://bugs.webkit.org/show_bug.cgi?id=71869 seems like it would work fine without this, right? Given my druthers, with my overloaded reviewer hat on, I'd rather spend time getting that patch figured out and landed first since it's conceptually much simpler and will provide us a much bigger bang for our buck in the short term.

I updated 71869 to no longer depend on this bug.

[David Reveman]

Created attachment 118207 [details]
Patch

[David Reveman]

*** Bug 72192 has been marked as a duplicate of this bug. ***

[David Levin]

I likely won't review this, but this patch is super huge.

In general it is better to do small focused patch rather than try to do everything in one patch.

For example, I was trying to locate one piece of this earlier (where it did threading) and it was pretty hard. Hopefully folks that you work with can give you advice on making smaller patches.

[David Reveman]

Created attachment 118412 [details]
Patch

[David Reveman]

(In reply to comment #16)
> I likely won't review this, but this patch is super huge.
> 
> In general it is better to do small focused patch rather than try to do everything in one patch.
> 
> For example, I was trying to locate one piece of this earlier (where it did threading) and it was pretty hard. Hopefully folks that you work with can give you advice on making smaller patches.

I'm not happy with the size of this patch either. It was previously two patches. One re-factoring patch and one that adds the feature described by the title of this bug. I was asked to merge these into one patch to avoid landing just a re-factoring patch that doesn't provide any direct benefit. I'd prefer to land the re-factoring changes first but will leave this up to jamesr to decide.

I'm working on moving the scheduler changes into a follow up patch. That will make this patch a bit smaller.

[James Robinson]

One good goal is that a patch should be the smallest amount of testable code. A smaller patch that does not do anything is not generally an improvement.  That said since this patch is about 150kb I haven't attempted to review it yet.

My understanding is that https://bugs.webkit.org/show_bug.cgi?id=72672 is required before this patch for correctness, so I'm a bit surprised to see the bugzilla dependency order the other way around. I was not planning to look at this patch at all until the atomicity issue was addressed.

[David Reveman]

Created attachment 118442 [details]
Patch

[David Reveman]

(In reply to comment #19)
> One good goal is that a patch should be the smallest amount of testable code. A smaller patch that does not do anything is not generally an improvement.  That said since this patch is about 150kb I haven't attempted to review it yet.
> 
> My understanding is that https://bugs.webkit.org/show_bug.cgi?id=72672 is required before this patch for correctness, so I'm a bit surprised to see the bugzilla dependency order the other way around. I was not planning to look at this patch at all until the atomicity issue was addressed.

72672 is not required. 72672 is required for this to be enabled in the threaded compositor. To make this patch smaller, I just moved everything related to the threaded compositor use case over to this bug: https://bugs.webkit.org/show_bug.cgi?id=74113

72672 was previously enabling parallel uploads for the threaded compositor but that's all in 74113 now.

This patch only makes it possible to do texture uploads in parallel with painting for the single threaded compositor.

[David Reveman]

Created attachment 118593 [details]
Patch

[David Reveman]

Created attachment 118710 [details]
Patch

[David Reveman]

Latest patch moves all the resource initialization changes required for threaded parallel paint/upload to a separate patch as that's no longer required since I moved threaded support to a separate patch.

[David Reveman]

Created attachment 120668 [details]
Patch

[David Reveman]

Created attachment 121160 [details]
Patch

[David Reveman]

Created attachment 122406 [details]
Patch

[WebKit Review Bot]

Comment on attachment 122406 [details]
Patch

Attachment 122406 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11189012

New failing tests:
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/lost-compositor-context-twice.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html

[David Reveman]

Created attachment 123459 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123459 [details]
Patch

Attachment 123459 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11323075

New failing tests:
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/lost-compositor-context-twice.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html

[David Reveman]

Created attachment 123557 [details]
Patch

[David Reveman]

Created attachment 124307 [details]
Patch

[David Reveman]

Comment on attachment 124307 [details]
Patch

Rebased.

[WebKit Review Bot]

Comment on attachment 124307 [details]
Patch

Attachment 124307 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11350762

New failing tests:
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/lost-compositor-context-twice.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html

[James Robinson]

(In reply to comment #34)
> (From update of attachment 124307 [details])
> Attachment 124307 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11350762
> 
> New failing tests:
> platform/chromium/compositing/lost-compositor-context-permanently.html
> platform/chromium/compositing/lost-compositor-context.html
> platform/chromium/compositing/webgl-loses-compositor-context.html
> platform/chromium/compositing/lost-compositor-context-twice.html
> platform/chromium/compositing/lost-compositor-context-with-rendersurface.html

Are you looking into these?

[James Robinson]

Comment on attachment 124307 [details]
Patch

A few high level points (I don't plan to look terribly closely at the exact details until incremental uploading is up and running since IMO that's much more important):

1.) I'm pretty sure we can't afford the thread kick in the middle of painting on mobile, the context switching is too expensive. At some hardware class above that the extra parallelism starts to become a win even with the extra scheduler overhead, but I'm not sure exactly where that is.  This has two implications:
 a.) We can't write any code that *depends* on this happening. All code has to work with either parallel uploads or without it.
 b.) We need a way to control parallel uploading. It can be as simple as #ifdefs in the right place, or a setting somewhere, but it needs to be clearly marked.

2.) I don't think we need yet another visitor abstraction here. If we want layers to do something special at paint time just add the call that they need to make at the right place. I'm also not sure why we need a separate prepareToUpdate() pass from the paint pass, but perhaps that'll be clear when looking at the details.

In general this patch seems to have a lot of spurious renames and code changes (like changing from "contentsTextureManager()->" to "m_contentsTextureManager->") that make the diff a lot larger and harder to read than it has to be. Please try to cut these down since the patch is already too large.

[David Reveman]

(In reply to comment #36)
> (From update of attachment 124307 [details])
> A few high level points (I don't plan to look terribly closely at the exact details until incremental uploading is up and running since IMO that's much more important):
> 
> 1.) I'm pretty sure we can't afford the thread kick in the middle of painting on mobile, the context switching is too expensive. At some hardware class above that the extra parallelism starts to become a win even with the extra scheduler overhead, but I'm not sure exactly where that is.  This has two implications:
>  a.) We can't write any code that *depends* on this happening. All code has to work with either parallel uploads or without it.
>  b.) We need a way to control parallel uploading. It can be as simple as #ifdefs in the right place, or a setting somewhere, but it needs to be clearly marked.

I agree. The current delayTextureUpdates setting exists for this purpose. The impl thread will wait with all texture updates until beginFrameComplete is called when this is set. The texture updates are still posted to the impl thread as soon as available. That could easily be changed but I assumed that this isn't a problem. If it is, then that seems like something that should be solved separately.

> 
> 2.) I don't think we need yet another visitor abstraction here. If we want layers to do something special at paint time just add the call that they need to make at the right place. I'm also not sure why we need a separate prepareToUpdate() pass from the paint pass, but perhaps that'll be clear when looking at the details.

The patch makes it possible to start uploading a soon as one tile has been painted. Some re-factoring is necessary to be able to do that and the separate prepareToUpdate pass has to do with that. We can discuss the best way to do this when you've had time to look at the details.

> 
> In general this patch seems to have a lot of spurious renames and code changes (like changing from "contentsTextureManager()->" to "m_contentsTextureManager->") that make the diff a lot larger and harder to read than it has to be. Please try to cut these down since the patch is already too large.

The only unnecessary renames in this patch should be the ones that fix the contentsTextureManager vs m_contentsTextureManager inconsistency. I'm happy to move those into a separate patch.

[David Reveman]

(In reply to comment #35)
> (In reply to comment #34)
> > (From update of attachment 124307 [details] [details])
> > Attachment 124307 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/11350762
> > 
> > New failing tests:
> > platform/chromium/compositing/lost-compositor-context-permanently.html
> > platform/chromium/compositing/lost-compositor-context.html
> > platform/chromium/compositing/webgl-loses-compositor-context.html
> > platform/chromium/compositing/lost-compositor-context-twice.html
> > platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
> 
> Are you looking into these?

I will do that, asap.

[David Reveman]

Created attachment 125025 [details]
Patch

[David Reveman]

Created attachment 125710 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125710 [details]
Patch

Attachment 125710 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11427757

New failing tests:
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/lost-compositor-context-twice.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html

[David Reveman]

Created attachment 125962 [details]
Patch

Rebase

[WebKit Review Bot]

Comment on attachment 125962 [details]
Patch

Attachment 125962 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11461239

New failing tests:
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/webgl-loses-compositor-context.html
compositing/masks/masked-ancestor.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/lost-compositor-context-twice.html

[David Reveman]

Created attachment 126645 [details]
Patch

Rebase

[WebKit Review Bot]

Comment on attachment 126645 [details]
Patch

Attachment 126645 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11509211

New failing tests:
platform/chromium/compositing/lost-compositor-context-with-video.html
platform/chromium/compositing/lost-compositor-context.html
platform/chromium/compositing/webgl-loses-compositor-context.html
platform/chromium/compositing/lost-compositor-context-with-rendersurface.html
platform/chromium/compositing/lost-compositor-context-permanently.html
platform/chromium/compositing/lost-compositor-context-twice.html

[David Reveman]

Created attachment 129331 [details]
Patch

Rebase and fix layout test crashes

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail] or [e-mail] before submitting because this patch contains changes to the Chromium platform API.