[TextureClientPool] Assess pool size values and adjust accordingly




3 years ago
3 years ago


(Reporter: gw280, Assigned: gw280, NeedInfo)


Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)


(Whiteboard: [gfx-noted])


(3 attachments, 1 obsolete attachment)

Currently, the allocation strategy for TextureClientPool is to seed a pool with an initial number of tiles (currently 50, but preffable), and if we exhaust that initial supply to grow the pool by an increment number (10, preffable) and keep doing that until the need is satisfied.

This is a trade off between memory usage and allocation times, as it means that we can have up to 9 unused tiles sitting around waiting to be used. We do shrink the pool if we have >10 unused tiles, and we also completely clear the pool if we hit a memory pressure situation, so it's not really that bad.

I chose these values as an initial "sane" set but they could probably do with some tweaking to fine tune the balance here.
Whiteboard: [gfx-noted]
Seeing as the allocation cost on desktop appears to be insignificant, let's push this back down to 1 (revert to the old behaviour) and keep it at 10 for mobile, as per your suggestion.
Attachment #8775359 - Flags: review?(jnicol)
Assignee: nobody → gwright
(This still seeds the pool with the initial tile set of 50 though, which wasn't the behaviour before, so for very limited cases we will still see a memory usage increase).
Blocks: 1288994
Attachment #8775359 - Flags: review?(jnicol) → review+
Jeff reckons that he can see the effects of allocations as detrimental to scrolling, so going to investigate this further. Probably won't land this patch.
(In reply to George Wright (:gw280) (:gwright) from comment #4)
> Created attachment 8775846 [details] [diff] [review]
> 0001-Bug-1289525-Grow-the-pool-one-at-a-time-instead-of-i.patch

FWIW, patches with a bunch of renames are easier to review on reviewboard.
Attachment #8775846 - Flags: review?(jmuizelaar) → review+
Pushed by gwright@mozilla.com:
Grow the pool one at a time instead of in chunk of increment size r=jrmuizel
Keywords: leave-open
Updated version of the shrink timeout patch. This re-adds the clear-after-timeout functionality (which is more appropriately named than it was before) so that we don't hold onto textures for a long time if we don't really need them.
Attachment #8776111 - Attachment is obsolete: true
Attachment #8776111 - Flags: review?(jmuizelaar)
Attachment #8777450 - Flags: review?(jmuizelaar)
Attachment #8777450 - Flags: review?(jmuizelaar) → review+
Pushed by gwright@mozilla.com:
Shrink the TextureClientPool to maximum size after a short delay, and clear the pool after a longer delay r=jrmuizel
I think we can resolve this now.
Closed: 3 years ago
Resolution: --- → FIXED
Keywords: leave-open
Comment on attachment 8777450 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: bug 1289525, bug 1290303
[User impact if declined]: choppy scrolling on sites with hefty layerisation
[Describe test coverage new/current, TreeHerder]: mozilla-central runs, been on nightly for a week now.
[Risks and why]: low risk, just changes some semantics around releasing and recycling textures.
[String/UUID change made/needed]: none
Attachment #8777450 - Flags: approval-mozilla-aurora?
Hi Milan, Jeff, could either of you help me decide whether this is something that can ride the trains to 51 or ought to be uplifted to Aurora50? I need a second opinion. Thanks!
Flags: needinfo?(milan)
Flags: needinfo?(jmuizelaar)
We didn't quite manage the bug numbers properly here.  This bug has two separate changes in it - the first set landed in July and rode 50.  This is a follow up to fix a regression (bug 1290303) introduced by that first patch, and now fixed by the follow up patch in this bug.  If this second patch is to ride 51, we should back out the original patch(es) from 50.
Flags: needinfo?(milan)
Bug 1290303 isn't a regression (it's really slow on Fx 47 on my MBP, which pre-dates my changes from this bug). I think we've always been slow on that. This patch is mostly independent from the other one in that the other one is fine on its own, but this one makes things a bit better for scrolling.
Thanks Milan and George. Since 50 is still in the Aurora cycle and the patch is not super complicated, I'll approve the uplift.
Fixed in 51, based on comment 12.
Comment on attachment 8777450 [details] [diff] [review]

Attachment #8777450 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We got a nice win from this on the AWSY test on Android:

== Change summary for alert #2359 (as of August 09 2016 13:03 UTC) ==


  4%  Resident Memory summary android-4-3-armv7-api15 opt     242652881.96 -> 232210734

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=2359
Duplicate of this bug: 1290303
You need to log in before you can comment on or make changes to this bug.