Closed Bug 1405359 Opened 7 years ago Closed 7 years ago

We do a push,push,push/pop,pop,pop dance for inner display items

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected

People

(Reporter: jrmuizel, Assigned: kats)

References

Details

(Whiteboard: [wr-mvp])

Attachments

(5 files)

Bug 1405359 - Stop passing around the clip id cache in all the functions.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1405359 - Replace the mPushed* variables with a more encapsulated struct.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1405359 - Make ScrollingLayersHelper a more stateful class.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1405359 - Avoid pushing and popping identical clip stacks for adjacent display items.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1405359 - Add some logging code in ScrollingLayersHelper.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
On a page like gmail with lots of display items inside clips/scrolls we do a bunch of work to push the appropriate stack every item.

The logs look like something like this:
Push(2)
Push(3)
PushItem()
Pop(3)
Pop(2)
Push(2)
Push(3)
PushItem()
Pop(3)
Pop(2)
Push(2)
Push(3)
PushItem()
Pop(3)
Pop(2)

Ideally we could do this more efficiently and only do the stack churn when the stack's changing.
My idea to do this has two parts: (1) is to modify the WR APIs for defining clips and scroll frames so that we can just provide the parent id instead of having to push the parent onto the stack beforehand, (2) roll the ScrollingLayersHelper and clip id cache into a new ClipConverter class or some such, that will be less RAII than ScrollingLayersHelper. This class can define clips as needed using the API from part (1) without having to push all the ancestor things onto the stack. It might also make it easier to use the local clip field on an item, if we decide we want to do that.
I'll pick this up once we have the new WR API in m-c.
Depends on: 1403971
See Also: → https://github.com/servo/webrender/pull/1796
Whiteboard: [wr-mvp] [triage]
This will also be a bit easier to do after we delete the layers-full code.
Depends on: 1403915
Priority: -- → P2
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
No longer depends on: 1403971
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Priority: P2 → P1
This is taking longer than I expected, but for stupid reasons. I'm still fairly happy with the overall architecture I had in mind.

WIP so far, there's some known issues with sticky positioning being broken and scrollbars not showing up in the right spot in some cases, but hopefully no other problems:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d8f0ebf42b64969806107e21c1f520d36c02745
Depends on: 1407213
No longer depends on: 1405399
Another option here is to combine the ClipAndScrollInfo into LayerPrimitiveInfo.
That could further reduce pushing and popping, yeah. However, doing that will still require the patchset I'm working on now as a prerequisite.
I'm going to split the work I have so far into bug 1409446 as it will reduce the pushes/pops to at most 2 (one scroll layer and one clip) per display item, but we can do better. My patch queue is already getting too large though so I want to land what I have now, and then continue working on this. The second set of patches which will go in this bug will make ScrollingLayersHelper more stateful (as opposed to RAII) so that when adjacent display items have the same ASR/clip chain, we can avoid popping stuff only to push the exact same things right back on.
That's much better.

Some ballpark numbers, based on running the async-scrolling reftest folder with the WRDL logging enabled and grepping the output for PushClip and PushScrollLayer calls:

- current m-c: 5154 pushes in 400 transactions (~12.8 pushes per transaction)
- with bug 1409446 only: 2766 pushes in 302 transactions (~9.2 pushes per transaction)
- with bug 1409446 plus this one: 1462 pushes in 297 transactions (~4.9 pushes per transaction)

I'm not sure why the number of transactions comes down so much but this is more or less in line with what I was expecting. On more complicated pages I expect the savings to be amplified; the async-scrolling reftests tend to have relatively few display items per page.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> That's much better.
> 
> Some ballpark numbers, based on running the async-scrolling reftest folder
> with the WRDL logging enabled and grepping the output for PushClip and
> PushScrollLayer calls:
...

Do we have an idea what non-WR looks like, if the comparisons make sense?
I don't think the comparisons really make sense, because in the non-WR case the clips get flattened onto the layers and we don't really Push/Pop them.
Comment on attachment 8920357 [details]
Bug 1405359 - Add some logging code in ScrollingLayersHelper.

https://reviewboard.mozilla.org/r/191348/#review197774
Attachment #8920357 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8920356 [details]
Bug 1405359 - Avoid pushing and popping identical clip stacks for adjacent display items.

https://reviewboard.mozilla.org/r/191346/#review197776
Attachment #8920356 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8920355 [details]
Bug 1405359 - Make ScrollingLayersHelper a more stateful class.

https://reviewboard.mozilla.org/r/191344/#review197778
Attachment #8920355 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8920354 [details]
Bug 1405359 - Replace the mPushed* variables with a more encapsulated struct.

https://reviewboard.mozilla.org/r/191342/#review197780
Attachment #8920354 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8920353 [details]
Bug 1405359 - Stop passing around the clip id cache in all the functions.

https://reviewboard.mozilla.org/r/191340/#review197782
Attachment #8920353 - Flags: review?(jmuizelaar) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb49e178390c
Stop passing around the clip id cache in all the functions. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/f6ce05f8e699
Replace the mPushed* variables with a more encapsulated struct. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/154c415885b8
Make ScrollingLayersHelper a more stateful class. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/988d6a397ea8
Avoid pushing and popping identical clip stacks for adjacent display items. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/1593dfc4cf04
Add some logging code in ScrollingLayersHelper. r=jrmuizel
Backed out for Windows build bustage in ScrollingLayersHelper.h:

https://hg.mozilla.org/integration/autoland/rev/262e8833748ea772ef008cf03ad3033d37943042
https://hg.mozilla.org/integration/autoland/rev/870e07ee24cf9af05c0e790afd2918ec6e63cf52
https://hg.mozilla.org/integration/autoland/rev/1d6b1bfb9e23e7acc6e73a6395619f3bfbce8948
https://hg.mozilla.org/integration/autoland/rev/79d7006caad1628cd502a789d87418b5c3972f26
https://hg.mozilla.org/integration/autoland/rev/06998a4853b71af2242b445b2b70fca259a52899

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1593dfc4cf04af2d32060262a5a0e77ff5b2688d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139299292&repo=autoland

21:33:01     INFO -  Unified_cpp_gfx_layers11.cpp
21:33:01     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/layers/ScrollingLayersHelper.h(74): error C2039: 'unordered_map': is not a member of 'std'
21:33:01     INFO -  z:\build\build\src\vs2015u3\VC\include\bitset(18): note: see declaration of 'std'
21:33:01     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/layers/ScrollingLayersHelper.h(74): error C2143: syntax error: missing ';' before '<'
21:33:01     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/layers/ScrollingLayersHelper.h(74): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
21:33:01     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/layers/ScrollingLayersHelper.h(74): error C2238: unexpected token(s) preceding ';'
21:33:01     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/layers/ScrollingLayersHelper.h(77): error C3646: 'mCache': unknown override specifier
21:33:01     INFO -  z:\build\build\src\obj-firefox\dist\include\mozilla/layers/ScrollingLayersHelper.h(77): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
...
Flags: needinfo?(bugmail)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1345767fb2d5
Stop passing around the clip id cache in all the functions. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/1921d545ea39
Replace the mPushed* variables with a more encapsulated struct. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/2cbf78901a3d
Make ScrollingLayersHelper a more stateful class. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/d3d823602aa1
Avoid pushing and popping identical clip stacks for adjacent display items. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/edc52ff233c4
Add some logging code in ScrollingLayersHelper. r=jrmuizel
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: