Closed Bug 1687378 Opened 4 years ago Closed 4 years ago

Lots of time spent allocating and freeing picture cache textures when zooming

Categories

(Core :: Graphics: WebRender, defect)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
86 Branch
Tracking Status
firefox86 --- fixed

People

(Reporter: jnicol, Assigned: jnicol)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Profile: https://share.firefox.dev/3nXjoNd

Taken on Samsung A5 2017, a mid-range phone with a Mali-T830 GPU. Test case is zooming in and out repeatedly on hacker news comments.

10% of the renderer thread is spent in create_texture and delete_texture. Adding logging, I can see this is all creating and destroying picture cache tiles. I believe this specific problem is a regression from bug 1682365, as it switched to using individual 1024x512 2d textures for each picture cache tile, rather than multiple tiles sharing a single texture array. We probably have always immediately deleted the texture when it becomes unused, but previously this occurred less frequently as multiple tiles would be sharing a texture.

In bug 1685261 we plan to switch from using individual 2d textures per tile to using a larger 2d texture atlas for multiple tiles. That would make the behaviour similar to before. We could also look in to not being quite so eager to free unused textures.

https://searchfox.org/mozilla-central/source/gfx/wr/webrender/src/texture_cache.rs#428 should in theory be managing the pool of picture cache tiles and recycling them. Maybe there's a bug there, or the GC is too aggressive in freeing targets.

That's orthogonal from using a larger 2d texture atlas for multiple tiles, which still makes sense.

The textures are indeed recycled in get_or_allocate_tile(). The problem appears to be that we're too eager in garbage collecting textures before they have a chance to be recycled: https://searchfox.org/mozilla-central/rev/c54c71ecbd9e64cafc0df3b596e206ac4072cb91/gfx/wr/webrender/src/texture_cache.rs#545

We allow ourselves to keep an extra 25% (rounded up) of the number of in-use textures around to be recycled. However, when I'm zooming in and out the number of in-use tiles varies between 11 and 20. When there's 11 in use we keep 3 unused around, which means we're freeing and re-allocating 6 each time we zoom in and out.

Oh, I have also just noticed that quite often we both allocate new textures and free old textures on the same frame. I guess this is because we call expire_old_picture_cache_tiles() at the end of the frame, after we have requested the allocations. Seems like this should be solvable by delaying the allocations until we know what can be freed. Will dig deeper in to that.

Moving expire_old_picture_cache_tiles() to begin_frame() rather than end_frame() seems to be the best solution to this. (But keep gc() in end_frame()). This gives us a chance to try to reuse the freed textures before they are gc'd. Patch incoming.

Currently we expire old picture cache tiles at the end of the frame,
immediately before garbage collecting them. This means that new
textures have already been allocated for newly-created picture cache
tiles, so we often end up both allocating and destroying textures in
the same frame.

Instead, move the call to expire_old_picture_cache_tiles() to the
beginning of the frame. Picture cache tiles added to the cache during
the frame can then recylce these textures rather than allocate new
ones. Garbage collection still occurs at the end of the frame,
destroying freed textures that were not recycled.

Note that expire_old_picture_cache_tiles() frees picture cache tiles
which were unused in the previous as well as the current frame. This
is a legacy from when the function freed all types of texture cache
entries, and could be called throughout the frame. Immediately prior
to this change, it could in fact have just checked for usage during
the current frame, as the function was only called at the end of the
frame. However, as this change moves the call to the beginning of the
frame, we do actually now need to check for usage during
the previous frame.

Assignee: nobody → jnicol
Status: NEW → ASSIGNED

This patch reduces the create_texture/delete_texture time from ~10% to 2.5%. It's probably still worth doing the rest of the things discussed in this bug, but this fixes the most of it

Pushed by jnicol@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4058cac4d85c Expire old picture cache tiles at the beginning of the frame. r=gw
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 86 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: