Closed
Bug 1150549
Opened 9 years ago
Closed 9 years ago
Simplify TiledContentHost
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: nical, Assigned: nical)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 7 obsolete files)
55.02 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
ThebesLayerBufferComposite is way more complicated than it needs to be. For one, it is based TiledLayerBuffer's templated Update function but doesn't need most of what Update does. It is also treated as a temporary object recreated each update, which makes a few things tricky, like deferring the release of the gfxSharedReadLock to the composition of frame N+1 (right now we keep a half-valid "mOldTiledLayerBuffer" around for that), and going through the fast paths with gralloc buffers. It's not surprising since we had very a tight deadline when we wrote it but now is a good time to clean it up (actually, we should not have waited that long).
Assignee | ||
Comment 1•9 years ago
|
||
With this patch, TiledLayerBufferComposite is kept across frames which makes it much easier to track the gfxSharedReadLocks and avoid unbinding gralloc buffers for no reason (which we currently do). This patch also implements all of the update logic in TiledLayerBufferComposite::UseTiles instead of going through TiledLayerBuffer::Update which is way more complicated and does a lot mor ethan what we care about. Aside from making things simpler on the host side, it'll make it easier to simplify the client side. I still need to implement partial updates on tiles, which I intend to do by passing the bound of each tile's update region rather than reconstructing the update region on the host side. It should make things faster because 1 rather small glTexSubImage2D is faster than several very small glTexSubImage2D, and simpler.
Assignee | ||
Comment 2•9 years ago
|
||
Added partial uploads, works like a charm on Linux.
Assignee | ||
Comment 3•9 years ago
|
||
This version of the patch is green on try, except for the (already existing) ics emulator r14 intermittent. On b2g it should improve performance on the compositor side since it removes a lot of the slow egl binding that we are doing (we used to re-bind every tile after each transaction, and now we only do that for the one s that changed).
Attachment #8587454 -
Attachment is obsolete: true
Attachment #8587508 -
Attachment is obsolete: true
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8592178 -
Attachment is obsolete: true
Attachment #8592816 -
Flags: review?(jmuizelaar)
Comment 5•9 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #4) > Created attachment 8592816 [details] [diff] [review] > Simplify and improve the compositor-side tiling code Do you mind if I ignore this review until next week after people leave Toronto?
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5) > (In reply to Nicolas Silva [:nical] from comment #4) > > Created attachment 8592816 [details] [diff] [review] > > Simplify and improve the compositor-side tiling code > > Do you mind if I ignore this review until next week after people leave > Toronto? No problem.
Comment 7•9 years ago
|
||
Comment on attachment 8592816 [details] [diff] [review] Simplify and improve the compositor-side tiling code Review of attachment 8592816 [details] [diff] [review]: ----------------------------------------------------------------- One comment so far: - It might be better to have a static PlaceHolderTile that gets used as needed instead of moving the bounds checking out of GetTile
Comment 8•9 years ago
|
||
Can you rebase this patch against trunk or use moz-review?
Flags: needinfo?(nical.bugzilla)
Assignee | ||
Comment 9•9 years ago
|
||
Rebased patch, also incorporates your suggestion about GetTile.
Attachment #8592816 -
Attachment is obsolete: true
Attachment #8592816 -
Flags: review?(jmuizelaar)
Flags: needinfo?(nical.bugzilla)
Attachment #8600897 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
There was a rebasing mistake on the previous version. Also, this one is rebased on top of m-c directly without requiring another patch.
Attachment #8600897 -
Attachment is obsolete: true
Attachment #8600935 -
Attachment is obsolete: true
Attachment #8600897 -
Flags: review?(jmuizelaar)
Attachment #8600949 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 12•9 years ago
|
||
reftests pass on try: https://treeherder.mozilla.org/#/jobs?repo=try&author=nsilva@mozilla.com
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #12) > reftests pass on try: > https://treeherder.mozilla.org/#/jobs?repo=try&author=nsilva@mozilla.com Actually I meant https://treeherder.mozilla.org/#/jobs?repo=try&revision=eeeb6afe9b49
Comment 14•9 years ago
|
||
I put a version of this on reviewboard with the hope that it would make the diff better. It does not seem to have: https://reviewboard.mozilla.org/r/8175/diff/
Comment 15•9 years ago
|
||
Comment on attachment 8600949 [details] [diff] [review] Simplify and improve the compositor-side tiling code Review of attachment 8600949 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/TiledLayerBuffer.h @@ +201,2 @@ > int mRetainedWidth; // in tiles > int mRetainedHeight; // in tiles It seems like these ints are basically an IntRect. Storing them as such will make that more obvious that they are all related and use the same units. I'm a bit worried that mFirstTileXY makes it harder for us to have tiles start at layer origin instead of document origin. Do you have any thoughts on this? ::: gfx/layers/composite/TiledContentHost.cpp @@ +338,5 @@ > +void > +TiledLayerBufferComposite::Clear() > +{ > + for (size_t i = 0; i < mRetainedTiles.Length(); ++i) { > + TileHost& tile = mRetainedTiles[i]; for (TileHost &tile : mRetainedTiles) {
Comment 16•9 years ago
|
||
Comment on attachment 8600949 [details] [diff] [review] Simplify and improve the compositor-side tiling code Review of attachment 8600949 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/LayersMessages.ipdlh @@ +314,5 @@ > > struct TexturedTileDescriptor { > PTexture texture; > MaybeTexture textureOnWhite; > + nsIntRegion updateRegion; I don't think we want this. I think we're probably not going to get any advantage from tracking updates in tiles on a per region basis. At most, a single rect should be enough. I've been talking with mstange about improving the region simplification that we do in layout to basically work as a rect per tile instead of a complicated region. Basically, I'd drop this from the patch for now and we can investigate adding it back as justified by numbers.
Assignee | ||
Comment 17•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #16) > ::: gfx/layers/ipc/LayersMessages.ipdlh > @@ +314,5 @@ > > > > struct TexturedTileDescriptor { > > PTexture texture; > > MaybeTexture textureOnWhite; > > + nsIntRegion updateRegion; > > I don't think we want this. I think we're probably not going to get any > advantage from tracking updates in tiles on a per region basis. At most, a > single rect should be enough. I have gone back and forth with this. I originally only shipped a rectangle, but Bas pointed out that we'd be using very large tiles with accelerated moz2d backends like D2D, and that it would be better to use regions there. I personally prefer to avoid using regions. > > I've been talking with mstange about improving the region simplification > that we do in layout to basically work as a rect per tile instead of a > complicated region. > > Basically, I'd drop this from the patch for now and we can investigate > adding it back as justified by numbers. Sounds good.
Assignee | ||
Comment 18•9 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #15) > > It seems like these ints are basically an IntRect. Storing them as such will > make that more obvious that they are all related and use the same units. > > I'm a bit worried that mFirstTileXY makes it harder for us to have tiles > start at layer origin instead of document origin. Do you have any thoughts > on this? > I don't have thoughts on this yet, this code comes directly from TiledLayerBuffer.h. I tried to avoid changing the client-side logic in this patch, but I want to clean the client side code up eventually too so we can look into this when I get there.
Assignee | ||
Comment 19•9 years ago
|
||
Review comments addressed except the mFirstTileX/Y and mretainedWidth/Height stuff which I'll sort out when I get around cleaning the client side code up.
Attachment #8600949 -
Attachment is obsolete: true
Attachment #8600949 -
Flags: review?(jmuizelaar)
Attachment #8608679 -
Flags: review?(nical.bugzilla)
Assignee | ||
Updated•9 years ago
|
Attachment #8608679 -
Flags: review?(nical.bugzilla) → review?(jmuizelaar)
Updated•9 years ago
|
Attachment #8608679 -
Flags: review?(jmuizelaar) → review+
Comment 22•9 years ago
|
||
Backed out for OSX asserts. https://treeherder.mozilla.org/logviewer.html#?job_id=10067251&repo=mozilla-inbound
Comment 23•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/00f2351e7744
Comment 26•9 years ago
|
||
Backed out for Mulet crashes. https://treeherder.mozilla.org/logviewer.html#?job_id=10128604&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/8911b21aa87c
Comment 27•9 years ago
|
||
Same for B2G Desktop. Other platforms appear to have been unaffected.
https://hg.mozilla.org/mozilla-central/rev/67efcb1efc43
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 30•9 years ago
|
||
I received a nice notification email that this patch made a 18.2% improvement in tp5_scroll on Mac. I was also told that it made a big difference on unagi. B2G is the place where I expect this to make the biggest difference.
Comment 31•9 years ago
|
||
This seems to cause the regression of flickering. bug 1169339
(In reply to Nicolas Silva [:nical] from comment #30) > ...B2G is the place where I expect this to make the biggest difference. (In reply to Sotaro Ikeda [:sotaro] from comment #31) > This seems to cause the regression of flickering. bug 1169339 Wrong kind of a difference :) Let's back this out if we can't fix it within a day.
Comment 33•9 years ago
|
||
FYI bug 1168015 landed on top this and needed a rebase, so backing this out will require either backing that out as well or doing more rebase work. Unless the regression breaks smoketests it might be better to give Nical a bit more time to try to fix it in place.
Assignee | ||
Comment 34•9 years ago
|
||
I am sure backing this out can wait for a few days.
You need to log in
before you can comment on or make changes to this bug.
Description
•