Closed Bug 1237454 Opened 8 years ago Closed 6 years ago

[Power] Optimize animations in visibility:hidden elements

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Performance Impact ?
Tracking Status
firefox60 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug, )

Details

(Keywords: perf, power, Whiteboard: [Power:P1][platform])

Attachments

(9 files, 1 obsolete file)

2.79 KB, text/html
Details
59 bytes, text/x-review-board-request
emilio
: review+
Details
59 bytes, text/x-review-board-request
boris
: review+
Details
59 bytes, text/x-review-board-request
boris
: review+
Details
59 bytes, text/x-review-board-request
boris
: review+
Details
59 bytes, text/x-review-board-request
boris
: review+
Details
59 bytes, text/x-review-board-request
boris
: review+
birtles
: review+
emilio
: review+
Details
3.33 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
birtles
: review+
Details
+++ This bug was initially created as a clone of Bug #1166500 +++

We are not going to handle animations on/inside visibility:hidden elements.
This bug aims to optimize those animations.  I believe we can optimize at least animations on child elements which are inside visibility:hidden elements.
This seems doable, for animations on CSS properties that don't influence layout.

I don't think we could optimize away animations on CSS properties that *do* influence layout, though (e.g. "width").
No longer blocks: 1219236
(In reply to Daniel Holbert [:dholbert] (largely AFK until June 28) from comment #1)
> This seems doable, for animations on CSS properties that don't influence
> layout.
> 
> I don't think we could optimize away animations on CSS properties that *do*
> influence layout, though (e.g. "width").

We have already KeyframeEffectReadOnly::CanIgnoreIfNotVisible() to check whether animation influences layout or not.

A missing part of this bug is a function to check whether all descendant elements are visibility:hidden or not.

A possible solution I can think of is:

1. Add a flag in nsStyleContext to represent that all descendants are visibility:hidden.
2. Set the flag true initially.
3. In nsStyleContext::ApplyStyleFixups(), check StyleVisibility()->IsVisible(), if it's true, then the flags of all ancestors are set to false.
4. Add a function to return the flag in nsStyleContext.
5. Throttle animations if the flag is true and CanIgnoreIfNotVisible() is true.

Cameron, does this solution sound feasible?
Flags: needinfo?(cam)
Blocks: 1218169
For reference I am attaching a patch that I wrote before.

I am not sure this patch can be applied on current trunk.
See Also: → 1315465
Whiteboard: [Power:P1][platform] → [Power:P1][platform][qf:investigate]
Keywords: perf
Priority: -- → P2
See Also: → 1405744
No longer blocks: 1190721
Should I open a different issue for this one.
https://webcompat.com/issues/12827

the dots are out of sight. Animation is constantly running making 3 dots varying from scale(0) to scale(1) asynchronously. CPU goes from 2% to 50% maintained.
That sounds like a different bug since it's not about visibility:hidden (and bug 1242969 might be related).
Thanks for checking.  I don't have an account for quora.com so I can't confirm the animation by myself for now.
If the reason for the out of sight is that the animating element is scrolled out from the view, it will be fixed by bug 1190721.  We haven't yet optimized transform animations in scrolled out elements.
If the reason is that the animating element is visibility:hidden ancestor, this bug is for it.

Oops, collision.  Yep, if the animation is out of sight for other reason, we might need other optimization there like bug 1242969.
Comment on attachment 8777151 [details] [diff] [review]
Throttle animations in elements that all descendants of the element have visivility:hidden

I've been thinking about this for stylo.  In stylo, as far as I know, while styling an element, we can't mutate any ancestors for the element (i.e. fixing up the ancestors which are visibility:hidden) when we notice the element is not visibility:hidden in the visibility:hidden subtree.  A way I can think of using a sequential task.  I am not sure we have other efficient ways.
Attachment #8777151 - Attachment is obsolete: true
I was just testing this out on the Quantum beta on my Macbook, here are some rough results using the attached test page.

The test page shows some typical examples of the "loading spinners" used on virtually every modern website, which are usually kept in a "visibility: hidden" element when not being displayed. There are often several of them on one page, which can quickly add up to 100% CPU usage for tiny hidden animations, at least on a Mac.

I didn't see any significant difference in CPU usage between Firefox 52 ESR, FF 56.0.2, and FF 57.0b14 with or without Servo enabled. The following numbers are roughly as reported by the CPU meter of iStat Menus; 100% is one core. I also included the values for the windowserver process:

CPU usage, on my 2012 MacBook Air with macOS 10.12, when animation is:

displaying: 19% + windowserver 16%
hidden: 32% + windowserver 4%
scrolled away: 21% + windowserver 14%
hidden & scrolled away: 32% + windowserver 4%
none: < 1% + windowserver 2%

As you can see, Firefox's CPU is actually about 50% higher when they're hidden! Also the usage when scrolled offscreen is considerably higher than I would expect from Bug #1166500, and furthermore that fix seems to have no effect if the animation is visibility: hidden.

As a comparison, the results for Safari are:

displaying: 7% + windowserver 16%
hidden: 8% + windowserver 8%
scrolled away : 6% + windowserver 8%
hidden & scrolled: same as scrolled
none: < 1% + windowserver 2%

Interestingly, Safari's CPU usage is about the same when the animation is displaying, hidden, or scrolled offscreen; with only an 8% increase in the windowserver process when actually displaying. The main difference, aside from not having FF's large CPU increase when hidden, is that it's just overall more efficient, about 1/3 to 1/2 of Firefox's usage.

At first I tried to track down and block hidden animations with uBlock, but it was a losing battle. Because of the widespread use of these hidden "spinner" animations, and the excessive CPU used on the Mac, to save battery power I've disabled (restricted to one loop iteration) all infinite CSS animations with this greasemonkey script:

// ==UserScript==
// @name        Stop CSS Infinite Animations
// @namespace   Elhem.Enohpi
// @include     *
// @version     1.0
// @grant       GM_addStyle
// ==/UserScript==

// Make all CSS animations run only once:
GM_addStyle("*, *::before, *::after {animation-iteration-count: 1 !important}");
Sorry, the attachment above doesn't seem to work by clicking on it, try "save link as" and opening as a file...
I just got FF58 on the developer channel. There seems to be a significant improvement, I'm no longer seeing the large increase in CPU when the animation is hidden. It's now the same or slightly less than when it's being displayed. It's still about double what Safari and Vivaldi use when hidden:

FF58.0b1
displaying: 21% + windowserver 16%
hidden: 19% + windowserver 4%
scrolled away: 19% + windowserver 7%
hidden & scrolled: same as hidden
none: < 1% + windowserver 2%

Vivaldi 1.12.955.48:
displaying: 27% + windowserver 16%
hidden: 9% + windowserver 2%
scrolled away: same as hidden
hidden & scrolled: same as hidden
none: < 1% + windowserver 2%
I came up with another idea instead of using sequential task.  The idea is just adding a new change hint for visibility property change.  I think it's much simpler.  Unfortunately there is no room for the new change hint in current nsChangeHint (uint32_t), so we need to change nsChangeHint to uint64_t.  Cameron told me on IRC that 'it would be OK if you need to'.  (I think it will be changed sooner or later)

Also note that FWIW, chrome does check whether all descendants are invisible or not [1]

[1] https://chromium.googlesource.com/chromium/src.git/+/34720eee6bf1fb2da346421b26cadb16052d113a/third_party/WebKit/Source/core/paint/PaintLayer.cpp#699
See Also: → 1427033
I was thinking that to optimize this properly we need to store visible descendants count on each element and decrease/increase it when a descendant gets visible/invisible respectively.  I meant I was thinking we can't avoid overkills without counting.  But after some thoughts and writing test cases, I think we don't overkill without counting.  So I'd take the approach which doesn't count visible descendants here in this bug , and later in other bug fix completely.

FWIW, here is a test case that can't be optimized by the approach what I am trying to do here;

  div.style.visibility = 'hidden'; // the 'div' has no children at this moment
  div.animate(..);
  // The animation is throttled

  // in later frame
  div.appendChild(visibleChild);
  // The animation isn't throttled

  // in later frame
  visibleChild.style.visibility = 'hidden';
  // Now the animation should throttle again, but actually it's not.

To optimize this without counting visible descendants, we need to check all siblings of the |visibleChild| have no visible children.  If the visiblity:hidden element is the grandparent, we need to walk up ancestors and check all siblings there.  It's horrible. :/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbf16465620a8b9e1036901227bfe29a81381d3d

I could confirm that the Google issue (bug 1218169) will be solved partially without counting visible descendants,  'partially' I meant is that some opacity animations on the Google search results don't specify 100% keyframe value so that we can't throttle the opacity animations due to bug 1419079.  So I'd like to land the fix without counting visible descendants here in this bug, and will fix other issues one by one.
FWIW, here is a patch to throttle visibility:hidden animations completely (without counting visible descendants).
Assignee: nobody → hikezoe
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review218724

I think I understand this but I'll wait for you to check it over once more and explain a little more about the operation.

Until then I just have some minor grammar/naming feedback.

::: commit-message-92e0f:3
(Diff revision 1)
> +This patch does basically throttle animations on visibility:hidden element
> +and unthrottle it once the animating element became visible or a child of the
> +animating element became visible.  But still there are some cases that we don't
> +throttle such animations perfectly.  For example;

Just to check I've understood the meaning of this comment, could we write it as:

"This patch throttles most animations on visibility:hidden elements where all descendant elements are also hidden. It unthrottles them once the animated element, or one of its descendants, becomes visible.

There are some cases that it doesn't throttle, however. For example:"

?

::: commit-message-92e0f:16
(Diff revision 1)
> +
> +  div.appendChild(visibleChild);
> +  // The animation isn't throttled
> +
> +  visibleChild.style.visibility = 'hidden';
> +  // Now the animation should throttle again, but actually it's not.

(Nit: s/should throttle/should be throttled/)

::: commit-message-92e0f:19
(Diff revision 1)
> +
> +  visibleChild.style.visibility = 'hidden';
> +  // Now the animation should throttle again, but actually it's not.
> +
> +To throttle this case properly, when the |visibleChild|'s visibility changed
> +to hidden, we need to do either

Nit: s/we need to do either/we would need to either/

(Just to make it clear that this is describing something we *don't* do in this patch.)

::: layout/base/RestyleManager.cpp:1724
(Diff revision 1)
> +
> +      if (hint & nsChangeHint_VisibilityChange) {
> +        frame->UpdateVisibleDescendantsState();
> +      }

(Style nit: The other change hint checks here don't include a blank line between them. I think we can drop the blank line here.)

::: layout/generic/nsFrame.cpp:735
(Diff revision 1)
>    DidSetStyleContext(nullptr);
>  
>    if (::IsXULBoxWrapped(this))
>      ::InitBoxMetrics(this, false);
> +
> +  // For newly created frame, we need to update this frame's visibility state.

(Nit: For a newly created...)

::: layout/generic/nsFrame.cpp:736
(Diff revision 1)
>  
>    if (::IsXULBoxWrapped(this))
>      ::InitBoxMetrics(this, false);
> +
> +  // For newly created frame, we need to update this frame's visibility state.
> +  // Usually we update the state when the frame is restyled and has

(Nit: ... is restyled and has a VisibilityChange...)

::: layout/generic/nsFrame.cpp:11331
(Diff revision 1)
> +static bool
> +HasNoVisibleDescendants(const nsIFrame* aFrame)
> +{
> +  for (nsIFrame::ChildListIterator lists(aFrame);
> +       !lists.IsDone();
> +       lists.Next()) {
> +    for (nsIFrame* f : lists.CurrentList()) {
> +      if (f->IsVisibleOrMayHaveVisibleDescendants()) {
> +        return false;
> +      }
> +    }
> +  }
> +  return true;
> +}
> +
> +void
> +nsIFrame::UpdateVisibleDescendantsState()
> +{
> +  if (StyleVisibility()->IsVisible()) {
> +    // Notify to invisible ancestors that a visible descendant exists now.
> +    nsIFrame* ancestor;
> +    for (ancestor = GetFlattenedTreeParentPrimaryFrame();
> +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> +         ancestor = ancestor->GetFlattenedTreeParentPrimaryFrame()) {
> +      ancestor->mHasNoVisibleDescendants = false;
> +    }
> +  } else {
> +    mHasNoVisibleDescendants = HasNoVisibleDescendants(this);
> +  }
> +}

I had trouble following this logic at first. I think the two problems were:

1. the naming mHasNoVisibleDescendants (see comment below)
2. I didn't understand that all descendants will get the relevant change hint and run this in turn.

I'm a bit concerned that if we make a large subtree visibility:hidden, that the result of processing all the change hints on descendants will mean we end up traversing the entire subtree. Is that the case?

I think you said you were going to check this code over once more. While you're at it, could you explain the steps that take place when we set visibility:hidden on frame B in the graph below:

      A
     / \
    B   C
   / \
  D   E
 /
F

::: layout/generic/nsFrame.cpp:11350
(Diff revision 1)
> +
> +void
> +nsIFrame::UpdateVisibleDescendantsState()
> +{
> +  if (StyleVisibility()->IsVisible()) {
> +    // Notify to invisible ancestors that a visible descendant exists now.

(Nit: Notify invisible...)

::: layout/generic/nsIFrame.h:4360
(Diff revision 1)
> +  /**
> +   * True if this frame has no visible descendants.
> +   */
> +  bool mHasNoVisibleDescendants : 1;

As discussed on IRC, the negative in this name makes this code a little hard to follow.

One suggestion is mAllDescendantsAreInvisible ?

And a comment such as:

/**
 * True if we are certain that all descendants are not visible.
 *
 * This flag is conservative in that it might sometimes be false
 * even if, in fact, all descendants are invisible.
 */
Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

https://reviewboard.mozilla.org/r/213034/#review218744

::: dom/animation/test/mozilla/file_restyles.html:641
(Diff revision 1)
>         'view by resizing');
>  
>      await ensureElementRemoval(parentElement);
>    });
>  
> -  add_task(async function no_restyling_main_thread_animations_in_visiblily_hidden_element() {
> +  add_task(async function restyling_animations_on_visiblily_hidden_element() {

nit:
I guess we should mention the "change" of visibility: e.g.
`restyling_animations_on_visiblily_hidden_element_which_gets_visible()`.

This might be better, I think.

::: dom/animation/test/mozilla/file_restyles.html:658
(Diff revision 1)
> +       'Animations running that was on visibility hidden element should ' +
> +       'not throttle restyling any more');

nit:

Also here. It's better to mention: this is a "visibility hidden element which gets visible"

Otherwise, the description might be misread.
Attachment #8942790 - Flags: review?(boris.chiou) → review+
Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

https://reviewboard.mozilla.org/r/213036/#review218746

::: dom/animation/test/mozilla/file_restyles.html:664
(Diff revision 1)
>         'not throttle restyling any more');
>  
>      await ensureElementRemoval(div);
>    });
>  
> +  add_task(async function restyling_animations_in_visiblily_hidden_parent() {

nit:

`restyling_animations_in_visiblily_changed_parent` or others which mention we change the visibility might be better.
Attachment #8942791 - Flags: review?(boris.chiou) → review+
Comment on attachment 8942792 [details]
Bug 1237454 - Test for an animation on a visibility:hidden element which has a child.

https://reviewboard.mozilla.org/r/213038/#review218754

::: dom/animation/test/mozilla/file_restyles.html:698
(Diff revision 1)
>  
>      await ensureElementRemoval(parentDiv);
>    });
>  
> +  add_task(
> +    async function restyling_animations_on_visiblily_hidden_element() {

This test looks good to me, but we could change the name to something like:
`restyling_animations_on_visibility_hidden_element with visibility_changed_children` or others, so we could understand we are trying to change the visibility of the children.
Attachment #8942792 - Flags: review?(boris.chiou) → review+
Comment on attachment 8942793 [details]
Bug 1237454 - Test for an animation on visibility: hidden element which has grandchild.

https://reviewboard.mozilla.org/r/213040/#review218764
Attachment #8942793 - Flags: review?(boris.chiou) → review+
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review218766

Not sure if you will add more tests according to Brian's comment, so r=me only on the current test change. Besides, I believe Brian and Cameron will review the rest part. :)
Attachment #8942794 - Flags: review?(boris.chiou) → review+
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review218812

::: layout/generic/nsFrame.cpp:11352
(Diff revision 1)
> +nsIFrame::UpdateVisibleDescendantsState()
> +{
> +  if (StyleVisibility()->IsVisible()) {
> +    // Notify to invisible ancestors that a visible descendant exists now.
> +    nsIFrame* ancestor;
> +    for (ancestor = GetFlattenedTreeParentPrimaryFrame();

FWIW, I'd use `GetInFlowParent` instead. `GetFlattenedTreeParentPrimaryFrame` is bogus for `display: contents`.
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

Clearing review request for now since the change hint is not handled what I expected, I need to investigate it.
Attachment #8942794 - Flags: review?(cam)
Attachment #8942794 - Flags: review?(bbirtles)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> Comment on attachment 8942794 [details]
> Bug 1237454 - Throttle animations on visibility:hidden element.
> 
> Clearing review request for now since the change hint is not handled what I
> expected, I need to investigate it.

Oh well, it's working as expected.  My debug print was wrong. :/
(In reply to Brian Birtles (:birtles) from comment #22)
> (Diff revision 1)
> > +static bool
> > +HasNoVisibleDescendants(const nsIFrame* aFrame)
> > +{
> > +  for (nsIFrame::ChildListIterator lists(aFrame);
> > +       !lists.IsDone();
> > +       lists.Next()) {
> > +    for (nsIFrame* f : lists.CurrentList()) {
> > +      if (f->IsVisibleOrMayHaveVisibleDescendants()) {
> > +        return false;
> > +      }
> > +    }
> > +  }
> > +  return true;
> > +}
> > +
> > +void
> > +nsIFrame::UpdateVisibleDescendantsState()
> > +{
> > +  if (StyleVisibility()->IsVisible()) {
> > +    // Notify to invisible ancestors that a visible descendant exists now.
> > +    nsIFrame* ancestor;
> > +    for (ancestor = GetFlattenedTreeParentPrimaryFrame();
> > +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> > +         ancestor = ancestor->GetFlattenedTreeParentPrimaryFrame()) {
> > +      ancestor->mHasNoVisibleDescendants = false;
> > +    }
> > +  } else {
> > +    mHasNoVisibleDescendants = HasNoVisibleDescendants(this);
> > +  }
> > +}
> 
> I had trouble following this logic at first. I think the two problems were:
> 
> 1. the naming mHasNoVisibleDescendants (see comment below)
> 2. I didn't understand that all descendants will get the relevant change
> hint and run this in turn.
> 
> I'm a bit concerned that if we make a large subtree visibility:hidden, that
> the result of processing all the change hints on descendants will mean we
> end up traversing the entire subtree. Is that the case?
> 
> I think you said you were going to check this code over once more. While
> you're at it, could you explain the steps that take place when we set
> visibility:hidden on frame B in the graph below:
> 
>       A
>      / \
>     B   C
>    / \
>   D   E
>  /
> F

Yeah, I did overlook that case, e.g. an element is initially visible and has a child whose visibility is 'inherit' and later, changing the parent element visibility to hidden.  This case is not optimized with the current patch.  In a previous version of the patch, there were code that traverses down all descendants, but after some local running test cases, I noticed all test cases I had passed without the traversing down all descendants, so I did drop the code.  Though actually there were a lack of test case that is not optimized. 

I added a lack test case in the last commit and push a try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1219c1b807c5f58feac10c0099a099b138fefac5

Note that if the element visibility is initially 'hidden', then all descendants' mAllDescendantsAreInvisible is set to true, since it's set in nsIFrame::Init(). So it's optimized.
Using GetInFlowParent caused a crash in a devtool test.  It seems that it should be called from UnbindFromTree().  I did add a UpdateVisibleDescendantsState [1] in UnbindFromTree() to make a test case (throttling again when a visible child is removed) [2] pass.   But the case is not a big deal, so I will drop the call in this bug.

A new try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4114c32f5807d681bf72f02f67ecafb531268978

[1] https://hg.mozilla.org/try/rev/849b77932773c67a4c29ae8dbce0f35e471be945#l3.13
[2] https://hg.mozilla.org/try/rev/849b77932773c67a4c29ae8dbce0f35e471be945#l2.154
Comment on attachment 8942791 [details]
Bug 1237454 - Test for an animation in the parent element whose visibility is changed.

https://reviewboard.mozilla.org/r/213036/#review219150

::: dom/animation/test/mozilla/file_restyles.html:666
(Diff revision 2)
>  
>        await ensureElementRemoval(div);
>      }
>    );
>  
> +  add_task(async function restyling_animations_in_visiblily_changed_parent() {

s/visiblily/visibility/
Comment on attachment 8942790 [details]
Bug 1237454 - An additional check that an animation on visibility: hidden element starts restyling when the element gets visible.

https://reviewboard.mozilla.org/r/213034/#review219152

::: dom/animation/test/mozilla/file_restyles.html:642
(Diff revision 2)
>  
>      await ensureElementRemoval(parentElement);
>    });
>  
> -  add_task(async function no_restyling_main_thread_animations_in_visiblily_hidden_element() {
> +  add_task(
> +    async function restyling_animations_on_visiblily_hidden_element_which_gets_visible() {

s/visiblily/visibility/
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review219172

::: layout/generic/nsFrame.cpp:11334
(Diff revision 2)
> +  for (nsIFrame::ChildListIterator lists(aFrame);
> +       !lists.IsDone();
> +       lists.Next()) {
> +    for (nsIFrame* f : lists.CurrentList()) {

Say we have:

<div style="position: absolute; visibility: hidden;">
  <div style="float: left; visibility: visible">
    hello
  </div>
</div>

Will we find the child <div>'s frame in these loops, or only its placeholder frame?  Do we have to follow the placeholder to the OOF frame to correctly return false?

Similarly, is it unneccessary to look at the OOF child lists of aFrame?  (Not a correctness issue, but we would unnecessarily not perform the optimization.)
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review219148

As discussed on IRC, I think it's probably worth investigating other approaches to handle large visibility:hidden subtrees (or, perhaps more accurately, animations in visibility:hidden subtrees that have grandchildren). But if this fixes the Google issues, and doesn't (so far as I can tell) have pathological performance cases, we should probably land this now.

::: commit-message-92e0f:45
(Diff revision 2)
> +  div.appendChild(child);
> +
> +  div.animate(..); // the 'div' is visible
> +  // The animation isn't throttled since the animating element is visible
> +
> +  div.style.visiblily = 'hidden';

s/visiblily/visibility/

(Probably worth doing an `rg visiblily` on your tree just in case there are other instances!)

::: layout/generic/nsFrame.cpp:11330
(Diff revision 2)
>    }
>  
>    return result;
>  }
>  
> +// Returns true if we can guarantee there is no visible descendants.

(Nit: s/there is/there are/)

::: layout/generic/nsIFrame.h:4098
(Diff revision 2)
>  
> +  // Returns true if this frame is visible or may have visible descendants.
> +  bool IsVisibleOrMayHaveVisibleDescendants() const {
> +    return !mAllDescendantsAreInvisible || StyleVisibility()->IsVisible();
> +  }
> +  // Update mAllDescendantsAreInvisible flag for this frame and ancestors.

"Update mAllDescendantsAreInvisible flag for this frame and ancestors (if this frame is visible)."

?
Attachment #8942794 - Flags: review?(bbirtles) → review+
Gosh, I did not notice this Cameron's comment..

(In reply to Cameron McCormack (:heycam) (away for the moment) from comment #41)
> Comment on attachment 8942794 [details]
> Bug 1237454 - Throttle animations on visibility:hidden element.
> 
> https://reviewboard.mozilla.org/r/213042/#review219172
> 
> ::: layout/generic/nsFrame.cpp:11334
> (Diff revision 2)
> > +  for (nsIFrame::ChildListIterator lists(aFrame);
> > +       !lists.IsDone();
> > +       lists.Next()) {
> > +    for (nsIFrame* f : lists.CurrentList()) {
> 
> Say we have:
> 
> <div style="position: absolute; visibility: hidden;">
>   <div style="float: left; visibility: visible">
>     hello
>   </div>
> </div>
> 
> Will we find the child <div>'s frame in these loops, or only its placeholder
> frame?  Do we have to follow the placeholder to the OOF frame to correctly
> return false?
> Gosh, I did not notice this Cameron's comment..

(In reply to Cameron McCormack (:heycam) (away for the moment) from comment #41)
> Comment on attachment 8942794 [details]
> Bug 1237454 - Throttle animations on visibility:hidden element.
> 
> https://reviewboard.mozilla.org/r/213042/#review219172
> 
> ::: layout/generic/nsFrame.cpp:11334
> (Diff revision 2)
> > +  for (nsIFrame::ChildListIterator lists(aFrame);
> > +       !lists.IsDone();
> > +       lists.Next()) {
> > +    for (nsIFrame* f : lists.CurrentList()) {
> 
> Say we have:
> 
> <div style="position: absolute; visibility: hidden;">
>   <div style="float: left; visibility: visible">
>     hello
>   </div>
> </div>
> 
> Will we find the child <div>'s frame in these loops, or only its placeholder
> frame?  Do we have to follow the placeholder to the OOF frame to correctly
> return false?
> 
> Similarly, is it unneccessary to look at the OOF child lists of aFrame? 
> (Not a correctness issue, but we would unnecessarily not perform the
> optimization.)

Oh right. Indeed, I missed the out-of-flow cases (honestly I usually miss the case since I am not yet familiar with frames), I will check the case and will revise my patches and send review request again. :)

Thanks for the review!
> Similarly, is it unneccessary to look at the OOF child lists of aFrame? 
> (Not a correctness issue, but we would unnecessarily not perform the
> optimization.)

Oh right. Indeed, I missed the out-of-flow cases (honestly I usually miss the case since I am not yet familiar with frames), I will check the case and will revise my patches and send review request again. :)

Thanks for the review!
Something weird thing happened..
I could get back this bug finally. :)

(In reply to Cameron McCormack (:heycam) (away for the moment) from comment #41)
> ::: layout/generic/nsFrame.cpp:11334
> (Diff revision 2)
> > +  for (nsIFrame::ChildListIterator lists(aFrame);
> > +       !lists.IsDone();
> > +       lists.Next()) {
> > +    for (nsIFrame* f : lists.CurrentList()) {
> 
> Say we have:
> 
> <div style="position: absolute; visibility: hidden;">
>   <div style="float: left; visibility: visible">
>     hello
>   </div>
> </div>
> 
> Will we find the child <div>'s frame in these loops, or only its placeholder
> frame?  Do we have to follow the placeholder to the OOF frame to correctly
> return false?

I did check the test case. You are absolutely right.  We have to follow the placeholder there.

> Similarly, is it unneccessary to look at the OOF child lists of aFrame? 
> (Not a correctness issue, but we would unnecessarily not perform the
> optimization.)

IIUC, it's not necessary if we correctly follow the placeholder then find the real frame.

Anyway, I will add a test case that needs to track the placeholder.

Thank you so much!
I did forget to mend visibilily typos there. :)

Emilio, could you please take a time to look the first and the last patches here?  I don't want to interfere heycam's rest days for now. :)  Thanks!
Note that my intention for this bug is to fix google search result page case (bug 1218169).  I know the patche here is not perfect but I think it fixes some amount of cases (most cases?) in the wild.
Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

https://reviewboard.mozilla.org/r/213032/#review223534

::: layout/base/nsChangeHint.h:254
(Diff revision 4)
> +   * Indicates that the visiblity property changed.
> +   * This change hint is used for skip restyling for animations on
> +   * visibility:hidden elements in the case where the elements have no visible
> +   * descendants.
> +   */
> +  nsChangeHint_VisibilityChange = 1u << 31,

Phew, the very last one...
Attachment #8942789 - Flags: review?(emilio) → review+
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review223536

::: layout/generic/nsFrame.cpp:742
(Diff revision 4)
> +
> +  // For a newly created frame, we need to update this frame's visibility state.
> +  // Usually we update the state when the frame is restyled and has a
> +  // VisibilityChange change hint but we don't generate any change hints for
> +  // newly created frames.
> +  UpdateVisibleDescendantsState();

If we manage to do it, there'd be no dependencies on the style tree of our descendants being up-to-date, we could even move this to stop being a hint (and move it to `nsStyleContext::DidSetStyleContext` or something similar...

::: layout/generic/nsFrame.cpp:11359
(Diff revision 4)
> +            IsVisibleOrMayHaveVisibleDescendants()) {
> +        return false;

I'm somewhat concerned about this loop... Since we post changes bottom-up, if I change a visibility: visible element to visibility: hidden, we're doing tons of subtree walks suddenlty, right?

::: layout/generic/nsFrame.cpp:11379
(Diff revision 4)
> +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> +         ancestor = ancestor->GetInFlowParent()) {
> +      ancestor->mAllDescendantsAreInvisible = false;
> +    }
> +  } else {
> +    mAllDescendantsAreInvisible = HasNoVisibleDescendants(this);

Why can't we rely on our children getting to notify us whenever they're visible, and assume `true` otherwise? That is, don't touch it when visibility changes?

Is it just to catch dynamic changes to visibility on children? If so, that looks kinda like a hack...

::: layout/generic/nsIFrame.h:4363
(Diff revision 4)
>  
> +  /**
> +   * True if we are certain that all descendants are not visible.
> +   *
> +   * This flag is conservative in that it might sometimes be false even if, in
> +   * fact, all descendants are invisible.

Putting an example of this would be nice.
Attachment #8942794 - Flags: review?(emilio)
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review223540

::: layout/generic/nsFrame.cpp:11379
(Diff revision 4)
> +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> +         ancestor = ancestor->GetInFlowParent()) {
> +      ancestor->mAllDescendantsAreInvisible = false;
> +    }
> +  } else {
> +    mAllDescendantsAreInvisible = HasNoVisibleDescendants(this);

I see, so when _our_ visibility changes to `hidden`, there's nothing that could possibly change the flag back to true... Hmm
Comment on attachment 8942794 [details]
Bug 1237454 - Throttle animations on visibility:hidden element.

https://reviewboard.mozilla.org/r/213042/#review223546

My last question is how does this play with frame continuations and such. I guess animations only look at primary frames? If so, this is fine I guess, but we never call `UpdateVisibilityState` on the later continuations... Should we?

::: layout/generic/nsFrame.cpp:11359
(Diff revision 4)
> +            IsVisibleOrMayHaveVisibleDescendants()) {
> +        return false;

Also, I cannot read, and this only traverses direct children, so it's not so concerning I guess... Though it would if we throttled all the cases we should throttle I think...

::: layout/generic/nsFrame.cpp:11379
(Diff revision 4)
> +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> +         ancestor = ancestor->GetInFlowParent()) {
> +      ancestor->mAllDescendantsAreInvisible = false;
> +    }
> +  } else {
> +    mAllDescendantsAreInvisible = HasNoVisibleDescendants(this);

Ok, so after thinking a bit more thoroughly, there's no great thing I can think of that doesn't involve depending on the style tree on the children to be up-to-date...

I think it's very unfortunate to have this full frame-tree walk... I suspect something like swithing the `visibility` of the `body` to `hidden` in a case like:

```
<style>
div { position: absolute }
</style>
<body><div><div><div><div><!-- ... -->
```

throttling it correctly would involve a _lot_ of frame-tree walking :(.

But I can't think of anything much better honestly. Also I guess given this should run relatively unfrequently this is r=me, thanks for bearing with me :)
Attachment #8942794 - Flags: review+
Comment on attachment 8942789 [details]
Bug 1237454 - Add VisibilityChange change hint.

https://reviewboard.mozilla.org/r/213032/#review223550

::: layout/style/nsStyleStruct.cpp:4106
(Diff revision 4)
>      if ((mImageOrientation != aNewData.mImageOrientation)) {
>        hint |= nsChangeHint_AllReflowHints |
>                nsChangeHint_RepaintFrame;
>      }
>      if (mVisible != aNewData.mVisible) {
> +      hint |= nsChangeHint_VisibilityChange;

May be worth to guard this in `if (mVisible == VISIBLE || aNewData.mVisible == VISIBLE)`.
Thanks for careful reviews!

(In reply to Emilio Cobos Álvarez [:emilio] from comment #63)
> Comment on attachment 8942794 [details]
> Bug 1237454 - Throttle animations on visibility:hidden element.
> 
> https://reviewboard.mozilla.org/r/213042/#review223546
> 
> My last question is how does this play with frame continuations and such. I
> guess animations only look at primary frames? If so, this is fine I guess,
> but we never call `UpdateVisibilityState` on the later continuations...
> Should we?

Yeah, as far as I know, we only care the primary frame.  But as per bug 1408731, there are some cases that animations on continuations don't work properly.  Someday I have to revisit bug 1408731, and should look into in detail what's going on there.  After that I can revisit this optimization again.  (After that I think I have to audit all over the place for animations)

> ::: layout/generic/nsFrame.cpp:11379
> (Diff revision 4)
> > +         ancestor && !ancestor->StyleVisibility()->IsVisible();
> > +         ancestor = ancestor->GetInFlowParent()) {
> > +      ancestor->mAllDescendantsAreInvisible = false;
> > +    }
> > +  } else {
> > +    mAllDescendantsAreInvisible = HasNoVisibleDescendants(this);
> 
> Ok, so after thinking a bit more thoroughly, there's no great thing I can
> think of that doesn't involve depending on the style tree on the children to
> be up-to-date...
> 
> I think it's very unfortunate to have this full frame-tree walk... I suspect
> something like swithing the `visibility` of the `body` to `hidden` in a case
> like:
> 
> ```
> <style>
> div { position: absolute }
> </style>
> <body><div><div><div><div><!-- ... -->
> ```

Yes, that's right. Tons of flat children is the worst case for this optimizations.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/027b0c65d944
Add VisibilityChange change hint. r=emilio
https://hg.mozilla.org/integration/autoland/rev/43ca55e7c93b
An additional check that an animation on visibility: hidden element starts restyling when the element gets visible. r=boris
https://hg.mozilla.org/integration/autoland/rev/0b9872865f0e
Test for an animation in the parent element whose visibility is changed. r=boris
https://hg.mozilla.org/integration/autoland/rev/c481f409feaa
Test for an animation on a visibility:hidden element which has a child. r=boris
https://hg.mozilla.org/integration/autoland/rev/2dbbfc331bdf
Test for an animation on visibility: hidden element which has grandchild. r=boris
https://hg.mozilla.org/integration/autoland/rev/f8d771835fd2
Throttle animations on visibility:hidden element. r=birtles,boris,emilio
I don't quite understand why the failure didn't appear on the try in comment 66.
One possibility I can think of is that the try was based on m-c.
Hmm, I did push another try based on m-c but it has already included today's merge.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b04839164fe455346b51593c62019ef1835047f1&selectedJob=160588043

It's green.  I did retrigger there, let's see what happens.
It seems to be an intermittent happens only on Windows. sigh.
Though I still don't understand why the test failure happens only on Windows, I noticed that when we unthrottle the transform animation in visibility:hidden we need to check the element is in a scrollable ancestor unlike the scrolled-out animation.  That's because visibility:hidden element keeps invisible even after the element is moved into view.

With this fix, the failure on Windows has gone away.  (There is an unrelated orange).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=080ec871e301bfb59eefe44fe816229a1a1e9f93
Flags: needinfo?(hikezoe)
Comment on attachment 8949660 [details]
Bug 1237454 - Unthrottle transform animations in visibility:hidden element periodically only if the element is scrolled out.

https://reviewboard.mozilla.org/r/219008/#review225412

At some point we should add comments explaining the difference between CanThrottleTransformChanges and CanThrottleTransformChangesInScrollable.
Attachment #8949660 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #87)
> Comment on attachment 8949660 [details]
> Bug 1237454 - Unthrottle transform animations in visibility:hidden element
> periodically only if the element is scrolled out.
> 
> https://reviewboard.mozilla.org/r/219008/#review225412
> 
> At some point we should add comments explaining the difference between
> CanThrottleTransformChanges and CanThrottleTransformChangesInScrollable.

OK, I will add comments their in a later bug (probably in bug 1437272).

I hope this will be the very final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbf80d62dcc988588ea9a5b0003c3f9b49601e9f
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79a183e59e6c
Add VisibilityChange change hint. r=emilio
https://hg.mozilla.org/integration/autoland/rev/25517928b230
An additional check that an animation on visibility: hidden element starts restyling when the element gets visible. r=boris
https://hg.mozilla.org/integration/autoland/rev/3948591d0a91
Test for an animation in the parent element whose visibility is changed. r=boris
https://hg.mozilla.org/integration/autoland/rev/3217309b2105
Test for an animation on a visibility:hidden element which has a child. r=boris
https://hg.mozilla.org/integration/autoland/rev/2588e518b520
Test for an animation on visibility: hidden element which has grandchild. r=boris
https://hg.mozilla.org/integration/autoland/rev/d57b9784043c
Throttle animations on visibility:hidden element. r=birtles,boris,emilio
https://hg.mozilla.org/integration/autoland/rev/0743a6a82eeb
Unthrottle transform animations in visibility:hidden element periodically only if the element is scrolled out. r=birtles
Depends on: 1589294
Type: defect → enhancement
Performance Impact: --- → ?
Whiteboard: [Power:P1][platform][qf:investigate] → [Power:P1][platform]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: