1468401 - motionmark_htmlsuite CSS_bouncing_filter_circles pgo regression

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Alex Thayer [:alexical] (she/her)]

Seeing a somewhat substantial regression[1] in the CSS_bouncing_filter_circles test[2] as a result of bug 1467619. Running the test with layers.draw-borders on, I notice that before the change, we just had a bunch of little textures, one for each circle. With 1467619, we combine some of the layers because mFilters changes[3], and what we get are occasionally some large rectangles closing in multiple circles. The end result is that we are painting and compositing a greater area because of the space in between the circles that we now have to "draw". So overall we're just doing more work. Looking into how we can mitigate this, but any thoughts from more experienced people are welcome.

[1] https://treeherder.mozilla.org/perf.html#/graphs?timerange=7776000&series=mozilla-central,1661664,1,1
[2] https://browserbench.org/MotionMark/developer.html?test-interval=15&display=minimal&tiles=big&controller=fixed&frame-rate=30&kalman-process-error=1&kalman-measurement-error=4&time-measurement=performance&suite-name=HTMLsuite&test-name=CSSbouncingfiltercircles&complexity=189
[3] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/layout/style/nsStyleStruct.cpp#4840

Comment 1

[Alex Thayer [:alexical] (she/her)]

(In reply to Doug Thayer [:dthayer] (PTO on June 4) from comment #0)
> The end result is that we are painting and compositing a greater area because of the
> space in between the circles that we now have to "draw".

Clarifying: we're compositing the same area, but the total size of all of the textures we have to composite is increased.

Comment 2

[Alex Thayer [:alexical] (she/her)]

Tentatively (I'm completely unfamiliar with this code), it looks like in ContainerState::ProcessDisplayItems, when finishing an inactive layer tree (here?[1]), we could get a measure of how sparse the rect is - maybe the rect's area compared to the area of all of the items inside it. If it's past a threshold, we would need to undo the work on it and process all of its children as though they were active layers (if undoing is not possible, we would need to look ahead in order to do this? Not sure yet how to accomplish this.)

Matt, does any of this sound at all reasonable?

[1] https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/layout/painting/FrameLayerBuilder.cpp#4809

Comment 3

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1468401 - Restrict ACTIVITY_TRIGGERED_REPAINT heuristic

See comment for explanation. This seemed like the most conservative
way to solve the motionmark regression from bug 1468401.

Review commit: https://reviewboard.mozilla.org/r/251022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251022/

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8985389 [details]
Bug 1468401 - Restrict ACTIVITY_TRIGGERED_REPAINT heuristic

https://reviewboard.mozilla.org/r/251022/#review257360

Hmm, bit sad, as this is somewhat unrelated to the actual problem that's causing the regression. Playing an unbounded game of whack-a-mole is pretty unappealing though, so just restricting the previous optimization is probably ok.

I do think getting the smaller layers pref enabled, and making sole-transforms active is valuable, but not a huge priority.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9f0b3d013f2
Restrict ACTIVITY_TRIGGERED_REPAINT heuristic r=mattwoodrow

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b9f0b3d013f2

Comment 7

[Alex Thayer [:alexical] (she/her)]

Looks like we kept the animometer win and dropped the htmlsuite regression, as intended:

https://treeherder.mozilla.org/perf.html#/graphs?series=mozilla-central,1661649,1,1&series=mozilla-central,1661659,1,1