Closed Bug 1452390 Opened 8 years ago Closed 8 years ago

We don't implement paint skipping for WebRender (less paints during scrolling)

Categories

(Core :: Graphics: WebRender, enhancement, P1)

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: jrmuizel, Assigned: kats)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Bug 1452390 - Make WebRenderScrollData::HasMetadataFor work properly on the parent side.
59 bytes, text/x-review-board-request
botond
: review+
Details
Bug 1452390 - Implement paint-skipping support in WebRender.
59 bytes, text/x-review-board-request
botond
: review+
Details
Here's a profile https://perfht.ml/2Hi8T3g that show nearly constant repainting while scrolling up and down on https://news.ycombinator.com/item?id=16775093 I don't see the same thing with WebRender off: https://perfht.ml/2Hggcsf
Summary: We seem to paint more often when scrolling then without WebRender → We seem to paint more often when scrolling than without WebRender
Blocks: 1452489
Assignee: nobody → bugmail
Summary: We seem to paint more often when scrolling than without WebRender → We don't implement paint skipping for WebRender (less paints during scrolling)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #0) > Here's a profile https://perfht.ml/2Hi8T3g that show nearly constant > repainting while scrolling up and down on > https://news.ycombinator.com/item?id=16775093 > > I don't see the same thing with WebRender off: > > https://perfht.ml/2Hggcsf So... actually your profile isn't going to be helped by the empty transaction thing at all. The profile shows the Renderer thread, which is doing a composite triggered by vsync, which is needed for scrolling no matter what. The same thing happens in the non-WR profile, it's just a lot cheaper in the non-WR case. I don't know why it's so expensive in the WR case, but it's not because of display list traffic. It's spending a lot of time in SwapBuffers. The empty transaction changes will help mostly with scrolling that's triggered by the main thread (i.e. from JS). For APZ-triggered scrolling we already have enough paint-skipping enabled that we go down the codepath at [1] which avoids the paints. I confirmed this by turning on the WR profiler HUD and scrolling around on the hacker news page, and there's only displaylist IPC happening when the displayport shifts to a new virtual tile. The same is true with and without the patch I wrote to implement the empty transaction thing. I have a try push going with my patch at [2]. [1] https://searchfox.org/mozilla-central/rev/eb6c5214a63e20a3fff455e92c876287a8d2e188/layout/generic/nsGfxScrollFrame.cpp#2910 [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=46fc6cf0b5e0dbefd80b7b9de70d7c1d0fade44c
The try push is showing a mochitest failure that looks related, I'll investigate. It also shows significant improvement on the tp5o_scroll and tscrollx talos tests, which is what I would expect since both of those do JS-based scrolling. The numbers on those tests with my patch are much better than the equivalent non-WR.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2) > So... actually your profile isn't going to be helped by the empty > transaction thing at all. The profile shows the Renderer thread, which is > doing a composite triggered by vsync, which is needed for scrolling no > matter what. The same thing happens in the non-WR profile, it's just a lot > cheaper in the non-WR case. I don't know why it's so expensive in the WR > case, but it's not because of display list traffic. It's spending a lot of > time in SwapBuffers. I was concerned about the paints happening on the content thread of that profile not the stuff happening on the Renderer thread.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4) > I was concerned about the paints happening on the content thread of that > profile not the stuff happening on the Renderer thread. Oh. Well in the WR profile there's definitely ticks where a scroll event is fired with no repainting, which is basically the pattern I would expect. What should happen is that we scroll for a few ticks until the displayport is forced to move at which point we do a paint of the new area. In the non-WR profile however there seem to be long stretches of ticks where there's a scroll event but no painting, and I don't know how it's managing to do that. Unless you were scrolling only a small amount up and down such that the displayport never shifted? At any rate, I fixed the issue with my patch and new try push is looking green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66af9a3fadfd11e6d4842156842c660070c88139 Will put up the patches for review shortly.
Comment on attachment 8974764 [details] Bug 1452390 - Make WebRenderScrollData::HasMetadataFor work properly on the parent side. https://reviewboard.mozilla.org/r/243164/#review249324
Attachment #8974764 - Flags: review?(botond) → review+
Comment on attachment 8974765 [details] Bug 1452390 - Implement paint-skipping support in WebRender. https://reviewboard.mozilla.org/r/243166/#review249342 Is it unnecessarily heavyweight for us, by comparison, to be sending over all the layers' ScrollMetadatas in a non-WR empty transaction? ::: gfx/layers/apz/public/APZUpdater.h:88 (Diff revision 1) > void UpdateScrollDataAndTreeState(LayersId aRootLayerTreeId, > LayersId aOriginatingLayersId, > const wr::Epoch& aEpoch, > WebRenderScrollData&& aScrollData); > + /** > + * This is called in the WR-enabled when we get an empty transaction that has There is a missing here ("the WR-enabled codepath"?) ::: gfx/layers/apz/src/APZUpdater.cpp:247 (Diff revision 1) > + RefPtr<APZUpdater> self = this; > + RunOnUpdaterThread(aOriginatingLayersId, NS_NewRunnableFunction( > + "APZUpdater::UpdateScrollOffsets", > + [=,updates=Move(aUpdates)]() { > + self->mScrollData[aOriginatingLayersId].ApplyUpdates(updates); > + self->mScrollData[aOriginatingLayersId].SetPaintSequenceNumber(aPaintSequenceNumber); Perhaps this step could be done by ApplyUpdates()? ::: gfx/layers/wr/WebRenderLayerManager.h:198 (Diff revision 1) > // This holds the scroll data that we need to send to the compositor for > // APZ to do it's job > WebRenderScrollData mScrollData; > + // This holds the scroll updates for paint-skipped scrollframes that we > + // plan to send via an empty transaction. > + nsTArray<PendingScrollOffsetUpdate> mPendingScrollUpdates; Any reason we're not just using LayerManager::mPendingScrollUpdates? It's a map from ViewID to ScrollUpdateInfo, which is basically the same data.
(In reply to Botond Ballo [:botond] from comment #9) > > Is it unnecessarily heavyweight for us, by comparison, to be sending over > all the layers' ScrollMetadatas in a non-WR empty transaction? I think it is, yeah. But the code for non-WR empty transactions is pre-existing and changing that might be nontrivial. Also because non-WR layer transactions tend to transmit the layer tree "diff" we do need to update the client-side layer tree metrics in order to make sure things stay in sync. > ::: gfx/layers/apz/public/APZUpdater.h:88 > > + /** > > + * This is called in the WR-enabled when we get an empty transaction that has > > There is a missing here ("the WR-enabled codepath"?) Whoops, will fix. Also it looks like you missed a word in "a missing here" :) > ::: gfx/layers/apz/src/APZUpdater.cpp:247 > > + self->mScrollData[aOriginatingLayersId].ApplyUpdates(updates); > > + self->mScrollData[aOriginatingLayersId].SetPaintSequenceNumber(aPaintSequenceNumber); > > Perhaps this step could be done by ApplyUpdates()? Yup, I can move that. I was debating doing that originally but couldn't decide which was more appropriate. > ::: gfx/layers/wr/WebRenderLayerManager.h:198 > > + // plan to send via an empty transaction. > > + nsTArray<PendingScrollOffsetUpdate> mPendingScrollUpdates; > > Any reason we're not just using LayerManager::mPendingScrollUpdates? It's a > map from ViewID to ScrollUpdateInfo, which is basically the same data. It is, but we can't send a std::map over IPC, so we'll have to convert it to a nsTArray at some point anyway. And if we're using the base LayerManager implementation of SetPendingScrollUpdateForNextTransaction then it also will run the code at [1]. Which should be a no-op, so not a big deal, but I figured I could avoid it this way. But if you prefer I can just use the base implementation, and then convert it to an nsTArray at IPC time, I don't feel too strongly about it. [1] https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/gfx/layers/Layers.cpp#2383-2389
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10) > > ::: gfx/layers/apz/public/APZUpdater.h:88 > > > + /** > > > + * This is called in the WR-enabled when we get an empty transaction that has > > > > There is a missing here ("the WR-enabled codepath"?) > > Whoops, will fix. Also it looks like you missed a word in "a missing here" :) That was deliberate for humorous effect :) > we can't send a std::map over IPC I beg to differ: https://searchfox.org/mozilla-central/rev/3f17a234769d25fca5144ebb8abc8e1cb3c56c16/ipc/chromium/src/chrome/common/ipc_message_utils.h#383
(In reply to Botond Ballo [:botond] from comment #11) > > Whoops, will fix. Also it looks like you missed a word in "a missing here" :) > > That was deliberate for humorous effect :) Lol :) > > we can't send a std::map over IPC > > I beg to differ: > > https://searchfox.org/mozilla-central/rev/ > 3f17a234769d25fca5144ebb8abc8e1cb3c56c16/ipc/chromium/src/chrome/common/ > ipc_message_utils.h#383 :O well that makes things a lot easier. I'll update the patch to use that.
Comment on attachment 8974765 [details] Bug 1452390 - Implement paint-skipping support in WebRender. https://reviewboard.mozilla.org/r/243166/#review249394 r+ with the Move() stuff addressed. ::: gfx/layers/apz/public/APZUpdater.h:94 (Diff revisions 1 - 2) > * side). This function will update the stored scroll offsets and the > * hit-testing tree. > */ > void UpdateScrollOffsets(LayersId aRootLayerTreeId, > LayersId aOriginatingLayersId, > - nsTArray<PendingScrollOffsetUpdate>&& aUpdates, > + const ScrollUpdatesMap&& aUpdates, As with WebRenderScrollData in the regular transaction path, pass this via non-const && and Move() throughout, with a single const_cast in WebRenderBridgeParent [1] to compensate for IPDL generating a const-ref signature. [1] https://searchfox.org/mozilla-central/rev/babf96cf0c37b608e9663e2af73a4d21819c4af1/gfx/layers/wr/WebRenderBridgeParent.cpp#639 ::: gfx/layers/wr/WebRenderBridgeParent.cpp:739 (Diff revisions 1 - 2) > > UpdateAPZFocusState(aFocusTarget); > - if (!aUpdates.IsEmpty()) { > + if (!aUpdates.empty()) { > + // aUpdates is moved into this function but that is not reflected by the > + // function signature due to the way the IPDL generator works. It should be > + // save to move the structure structure all the way to the desired It's safe, but ineffective, to Move() a const-ref: the result with still be a const-ref. We really do need the const_cast that we have in the regular transaction codepath for this to be effective.
Attachment #8974765 - Flags: review?(botond) → review+
Pushed by kgupta@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d8b7069bf6ca Make WebRenderScrollData::HasMetadataFor work properly on the parent side. r=botond https://hg.mozilla.org/integration/autoland/rev/61f51684cc29 Implement paint-skipping support in WebRender. r=botond
(In reply to Botond Ballo [:botond] from comment #16) > ::: gfx/layers/apz/public/APZUpdater.h:94 > (Diff revisions 1 - 2) > > * side). This function will update the stored scroll offsets and the > > * hit-testing tree. > > */ > > void UpdateScrollOffsets(LayersId aRootLayerTreeId, > > LayersId aOriginatingLayersId, > > - nsTArray<PendingScrollOffsetUpdate>&& aUpdates, > > + const ScrollUpdatesMap&& aUpdates, > > As with WebRenderScrollData in the regular transaction path, pass this via > non-const && and Move() throughout, with a single const_cast in > WebRenderBridgeParent [1] to compensate for IPDL generating a const-ref > signature. Thinking about this some more, this was a misguided suggestion. Unlike WebRenderScrollData, which we store in APZUpdater::mScrollData, the updates are something we just loop through and process without storing them. So, just passing a const reference to the map throughout would have been fine, as we never actually attempt to copy it. (I was partly confused by the const&&, and the Move() in WebRenderBridgeParent::RecvEmptyTransaction, which suggested a failed attempt to move the data structure around. With the exception of some very niche use cases [1], const&& is generally a mistake.) Anyways, sorry to make you go through that pointless sprinkling of Move()s :/ [1] https://stackoverflow.com/questions/4938875/do-rvalue-references-to-const-have-any-use
This is the change that I _should_ have suggested when reviewing revision 2.
Attachment #8975223 - Flags: review?(bugmail)
Comment on attachment 8975223 [details] [diff] [review] Bug 1452390 - Pass ScrollUpdatesMap around by const references as we never try to copy it Review of attachment 8975223 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/APZUpdater.cpp @@ +241,5 @@ > MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); > RefPtr<APZUpdater> self = this; > RunOnUpdaterThread(aOriginatingLayersId, NS_NewRunnableFunction( > "APZUpdater::UpdateScrollOffsets", > + [=,&aUpdates]() { This is the part I was unsure about. If we are just capturing aUpdates by reference, doesn't that mean that by the time the lambda runs, the reference is garbage because the caller stack has unwound and the ScrollUpdatesMap is gone? That's why I wanted to make sure we at least moved it into the lambda. The article I had read yesterday was https://www.codesynthesis.com/~boris/blog/2012/07/24/const-rvalue-references/ which seems to imply that const&& works to move a thing the same as non-const &&, but there's not many practical uses of it because you can use const& instead for most things. In this case though I wanted to make sure we did actually move it into the lambda, so I thought const& wouldn't be safe. And using const&& instead of non-const&& allowed omitting the const_cast, so that's what I originally went with.
Depends on: 1461122
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25) > Comment on attachment 8975223 [details] [diff] [review] > Bug 1452390 - Pass ScrollUpdatesMap around by const references as we never > try to copy it > > Review of attachment 8975223 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/apz/src/APZUpdater.cpp > @@ +241,5 @@ > > MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); > > RefPtr<APZUpdater> self = this; > > RunOnUpdaterThread(aOriginatingLayersId, NS_NewRunnableFunction( > > "APZUpdater::UpdateScrollOffsets", > > + [=,&aUpdates]() { > > This is the part I was unsure about. If we are just capturing aUpdates by > reference, doesn't that mean that by the time the lambda runs, the reference > is garbage because the caller stack has unwound and the ScrollUpdatesMap is > gone? That's why I wanted to make sure we at least moved it into the lambda. Shoot, you're right. I overlooked the fact that we're storing the lambda to be called later. Thanks for catching that! So, yes, we do in fact need to create a new ScrollUpdatesMap object (the copy stored in the lambda), and we should be careful to move rather than copy it into there, which makes the patch as landed do what we want. Apologies for the additional confusion. > The article I had read yesterday was > https://www.codesynthesis.com/~boris/blog/2012/07/24/const-rvalue-references/ > which seems to imply that const&& works to move a thing the same as > non-const &&, but there's not many practical uses of it because you can use > const& instead for most things. In this case though I wanted to make sure we > did actually move it into the lambda, so I thought const& wouldn't be safe. > And using const&& instead of non-const&& allowed omitting the const_cast, so > that's what I originally went with. I assume you're referring to this statement in the article: > Yes, we can have a function that returns a const value, like g() above, or > call std::move() on a const object, but all this is rather pointless. What the article isn't saying is that when you call move() on a const object, the result will have type const&&. (This is why, I assume, you were forced to make e.g. WebRenderBridgeParent::UpdateAPZScrollOffsets() in your r2 patch take a const&& to get things to compile.) The reason that's a problem is that when it comes time to creating the lambda: [updates=Move(aUpdates)] and |aUpdates| has type const&&, so will |Move(aUpdates)|, the move constructor of the type (std::map in this case) will not be a match (since binding a const&& argument to the move constructor's non-const && parameter would be casting away constness), and we fall back to the copy constructor.
Attachment #8975223 - Attachment is obsolete: true
Attachment #8975223 - Flags: review?(bugmail)
Some very nice perf improvements! \(^_^)/ == Change summary for alert #13188 (as of Fri, 11 May 2018 22:21:20 GMT) == Improvements: 89% tp5o_scroll linux64-qr opt e10s stylo 1.69 -> 0.18 87% tp5o_scroll windows10-64-qr opt e10s stylo1.23 -> 0.16 72% tscrollx linux64-qr opt e10s stylo 0.77 -> 0.22 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13188
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: