Bug 72752 - [Chromium] Upload textures in parallel with painting contents
Summary: [Chromium] Upload textures in parallel with painting contents
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
Depends on: 71388
Blocks: 72192 73279 74113
  Show dependency treegraph
 
Reported: 2011-11-18 13:15 PST by David Reveman
Modified: 2013-04-08 14:42 PDT (History)
11 users (show)

See Also:


Attachments
Patch (27.90 KB, patch)
2011-11-18 13:51 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (28.51 KB, patch)
2011-11-18 15:19 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (29.51 KB, patch)
2011-11-20 16:07 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (121.50 KB, patch)
2011-11-29 16:58 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (121.87 KB, patch)
2011-11-30 10:32 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (140.53 KB, patch)
2011-12-02 08:03 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (140.44 KB, patch)
2011-12-02 10:08 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (121.90 KB, patch)
2011-12-06 09:04 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (145.48 KB, patch)
2011-12-07 07:26 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (146.89 KB, patch)
2011-12-08 10:02 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (121.10 KB, patch)
2011-12-08 12:12 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (121.01 KB, patch)
2011-12-09 10:19 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (86.42 KB, patch)
2011-12-11 13:14 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (91.08 KB, patch)
2011-12-28 07:29 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (92.47 KB, patch)
2012-01-04 14:51 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (98.19 KB, patch)
2012-01-13 03:31 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (105.33 KB, patch)
2012-01-21 15:23 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (105.39 KB, patch)
2012-01-23 07:58 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (110.94 KB, patch)
2012-01-27 06:47 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (119.53 KB, patch)
2012-02-01 15:04 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (119.85 KB, patch)
2012-02-06 14:44 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (120.82 KB, patch)
2012-02-07 17:28 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (131.57 KB, patch)
2012-02-11 14:00 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (137.31 KB, patch)
2012-02-28 14:25 PST, David Reveman
benjamin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2011-11-18 13:15:28 PST
Start uploading texture resources as soon as enough contents has been painted for one texture to be updated.
Comment 1 David Reveman 2011-11-18 13:51:14 PST
Created attachment 115869 [details]
Patch
Comment 2 David Reveman 2011-11-18 15:19:11 PST
Created attachment 115886 [details]
Patch
Comment 3 David Reveman 2011-11-20 16:07:16 PST
Created attachment 116005 [details]
Patch
Comment 4 David Reveman 2011-11-29 16:58:14 PST
Created attachment 117073 [details]
Patch

With changes from https://bugs.webkit.org/show_bug.cgi?id=72192 folded into this patch.
Comment 5 David Reveman 2011-11-30 10:32:17 PST
Created attachment 117218 [details]
Patch

Add a few comments about thread restrictions to LayerTextureUpdater::Texture functions and remove unnecessary semi-colon.
Comment 6 David Reveman 2011-12-02 08:03:26 PST
Created attachment 117624 [details]
Patch
Comment 7 WebKit Review Bot 2011-12-02 08:05:54 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 8 David Levin 2011-12-02 08:18:30 PST
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.
Comment 9 David Reveman 2011-12-02 10:08:54 PST
Created attachment 117646 [details]
Patch
Comment 10 David Reveman 2011-12-02 10:38:10 PST
(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.
Comment 11 James Robinson 2011-12-05 15:11:18 PST
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.
Comment 12 David Reveman 2011-12-06 09:04:00 PST
Created attachment 118058 [details]
Patch
Comment 13 David Reveman 2011-12-06 09:05:07 PST
(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.
Comment 14 David Reveman 2011-12-07 07:26:47 PST
Created attachment 118207 [details]
Patch
Comment 15 David Reveman 2011-12-07 07:31:28 PST
*** Bug 72192 has been marked as a duplicate of this bug. ***
Comment 16 David Levin 2011-12-07 22:09:51 PST
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.
Comment 17 David Reveman 2011-12-08 10:02:35 PST
Created attachment 118412 [details]
Patch
Comment 18 David Reveman 2011-12-08 10:09:04 PST
(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.
Comment 19 James Robinson 2011-12-08 12:03:39 PST
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.
Comment 20 David Reveman 2011-12-08 12:12:34 PST
Created attachment 118442 [details]
Patch
Comment 21 David Reveman 2011-12-08 12:27:37 PST
(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.
Comment 22 David Reveman 2011-12-09 10:19:00 PST
Created attachment 118593 [details]
Patch
Comment 23 David Reveman 2011-12-11 13:14:53 PST
Created attachment 118710 [details]
Patch
Comment 24 David Reveman 2011-12-11 16:37:35 PST
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.
Comment 25 David Reveman 2011-12-28 07:29:56 PST
Created attachment 120668 [details]
Patch
Comment 26 David Reveman 2012-01-04 14:51:52 PST
Created attachment 121160 [details]
Patch
Comment 27 David Reveman 2012-01-13 03:31:18 PST
Created attachment 122406 [details]
Patch
Comment 28 WebKit Review Bot 2012-01-13 04:22:55 PST
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
Comment 29 David Reveman 2012-01-21 15:23:44 PST
Created attachment 123459 [details]
Patch
Comment 30 WebKit Review Bot 2012-01-21 15:59:07 PST
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
Comment 31 David Reveman 2012-01-23 07:58:48 PST
Created attachment 123557 [details]
Patch
Comment 32 David Reveman 2012-01-27 06:47:34 PST
Created attachment 124307 [details]
Patch
Comment 33 David Reveman 2012-01-27 06:48:05 PST
Comment on attachment 124307 [details]
Patch

Rebased.
Comment 34 WebKit Review Bot 2012-01-27 07:25:48 PST
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
Comment 35 James Robinson 2012-01-30 16:41:00 PST
(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?
Comment 36 James Robinson 2012-01-30 17:01:51 PST
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.
Comment 37 David Reveman 2012-01-31 11:06:15 PST
(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.
Comment 38 David Reveman 2012-01-31 11:07:23 PST
(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.
Comment 39 David Reveman 2012-02-01 15:04:25 PST
Created attachment 125025 [details]
Patch
Comment 40 David Reveman 2012-02-06 14:44:48 PST
Created attachment 125710 [details]
Patch
Comment 41 WebKit Review Bot 2012-02-06 15:18:45 PST
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
Comment 42 David Reveman 2012-02-07 17:28:35 PST
Created attachment 125962 [details]
Patch

Rebase
Comment 43 WebKit Review Bot 2012-02-07 19:05:32 PST
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
Comment 44 David Reveman 2012-02-11 14:00:30 PST
Created attachment 126645 [details]
Patch

Rebase
Comment 45 WebKit Review Bot 2012-02-11 14:52:38 PST
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
Comment 46 David Reveman 2012-02-28 14:25:45 PST
Created attachment 129331 [details]
Patch

Rebase and fix layout test crashes
Comment 47 WebKit Review Bot 2012-02-28 14:28:29 PST
Please wait for approval from fishd@chromium.org, abarth@webkit.org or jamesr@chromium.org before submitting because this patch contains changes to the Chromium platform API.