I've noticed this on the mobile version of the site http://hundredzeros.com/ It has a list of books with a "card" layout - each card has an image, title, author etc. Just after the page loads javascript sets the "left" and "top" properties of the cards. The program flow goes from nsDOMCSSAttributeDeclaration::SetPropertyValue() to ActiveLayerTracker::NotifyInlineStyleRuleModified(). Here, IsPresContextInScriptAnimationCallback() is true so we call NotifyAnimated(), which sets the mutationCount for the property immediately to 0xFF. On this site this makes us give each card its own layer causing us to OOM. The properties are actually being set to the values that they already have. Perhaps we should ignore it when this happens? Also the properties are only set once, so if we counted it as a restyle instead of an animation we would avoid this problem (as that increments the mutation count instead of setting immediately to 0xFF). Are we sure IsPresContextInScriptAnimationCallback() is a reliable way of telling if we're animating something instead of just setting a property once? Was there a strong reason to immediately set the counts to 0xFF, rather than wait for them to reach 2? I'd like to change that in order to fix this site.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #1) > Created attachment 8718096 [details] [diff] [review] > Don't force layerization of a frame just because the style is modified in a > refresh/timeout Probably a better description is "Don't assume a frame is animated just because the style is modified during refresh/timeout" Most style changes will occur during a refresh because we intentionally defer them to occur there just before paint. We should just rely on the mutation count to trigger layerization.

Patch from jnicol

On the page in question (hundredzeros.com) these patches reduced peak texture usage from 270MB down to 65MB in Fennec (Nexus 6P). I believe these sorts of issues are contributing heavily to the OOM issues we're facing on Fennec.

Comment on attachment 8718096 [details] [diff] [review] Don't force layerization of a frame just because the style is modified in a refresh/timeout Matt convinced me that we probably at least want to keep this behavior for rAF, otherwise we'll have something like 400ms of slow frames in cases where we are actually animating something. We need a pixel-based layer budget anyway to avoid other texture explosions, so Jamie is going to look down that path.

It's also worth noting that just the de-dupe patch fixes the problem for the test page, because the second set comes from a rAF callback (and not a style flush as I originally thought).

Comment on attachment 8718098 [details] [diff] [review] De-dupe changes before calling ActiveLayerTracker::NotifyInlineStyleRuleModified() Review of attachment 8718098 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsDOMCSSAttrDeclaration.cpp @@ +188,5 @@ > if (frame) { > + nsAutoString currentValue; > + GetPropertyValue(aPropID, currentValue); > + if (aValue != currentValue) { > + ActiveLayerTracker::NotifyInlineStyleRuleModified(frame, aPropID); Isn't this going to cause a perf regression, e.g. if JS does a tight loop setting element.style.opacity to various values? I think we might want to push this check down into ActiveLayerTracker so we only do it when it's actually going to stop something from being marked animated. I.e. if the thing is already animated (because of a previous ActiveLayerTracker notification we don't need to do this check again).

Is the expensive call the GetPropertyValue? So we want to push this into NotifyAnimated, and only if mutationCount != 0xFF? That would mean we need to pass the nsDOMCSSDeclaration* into ActivityTracker, or is there a way to get that from the frame?

(In reply to Jamie Nicol [:jnicol] from comment #8) > Is the expensive call the GetPropertyValue? Potentially. Maybe you should measure a testcase. Something that just does var s = element.style; for (i = 0; i < 1000000000; ++i) { s.opacity = "0.1"; } > So we want to push this into > NotifyAnimated, and only if mutationCount != 0xFF? Yes I think so > That would mean we need to pass the nsDOMCSSDeclaration* into > ActivityTracker, or is there a way to get that from the frame? Pass it in.

Move check in to ActiveLayerTracker::NotifyAnimated. Rather embarassingly I know no js or html, so getting this testcase is taking slightly longer than I'd like, but I'm working on it.

Okay, ran some tests, and you're right - checking in nsDOMCSSAttrDeclaration::SetPropertyValue would cause a slight performance regression so it's best to push the check back into ActiveLayerTracker. Requesting checkin of "De-dupe changes v2" please. Here's a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69cae76a19c6 I accidentally hit the cancel button with some android runs remaining, so here's another one with just the android runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=337c66e6439d

Jamie, can we uplift this? It is pretty low-risk and I wonder if it will have any impact on our OOMs on 45.

Comment on attachment 8718842 [details] [diff] [review] De-dupe changes v2 Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: Unnecessarily high memory usage, possibly crashes on Android [Describe test coverage new/current, TreeHerder]: Nightly, automated tests, etc [Risks and why]: Low, only de-dupes a CSS property change [String/UUID change made/needed]: None

Marking current branches as affected - these aren't recent regressions

Comment on attachment 8718842 [details] [diff] [review] De-dupe changes v2 Should decrease OOM crashes for android, been on nightly for a week. OK to uplift to aurora.

Comment on attachment 8718842 [details] [diff] [review] De-dupe changes v2 As discussed on IRC, too late for 45.