Try harder to enforce layers.max-active in FrameLayerBuilder

RESOLVED FIXED in Firefox 45

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jnicol, Assigned: jnicol)

Tracking

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox45 fixed, fennec-)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
As noted here - https://bugzilla.mozilla.org/show_bug.cgi?id=1164027#c32 - the website http://www.kansascity.com/news/state/missouri/article44912667.html causes incredibly high memory usage. This is because it has hundreds of small animated items and we create a painted layer for each of them.

We should implement a budgeting system for this, like we do for will-change. In fact we could repurpose the will-change budget into a generic layerisation budget.
Jamie, any update on this? Do we suspect that many instances of bug 1164027 are due to a layerization explosion?
Flags: needinfo?(jnicol)
tracking-fennec: --- → ?
(Assignee)

Comment 2

2 years ago
I have a working patch but it needs some tweaking.

I only know of the one site for sure where this is responsible, but suspect/hope there will be more. I need to find more urls which reproduce 1164027 and see why they're causing it.
Flags: needinfo?(jnicol)
(Assignee)

Comment 3

2 years ago
Created attachment 8704077 [details] [diff] [review]
Patch v1

I spoke to Benoit about this on irc before the holidays and it seemed like using the will-change budget wasn't a great idea.

Instead I've tried to make ContainerState::ProcessDisplayItems() respect layers.max-active on the branch which calls PaintedLayerDataTree::FindPaintedLayerFor(), as well as the one which calls PaintedLayerDataTree::AddingOwnLayer().

We might also want to consider reducing the value of layers.max-active. I doubt we'd ever manage anywhere near 20 display ports of tiles before OOMing.
Attachment #8704077 - Flags: review?(mstange)
(Assignee)

Comment 4

2 years ago
> I doubt we'd ever manage anywhere near 20 display ports of tiles before OOMing.

But of course not all layers are that large.

Also, to add to above. I don't think the above patch entirely prevents us going over the layer limit. It prevents us adding a new node to the tree, but we could still add new datas to the stack of an existing node. Which is certainly a significant improvement on before.
Comment on attachment 8704077 [details] [diff] [review]
Patch v1

Review of attachment 8704077 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: layout/base/FrameLayerBuilder.cpp
@@ +1008,5 @@
> +   * aAnimatedGeometryRoot which already exists in the tree.
> +   */
> +  PaintedLayerDataNode* FindNodeForNearestAncestor(AnimatedGeometryRoot* aAnimatedGeometryRoot);
> +
> +

Only one new line needed here :-)

@@ +2953,5 @@
> +    }
> +  } else {
> +    return nullptr;
> +  }
> +}

Mozilla style says "Don't put an else right after a return."
Attachment #8704077 - Flags: review?(mstange) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8705168 [details] [diff] [review]
Patch v2

Fix style issues
(Assignee)

Updated

2 years ago
Attachment #8704077 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
Requesting checkin of patch v2 please.

here's the try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=70499852bec0
Keywords: checkin-needed
(Assignee)

Comment 9

2 years ago
Adding leave-open because we *possibly* might want to uplift it somewhere. I'd be hesitant to because FrameLayerBuilder scares me, and I'm also not confident about it improving memory usage too much. But it's worth waiting to see what the impact is.
Keywords: leave-open
Unless we can verify that this can for sure reduce memory usage (in common cases), I don't think we want to uplift.
tracking-fennec: ? → -
(Assignee)

Comment 12

2 years ago
Comment on attachment 8705168 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: The code this fixed never worked, but was only noticed as number of OOM crashes increased.
[User impact if declined]: More OOM crashes
[Describe test coverage new/current, TreeHerder]: Been on nightly for weeks without trouble
[Risks and why]: There is some unknown on how it will affect layerisation and therefore memory usage. From my testing it only makes a positive change. And it will only affect sites that are already horrendous and causing us severe memory issues.
[String/UUID change made/needed]: None

This probably won't have a massive impact by itself, but we might want to also reduce layers.max-active and this is required for that to have any effect.
Attachment #8705168 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Summary: Budget layerisation of items with animated offsets/margins → Try harder to enforce layers.max-active in FrameLayerBuilder
(Assignee)

Updated

2 years ago
See Also: → bug 1247554
Comment on attachment 8705168 [details] [diff] [review]
Patch v2

Hopefully fix the OOM, taking it.
Should be in 45 beta 5
Attachment #8705168 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8705168 [details] [diff] [review]
Patch v2

Sorry, reverting, cf comment #11
Attachment #8705168 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
Sylvestre, I think we actually do want this on Beta. We're (Jamie) is working on a better solution, but this might be a good stop-gap. Please apply it once again :)
Attachment #8705168 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Keywords: leave-open
Comment on attachment 8705168 [details] [diff] [review]
Patch v2

ok, let's take it then.
Attachment #8705168 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
has problems uplifting to beta:

grafting 322631:4bc445568814 "Bug 1231818 - Make FrameLayerBuilder try harder to respect layers.max-active pref. r=mstange"
merging layout/base/FrameLayerBuilder.cpp
warning: conflicts while merging layout/base/FrameLayerBuilder.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
Flags: needinfo?(jnicol)
(Assignee)

Comment 18

2 years ago
Created attachment 8719558 [details] [diff] [review]
patch for beta

There'd been a few changes to the function signature so it needed a manual rebase. This should work fine.
(Assignee)

Updated

2 years ago
Flags: needinfo?(jnicol)
 https://hg.mozilla.org/releases/mozilla-beta/rev/e0341a41fceb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.