Closed Bug 911889 Opened 11 years ago Closed 11 years ago

Improve heuristics for active layers

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28

People

(Reporter: roc, Assigned: roc)

References

(Depends on 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(7 files, 1 obsolete file)

Part 1: Update mutation count for first as well as subsequent mutations
1.15 KB, patch
BenWa
: review+
roc
: approval-mozilla-aurora-
Details | Diff | Splinter Review
Part 2: Refactor MarkLayersActive code into its own class and be much more explicit about what it does
33.89 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
Part 3: Create nsLayoutUtils::HasAnimations, and suppress opacity:0 optimizations whenever there's any opacity animation even if it's not using the compositor911889-animations
7.29 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Part 4. Add API to detect whether an nsGlobalWindow is running a timeout handler
1.70 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 5. Add API to detect whether an nsRefreshDriver is in the middle of a refresh
3.25 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Part 6: A scripted change to element.style.opacity or element.style.transform in a setTimeout or requestAnimationFrame callback should trigger our "style property is animated" heuristic
7.56 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Part 7: A single change to CSS 'transform' should not be treated as animation
1.12 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
Bug 894773's patch is a bit broken. It requires three property changes to trigger active layers, not two as intended. We should also consider making scripted property changes that run during a setTimeout or requestAnimationFrame immediately trigger active layers, on the grounds that they're likely to be part of an animation.
We should also make layers eagerly active when we have CSS animations/transitions present --- even if those transitions/animations are not being offloaded to the compositor.
Without this patch it takes three property changes to make the layer active: 1) Create the LayerActivity with mutation count 0. 2) Increment the mutation count to 1. (Still considered inactive.) 3) Increment the mutation count to 2. (Now considered active.)
This can improve performance and smoothness by making layers active as quickly as possible, before the animation starts. It also makes OMTC more consistent with non-OMTC behavior. Maybe it violates the CSS spec, but no more so than our existing OMTA code, and only in a barely observable way. I guess it can be observed by triggering a transition on 'transform' or 'opacity' and then using elementFromPoint to detect that a stacking context has already been created even though 'transform'/'opacity' still have their default values? What do you think?
Attachment #798691 - Flags: review?(dbaron)
Attachment #798688 - Flags: review?(bgirard) → review+
I'd probably be ok with one frame discontinuity. But does it handle animations that have an extended period where opacity is >=1 and therefore there should be no stacking context? (I haven't looked at the code yet.) This can happen with animation-delay or with cubic-bezier() timing functions that have y1 or y2 outside of [0, 1], or with step timing functions.
(In reply to David Baron [:dbaron] (needinfo? me; away Aug 28 - Sep 3) from comment #4) > I'd probably be ok with one frame discontinuity. But does it handle > animations that have an extended period where opacity is >=1 and therefore > there should be no stacking context? (I haven't looked at the code yet.) > This can happen with animation-delay or with cubic-bezier() timing functions > that have y1 or y2 outside of [0, 1], or with step timing functions. No, it doesn't. Neither does our off-main-thread-animation code, I believe. But I'm going to back off on this patch for now.
Flags: needinfo?(roc)
Attachment #798691 - Attachment is obsolete: true
Attachment #798691 - Flags: review?(dbaron)
The MarkLayersActive code was getting somewhat out of control, doing several things at once, so I've refactored it with much more precise API.
Attachment #799419 - Flags: review?(matt.woodrow)
Attachment #799429 - Flags: review? → review?(dbaron)
Comment on attachment 799427 [details] [diff] [review] Part 4. Add API to detect whether an nsGlobalWindow is running a timeout handler This seems OK, but do we want to detect all timeouts or just "nested" timeouts? Timeouts keep track of a nesting level, so we could detect just the latter if we want to... That would fail on cases like "rAF callback calls setTimeout which does all the real work", but people shouldn't be doing that anyway.
Attachment #799427 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #15) > This seems OK, but do we want to detect all timeouts or just "nested" > timeouts? Timeouts keep track of a nesting level, so we could detect just > the latter if we want to... That would fail on cases like "rAF callback > calls setTimeout which does all the real work", but people shouldn't be > doing that anyway. I want to detect all timeouts. A nested timeout would typically correspond the second frame of an animation, at which point we'll have seen two style changes and guess that we're in an animation anyway. The goal here is to guess when a style change is the first frame of an animation. Treating every style.opacity or style.transform change in a timeout handler as the first frame of an animation may be overkill, but I think it's worth trying. (And until recently we basically treated every style change as potentially the first frame of an animation, so this change is just backing off bug 894773 a bit.)
Comment on attachment 798688 [details] [diff] [review] Part 1: Update mutation count for first as well as subsequent mutations [Approval Request Comment] Bug caused by (feature/regressing bug #): 894773 User impact if declined: Unnecessarily sluggish starting of jQuery animations Testing completed (on m-c, etc.): none Risk to taking this patch (and alternatives if risky): low risk. It's small simple change. String or IDL/UUID changes made by this patch: none
Attachment #798688 - Flags: approval-mozilla-aurora?
Comment on attachment 798688 [details] [diff] [review] Part 1: Update mutation count for first as well as subsequent mutations Review of attachment 798688 [details] [diff] [review]: ----------------------------------------------------------------- Never mind, we decided to back 894773 out of Aurora instead.
Attachment #798688 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Attachment #799419 - Flags: review?(matt.woodrow) → review+
Comment on attachment 799421 [details] [diff] [review] Part 3: Create nsLayoutUtils::HasAnimations, and suppress opacity:0 optimizations whenever there's any opacity animation even if it's not using the compositor911889-animations r=dbaron though I think nsLayoutUtils::HasAnimationsForCompositor and nsLayoutUtils::HasAnimations would be clearer if they ended with: return has-animations || has-transitions; rather than: if (has-animations) { return true; } return has-transitions; (where "has-animations" and "has-transitions" stand for the function calls computing that). Feel free to fix that while you're there.
Attachment #799421 - Flags: review?(dbaron) → review+
Comment on attachment 799429 [details] [diff] [review] Part 5. Add API to detect whether an nsRefreshDriver is in the middle of a refresh r=dbaron if you assert that it's the expected value before setting it (for both sets). (Perhaps even better would be use of an RAII class like the AutoToggle patch sitting in bug 518756, since that would be exception-safe too. I did promise once to review for exception-safety when people were advocating use of C++ exceptions.)
Attachment #799429 - Flags: review?(dbaron) → review+
Comment on attachment 799432 [details] [diff] [review] Part 6: A scripted change to element.style.opacity or element.style.transform in a setTimeout or requestAnimationFrame callback should trigger our "style property is animated" heuristic > ActiveLayerTracker::NotifyAnimated(nsIFrame* aFrame, nsCSSProperty aProperty) > { > LayerActivity* layerActivity = GetLayerActivityForUpdate(aFrame); > uint8_t& mutationCount = layerActivity->RestyleCountForProperty(aProperty); > // We know this is animated, so just hack the mutation count. > mutationCount = 0xFF; > } >+/* static */ void >+ActiveLayerTracker::NotifyInlineStyleRuleModified(nsIFrame* aFrame, >+ nsCSSProperty aProperty) >+{ >+ if (!IsPresContextInScriptAnimationCallback(aFrame->PresContext())) { >+ return; >+ } >+ LayerActivity* layerActivity = GetLayerActivityForUpdate(aFrame); >+ uint8_t& mutationCount = layerActivity->RestyleCountForProperty(aProperty); >+ // Assume this is animated right away, so just hack the mutation count. >+ mutationCount = 0xFF; >+} Maybe call NotifyAnimated here rather than copying it into the last three lines of NotifyInlineStyleRuleModified? r=dbaron
Attachment #799432 - Flags: review?(dbaron) → review+
Comment on attachment 799419 [details] [diff] [review] Part 2: Refactor MarkLayersActive code into its own class and be much more explicit about what it does Review of attachment 799419 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/RestyleManager.cpp @@ +246,5 @@ > needInvalidatingPaint = true; > nsIFrame* childFrame = > GetFrameForChildrenOnlyTransformHint(aFrame)->GetFirstPrincipalChild(); > for ( ; childFrame; childFrame = childFrame->GetNextSibling()) { > + ActiveLayerTracker::NotifyRestyle(aFrame, eCSSProperty_transform); Matt points out that this should be childFrame, not aFrame.
With that fixed, plus an uninitialized mContentActive fixed, part 2 is green on mochitest-other finally. I'm still not sure what the problem was.
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/cd94525c17a4 - you may have been green in Moth, but you were rather orange in mochitest-5, racking up 77 unexpected "Not yet in refresh: '!mInRefresh'" assertions in test_transitions_per_property.html in https://tbpl.mozilla.org/php/getParsedLog.php?id=29821142&tree=Mozilla-Inbound
Not entirely green in Moth either, since https://tbpl.mozilla.org/php/getParsedLog.php?id=29821668&full=1&branch=mozilla-inbound is test_mousecapture.xul being surprised by your assertions.
Depends on: 934301
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: