Closed
Bug 1289525
Opened 8 years ago
Closed 8 years ago
[TextureClientPool] Assess pool size values and adjust accordingly
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
People
(Reporter: gw280, Assigned: gw280, NeedInfo)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(3 files, 1 obsolete file)
1.84 KB,
patch
|
jnicol
:
review+
|
Details | Diff | Splinter Review |
8.35 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
8.27 KB,
patch
|
jrmuizel
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Whiteboard: [gfx-noted]
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
Assignee: nobody → gwright
Assignee | ||
Comment 2•8 years ago
|
||
(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).
Updated•8 years ago
|
Attachment #8775359 -
Flags: review?(jnicol) → review+
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8775846 -
Flags: review?(jmuizelaar)
Comment 5•8 years ago
|
||
(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.
Assignee | ||
Comment 6•8 years ago
|
||
A try run that isn't broken: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ff638020f73&selectedJob=24741922
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8776111 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8775846 -
Flags: review?(jmuizelaar) → review+
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•8 years ago
|
Keywords: leave-open
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/81aab5c5f3d0
Assignee | ||
Comment 10•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8777450 -
Flags: review?(jmuizelaar) → review+
Comment 11•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/36765ad4e9d4
Assignee | ||
Comment 13•8 years ago
|
||
I think we can resolve this now.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•8 years ago
|
Keywords: leave-open
Assignee | ||
Comment 14•8 years ago
|
||
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?
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)
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)
Assignee | ||
Comment 17•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/a1d6e5e2864b
Comment 22•8 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•