1531142 - Long pinch actions seem to continuously increase CPU (compositor) time

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

On Android, if you zoom in and out slightly for a long time with the webrender profiler HUD on, the CPU (compositor) graph shows the time increasing per frame as long as you continue the pinching action. Seems kind of odd, like we're accumulating some state or not freeing stuff until the user lets go of the pinch.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

http://bit.ly/2SvxGoD is an unsymbolicated profile. Not super useful but it shows the memory increasing throughout which is probably related.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

These profiles doen't seem to have Renderer thread. Also, do you know why they're unsymbolicated?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I forgot to add the Renderer thread. I knew I was forgetting one :) As for symbolication, not sure exactly but it was a local build with a mozconfig I had lying around so maybe didn't have the right options.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1511740 - Use nsDisplayAsyncZoom items to insert zooming animations in WR. r?jrmuizel,botond

The way we control APZ zooming in WebRender is by inserting an animation
property placeholder on a WR stacking context, and then having APZ
update the animation transform value with the proper matrix at composite
time.

Previously, the stacking context being used was the rootmost
stacking context in the content process. However this doesn't work for
zoomable content in the UI process (e.g. about:support), and after
recent changes, also wraps display items that should not be affected by
zoom (e.g. scrollbars or the background color item).

This patch moves the animation property placeholder so that it
corresponds to the newly added nsDisplayAsyncZoom display item, which
corrects both of the above problems and is conceptually in line with the
desired behaviour.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

https://perfht.ml/2SLH4EM is more symbolicated and has the Renderer thread. Clearly shows increasing composite times.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Now with Bobby's patches from bug 1532810: https://perfht.ml/2XHg5h9

copy_image_sub_data seems to be the winner

Comment 7

[Kartikaya Gupta (email:[e-mail])]

At least some of the problem seems to be the default eviction algorithm. We might need to tune it more for Android. Reducing the MAX_MEMORY_PRESSURE_BYTES seemed to improve the performance a bit.

Comment 8

[Jamie Nicol [:jnicol]]

The problem seems to be that pinch zooming fills up the texture cache incredibly quickly. I assume because we cache glyphs/images for every slight change in zoom level, compared to on desktop when we zoom at discrete increments.

So it grows very large, and whenever it is resized we need to perform a copy of the entire thing.

FWIW I also see this on my OnePlus 6, though not nearly as serverely.

Tweaking with the eviction algorithm definitely seems like it would help. We could also resize the cache in larger increments so that we don't have to copy as frequently. Also if we can delay resizing until we know exactly how large the cache needs to be that would mean worst case one copy per frame, but I don't know how feasible that is.

Comment 9

[Jamie Nicol [:jnicol]]

Actually, it looks like we already do that last thing.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

We should also probably not be trying to cache glyphs for every slight change in zoom level.

Comment 11

[Jamie Nicol [:jnicol]]

Yeah I think that would help significantly.

Just for a sense of scale, a few zooms in and out of planet.mozilla.org grows my texture cache to >200 layers

Comment 12

[Bobby Holley (:bholley)]

Yeah, it seems like the issue here is less about our eviction algorithms being miscalibrated for android and more that we need to handle continuous scaling downstream of the texture cache.

Comment 13

[Bobby Holley (:bholley)]

Also, we should make sure we're not kicking off lots of CPU glyph rasterization tasks for all the different zoom levels.

Comment 14

[Jamie Nicol [:jnicol]]

I have a WIP patch doing just that. It seems like the eviction policy might also need tweaked, but it's a huge improvement.

Comment 15

[Jamie Nicol [:jnicol]]

Attachment: Bug 1531142 - Avoid excessive glyph rasterization when pinch zooming. r?gw,kats

When pinch zooming webrender would re-rasterize glyphs for each tiny
difference in zoom level. This takes time in itself, but also causes
the texture cache to grow incredibly large, to the point where
resizing it to make room for more glyphs takes far too much time.

This patch avoids this by rounding the size at which glyphs are
rasterized whilst pinch zooming. To do this we add a FrameMsg which
APZ uses to tell webrender whether a spatial node is being pinch
zoomed. Then during frame building if a spatial node is being pinch
zoomed we override the raster space of its corresponding picture.

The chosen raster space is the current zoom level rounded up to the
nearest power of two, but not exceeding 8x. This seems to be a good
balance between quality and performance, though at high zoom levels
the cache still does grow very large due to the size of the glyphs.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/582dab79ef68
Avoid excessive glyph rasterization when pinch zooming. r=gw,kats

Comment 17

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/582dab79ef68