Improve heuristics for active layers

RESOLVED FIXED in mozilla28

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla28
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(7 attachments, 1 obsolete attachment)

1.15 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
33.89 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
7.29 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
1.70 KB, patch
Details | Diff | Splinter Review
3.25 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.56 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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.
Created attachment 798688 [details] [diff] [review]
Part 1: Update mutation count for first as well as subsequent mutations

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.)
Created attachment 798691 [details] [diff] [review]
Part 2: Create active layers when we have CSS transitions/animations on the relevant property, even if it's not being offloaded to the compositor

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)

Updated

5 years ago
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.
Flags: needinfo?(roc)
(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)
Created attachment 799419 [details] [diff] [review]
Part 2: Refactor MarkLayersActive code into its own class and be much more explicit about what it does

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)
Created 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
Attachment #799421 - Flags: review?(dbaron)
Created attachment 799427 [details] [diff] [review]
Part 4. Add API to detect whether an nsGlobalWindow is running a timeout handler
Attachment #799427 - Flags: review?(bzbarsky)
Created attachment 799429 [details] [diff] [review]
Part 5. Add API to detect whether an nsRefreshDriver is in the middle of a refresh
Attachment #799429 - Flags: review?
Attachment #799429 - Flags: review? → review?(dbaron)
Created 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
Attachment #799432 - Flags: review?(dbaron)
Created attachment 799437 [details] [diff] [review]
Part 7: A single change to CSS 'transform' should not be treated as animation
Attachment #799437 - Flags: 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+
Attachment #799437 - 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.

Updated

4 years ago
Duplicate of this bug: 932196

Updated

4 years ago
Duplicate of this bug: 932195

Updated

4 years ago
Duplicate of this bug: 931082
Depends on: 934301

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.