[TextureClientPool] Assess pool size values and adjust accordingly

RESOLVED FIXED

Status

()

Core
Graphics: Layers
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: gw280, Assigned: gw280, NeedInfo)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(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]
Created attachment 8775359 [details] [diff] [review]
0001-Bug-1289525-Set-the-default-TextureClientPool-increm.patch

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)

Updated

a year ago
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).
(Assignee)

Updated

a year ago
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.
Created attachment 8775846 [details] [diff] [review]
0001-Bug-1289525-Grow-the-pool-one-at-a-time-instead-of-i.patch
Attachment #8775846 - Flags: review?(jmuizelaar)
(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.
A try run that isn't broken: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ff638020f73&selectedJob=24741922
Created attachment 8776111 [details] [diff] [review]
0001-Bug-1289525-Shrink-the-TextureClientPool-to-maximum-.patch
Attachment #8776111 - Flags: review?(jmuizelaar)
Attachment #8775846 - Flags: review?(jmuizelaar) → review+

Comment 8

a year ago
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81aab5c5f3d0
Grow the pool one at a time instead of in chunk of increment size r=jrmuizel
(Assignee)

Updated

a year ago
Keywords: leave-open

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/81aab5c5f3d0
Created attachment 8777450 [details] [diff] [review]
0001-Bug-1289525-Shrink-the-TextureClientPool-to-maximum-.patch

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+

Comment 11

a year ago
Pushed by gwright@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36765ad4e9d4
Shrink the TextureClientPool to maximum size after a short delay, and clear the pool after a longer delay r=jrmuizel

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/36765ad4e9d4
I think we can resolve this now.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Keywords: leave-open
Comment on attachment 8777450 [details] [diff] [review]
0001-Bug-1289525-Shrink-the-TextureClientPool-to-maximum-.patch

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?

Updated

a year ago
status-firefox50: --- → affected
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)
Blocks: 1296206
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.
status-firefox51: --- → fixed
Comment on attachment 8777450 [details] [diff] [review]
0001-Bug-1289525-Shrink-the-TextureClientPool-to-maximum-.patch

Aurora50+
Attachment #8777450 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 21

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1d6e5e2864b
status-firefox50: affected → fixed
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) ==

Improvements:

  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
(Assignee)

Updated

a year ago
Duplicate of this bug: 1290303
You need to log in before you can comment on or make changes to this bug.