Closed Bug 1171966 Opened 9 years ago Closed 9 years ago

animation performance degraded in version 38.0.5 (38+) slow bug (version 37.0.2 works fast)

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Version:
38 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox38.0.5 - ---
firefox39 + wontfix
firefox40 + wontfix
firefox41 + verified
firefox42 + verified
firefox-esr38 - ---

People

(Reporter: Atom_linkwww, Assigned: birtles)

References

(
URL
)

Details

(Keywords: perf, regression)

Attachments

(7 files)

DNES_CLEAR.rar
1.67 MB, application/x-rar
Details
Test speed for 37.0.2 and 38.0.5(animate mouseover slow).html
467.33 KB, text/html
Details
Test speed for 37.0.2 and 38.0.5.html
467.33 KB, text/html
Details
Test speed for 37.0.2 and 38.0.5 - another tamplate.html
466.94 KB, text/html
Details
Regression.jpg
209.62 KB, image/jpeg
Details
Update SMIL animation styles only when there are pending changes
7.05 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Rebased patch for aurora
7.11 KB, patch
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file DNES_CLEAR.rar — 
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.81 Safari/537.36

Steps to reproduce:

When page loaded


Actual results:

it behaves slowly


Expected results:

fast response, like in version
https://youtu.be/1RK3l6H0uDU
Summary: version 38.0.5 (38+) slow bug → version 38.0.5 (38+) slow bug (version 37.0.5 works fast)
Summary: version 38.0.5 (38+) slow bug (version 37.0.5 works fast) → version 38.0.5 (38+) slow bug (version 37.0.2 works fast)
URL: https://youtu.be/1RK3l6H0uDU
Slow animation after version 38
The same (slow) situation in cases:
- safe mode
- when hardware acceleration disabled 
- after updating the graphic drivers
could you type about:support into the address-bar, copy the graphics section of that page & provide that here in the bug report?
Flags: needinfo?(Atom_linkwww)
I ll try that, thanks.You got the same slow page (folder 2) in last version FF ? 
The difference between folder 1 and folder 2 is that in folder 1 no active elements
Flags: needinfo?(Atom_linkwww)
https://youtu.be/1RK3l6H0uDU
This bug is seen for all users, making the work with the browser FF difficult

Source of problem is numerios active elements, look like next templates. 

<!--switcher on -->
<g id="switcher.y" cursor="pointer" stroke-width="0.15">
    <g transform="scale(1,1.375)">
        <g>
            <rect x="-0.5" y="-0.5" width="1" height="1" stroke="white" pointer-events="none"></rect>
            <rect x="-0.5" y="-0.5" width="1" height="1" fill="white">
                <!--it makes half-visible selecting effect -->
                <set attributeName="stroke-opacity" begin="mouseover" end="mouseout" to="0.5"></set>
                <!-- explicitly reverse the opacity animation on mouseout -->
                <set attributeName="stroke-opacity" begin="mouseout" end="mouseover" to="1"></set>
            </rect>
            <line x1="0" y1="-0.25" x2="0" y2="0.25" stroke-width="0.17" stroke-linecap="round" pointer-events="none"></line><!-- vertical on -->
            <!--animation-->
            <!--it scales a few times after change committed to this element -->
            <animateTransform attributeType="XML" attributeName="transform" type="scale" dur="0.5s" keyTimes="0;0.5;0.5;1" values="1;1.12;1.12;1" repeatCount="6" fill="freeze"></animateTransform>
            <!--it animates scale up and scale down onclick -->
            <animateTransform attributeName="transform" attributeType="XML" type="scale" from="1" to="1.15" repeatCount="1" begin="mousedown+0.2s" dur="0.2s" fill="freeze"></animateTransform>
            <animateTransform attributeName="transform" attributeType="XML" type="scale" from="1.15" to="1" repeatCount="1" begin="mouseup+0.4s" dur="0.2s" fill="freeze"></animateTransform>
        </g>
    </g>
</g>
<!-- switched on -->
<g id="zn.y" cursor="pointer">
    <g transform="rotate(-90 0 0)">
        <g>
            <rect x="-1.32" y="-0.63" width="3.64" height="1.26" fill="white" stroke-width="0.0" rx="0.12"></rect>
            <rect x="-1.32" y="-0.63" width="3.64" height="1.26" opacity="0.15" stroke-width="0.0" rx="0.12">
                <!--this animation makes half-visible selecting effect -->
                <animate attributeType="CSS" attributeName="opacity" to="0.07" dur="0.5s" begin="mouseover" fill="freeze"></animate>
                <animate attributeType="CSS" attributeName="opacity" to="0.15" dur="0.5s" begin="mouseout" fill="freeze"></animate>
            </rect>
            <!--brush-->
            <line stroke-width="0.15" x1="1.995" y1="-0.05" x2="1.995" y2="0.05" stroke-linecap="round" stroke-miterlimit="10" pointer-events="none"></line>
            <line stroke-width="0.15" x1="1.795" y1="-0.2" x2="1.795" y2="0.2" stroke-linecap="round" stroke-miterlimit="10" pointer-events="none"></line>
            <line stroke-width="0.15" x1="1.595" y1="-0.35" x2="1.595" y2="0.35" stroke-linecap="round" stroke-miterlimit="10" pointer-events="none"></line>
            <line stroke-width="0.15" x1="1.395" y1="-0.5" x2="1.395" y2="0.5" stroke-linecap="round" stroke-miterlimit="10" pointer-events="none"></line>
            <line stroke-width="0.22" x1="1.39" y1="0.005" x2="0.79" y2="0.005" stroke-miterlimit="10" pointer-events="none"></line>
            <!--clock-arrow-->
            <line stroke-width="0.22" stroke-miterlimit="10" x1="0.37" y1="0.005" x2="-0.75" y2="0.005" pointer-events="none"></line>
            <!--rotate-mechanism -->
            <circle cx="0.595" cy="0" r="0.25" stroke-width="0" stroke-miterlimit="10" pointer-events="none"></circle>
            <circle cx="0.595" cy="0" r="0.15" fill="white" fill-opacity="0.8" stroke-width="0" stroke-miterlimit="10" pointer-events="none"></circle>
            <line stroke-width="0.22" stroke-linecap="round" stroke-miterlimit="10" x1="-0.805" y1="-0.5" x2="-0.805" y2="0.50" pointer-events="none"></line>
            <!--it scales a few times after change committed to this element -->
            <animateTransform attributeType="XML" attributeName="transform" type="scale" dur="0.5s" keyTimes="0;0.5;0.5;1" values="1;1.12;1.12;1" repeatCount="6" fill="freeze"></animateTransform>
            <!--it animates scale up and scale down onclick -->
            <animateTransform attributeName="transform" attributeType="XML" type="scale" from="1" to="1.15" repeatCount="1" begin="mousedown+0.2s" dur="0.2s" fill="freeze"></animateTransform>
            <animateTransform attributeName="transform" attributeType="XML" type="scale" from="1.15" to="1" repeatCount="1" begin="mouseup+0.4s" dur="0.2s" fill="freeze"></animateTransform>
        </g>
        <!--unanimate contact to scheme-->
        <line stroke-width="0.22" stroke-miterlimit="10" x1="-0.85" y1="0.005" x2="-2.23" y2="0.005" pointer-events="none"></line>
        <circle cx="-2.23" cy="0" r="0.25" stroke-width="0" stroke-miterlimit="10" pointer-events="none"></circle>
        <circle cx="-2.23" cy="0" r="0.15" fill="white" fill-opacity="0.8" stroke-width="0" stroke-miterlimit="10" pointer-events="none"></circle>
    </g>
</g>
<!-- switched on -->
<g id="dscn.y" cursor="pointer">
    <rect x="-0.63" y="-0.82" width="1.26" height="1.64" fill="white" stroke-width="0" rx="0.12" pointer-events="none"></rect>
    <g>
        <rect x="-0.63" y="-0.82" width="1.26" height="1.64" opacity="0.15" stroke-width="0.0" rx="0.12"/>
            <!--it makes half-visible selecting effect -->
            <animate attributeType="CSS" attributeName="opacity" to="0.1" dur="0.5s" begin="mouseover" fill="freeze"></animate>
            <animate attributeType="CSS" attributeName="opacity" to="0.15" dur="0.5s" begin="mouseout" fill="freeze"></animate>
        </rect>
        
        <line x1="0" y1="-0.73" x2="0" y2="0.55" stroke-width="0.22" pointer-events="none"></line><!-- left from dot -->
        <line x1="-0.5" y1="-0.73" x2="0.5" y2="-0.73" stroke-width="0.22" stroke-linecap="round" pointer-events="none"></line><!-- left vertical -->
        
        <circle cx="0.0" cy="0.61" r="0.215" stroke-width="0" stroke-miterlimit="10" pointer-events="none"></circle><!-- dot -->
        <circle cx="0.0" cy="0.61" r="0.13" fill="white" fill-opacity="0.8" stroke-width="0" stroke-miterlimit="10" pointer-events="none"></circle><!-- dot -->
        
        <!--it scales a few times after change committed to this element -->
        <animateTransform attributeType="XML" attributeName="transform" type="scale" dur="0.5s" keyTimes="0;0.5;0.5;1" values="1;1.12;1.12;1" repeatCount="6" fill="freeze"></animateTransform>
        
        <!--it animates scale up and scale down onclick -->
        <animateTransform attributeName="transform" attributeType="XML" type="scale" from="1" to="1.15" repeatCount="1" begin="mousedown+0.2s" dur="0.2s" fill="freeze"></animateTransform>
        <animateTransform attributeName="transform" attributeType="XML" type="scale" from="1.15" to="1" repeatCount="1" begin="mouseup+0.4s" dur="0.2s" fill="freeze"></animateTransform>
    </g>
</g>

If I try to explicity them and make look like this
<!--switcher on -->
<g id="switcher.y" cursor="pointer" stroke-width="0.15">
    <g transform="scale(1,1.375)">
        <rect x="-0.5" y="-0.5" width="1" height="1" stroke="white" pointer-events="none"></rect>
    </g>
</g>
<!-- switched on -->
<g id="zn.y" cursor="pointer">
    <g transform="rotate(-90 0 0)">
        <rect x="-1.32" y="-0.63" width="3.64" height="1.26" fill="white" stroke-width="0.0" rx="0.12"></rect>
    </g>
</g>
<!-- switched on -->
<g id="dscn.y" cursor="pointer">
    <rect x="-0.63" y="-0.82" width="1.26" height="1.64" fill="white" stroke-width="0" rx="0.12" pointer-events="none"></rect>
</g>

Then spped is ok on 38.0.5, like version 37.0.2
I noticed that the reason for the slow behavior in versions 38+ is attribute 
<animateTransform> ... </animateTransform>
Component: Untriaged → Graphics
Product: Firefox → Core
[Tracking Requested - why for this release]: Large performance regression in 38
tracking-firefox39: --- → ?
tracking-firefox40: --- → ?
tracking-firefox41: --- → ?
tracking-firefox-esr38: --- → ?
Status: UNCONFIRMED → NEW
tracking-firefox38.0.5: --- → +
tracking-firefox39: ? → +
tracking-firefox40: ? → +
tracking-firefox41: ? → +
Ever confirmed: true
Keywords: regression
In 38+ If you hover mouse on element with animation effect the page Then starts to be slowly
In 38+ If you hover mouse on element with animation effect the page Then starts to be slowly
youtu.be/k1urnpoiO5E
Mention - if remove all <animate> / <animateTransform> tags the page not so slow any more
http://youtu.be/RgfikYEHc8Y
Hard case - more difficult tamplete with animate tags in defs block. Any browser load a little harder, but in versions 38+ we can see real differences in speed slowdown
Summary: version 38.0.5 (38+) slow bug (version 37.0.2 works fast) → animation performance degraded in version 38.0.5 (38+) slow bug (version 37.0.2 works fast)
Attached image Regression.jpg — 
Is this was meant ? Is there enough information, or try again with the inbound (Bisection) instead Nightlies
2015-02-17 is good
2015-02-18 is bad
Worked with attach 'DNES_CLEAR.rar'

I tried to select the desired diapason in url (end 17 day, full 18 day):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b6c56fab513d&tochange=9696d1c4b3ba
Somewhere here bad changeset, how to find where, fix it?

It is real to find reason of bug, fix it, and  wait until update new fixed version of Stable release through Preview release. How long it may take time, what's the chance of occurrence of this happy variant(find the cause of a bug, fix it, add correction in a new version of FF).
Or I should count more on my strength? Find a different direction for the solution of this problem (to bypass around - somehow not using animate tag) or adapt the project to another browser
Thank you
Component: Graphics → CSS Parsing and Computation
Via local build
Last Good: 0288ff191edf
First Bad: 7d16f2fd8329

Triggered by: 	7d16f2fd8329	L. David Baron — Bug 960465 patch 6 - Add the new mechanism for avoiding starting spurious transitions as a result of animations: flush animation styles only before doing other restyle processing. r=birtles
This sounds like it's probably related to the first FIXME in RestyleManager::UpdateOnlyAnimationStyles, and affects only pages that use SMIL Animation.  (I believe Chrome is removing support for SMIL Animation, and I wouldn't consider its performance a high priority.)
At the moment they intend (so they say) merely to deprecate it and not remove at least according to https://groups.google.com/a/chromium.org/d/msg/blink-dev/5o0yiO440LM/59rZqirUQNwJ
sounds highpromising xD
I'll try to adapt the project to ccs transiction
Mozilla SMIL animation performance is still degraded (the problem  not resolved)
It is too late to get any fix for this into 39.   It sounds like this performance problem started in 38. 

Do we also intend to deprecate this as it's mentioned that Chrome is in comment 18?
status-firefox39: --- → wontfix
(In reply to Liz Henry (:lizzard) from comment #22)
> Do we also intend to deprecate this as it's mentioned that Chrome is in
> comment 18?

We don't have any intention to deprecate SMIL at this stage.
Brian, is this something we should be fixing in FF40? We have ~2 weeks of time remaining to do so. Could you please help find an owner for this bug?
Flags: needinfo?(bbirtles)
I'll have a look at it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f78a6f642ae

Assuming this patch works, I'm still not sure if this is critical enough to warrant landing on beta so late in the game, however.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #29)
> I'll have a look at it:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f78a6f642ae
> 
> Assuming this patch works, I'm still not sure if this is critical enough to
> warrant landing on beta so late in the game, however.

That patch fixes the performance problem but breaks some tests. I'm not going to be able to look at this properly in time for an uplift to beta.
Untracked for ESR38 as this does not meet the ESR triage bar which includes security related bug fixed and issue that have a significantly impact our end-users. Also see coment #18.
tracking-firefox-esr38: ? → -
I've been trying to understand when we actually need to update SMIL and
I *think* the problem is mapped attributes. I *think* what's happening is
something like the following:

* Animations play normally with the nsSMILAnimationController getting ticks from
  the refresh driver
* If anything happens out-of-band (e.g. a new animation element added, an
  animation element's attribute changed etc. etc.) we tell the animation
  controller a resample is needed
* On a resample (which can happen due to either of the above) we cycle through
  all the timed elements and they mark their animation functions as having
  changed (or not) and give them the required timing information
* Then we build up a compositor for each animated attribute that is changing
  which contains a list of all the animation functions targeting it
* After building up the animation value by visiting each animation function we
  then apply it to the target
* For the case of mapped attributes, this means we set a property of category
  SMIL_MAPPED_ATTR_ANIMVAL, with name corresponding to the attribute name,
  holding the string representing the animated value.
* Then we blow away the computed content-style-rule on the element, i.e. we do:
     mElement->DeleteProperty(SMIL_MAPPED_ATTR_ANIMVAL, SMIL_MAPPED_ATTR_STYLERULE_ATOM);
* Then we request an animation restyle (to fill in the style rule)
     shell->RestyleForAnimation(mElement, eRestyle_Self);

At this point, there's nothing in the nsSMILAnimationController or any of the
nsSMILAnimationFunctions etc. to indicate that there's more work to be done, but
there is.

I *think* this is different to the case for nsSMILCSSProperty where SetAnimValue
updates the SMIL override style immediately.

If that's right, it might explain why we need to update SMIL on every call to
UpdateOnlyAnimationStyles under the scheme introduced in bug 960465.

I'm not really sure about all those details but I wonder if we could better
detect when SMIL needs to be updated by iterating through all the
SMIL_MAPPED_ATTR_ANIMVAL properties in the document and looking for an element
that has a SMIL_MAPPED_ATTR_ANIMVAL attribute but no
SMIL_MAPPED_ATTR_STYLERULE_ATOM. If we find one we know we need to do work.

I don't *think* we also need to inspect the animation controller's "needs
resample state" since I don't think anything in the existing code called by
UpdateOnlyAnimationStyles triggers a SMIL resample and it appears to work so
simply checking of the style rule needs to be updated from the currently
recorded animation values should be enough.
Actually, fixing mapped attributes isn't enough. We have similar issues in other cases too. I'll try a few more ways of fixing this but I'm not sure how much more time this bug is worth.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e38f0885b3db
This hopefully fixes it (a previous version of the patch did but broke other tests; not sure if my opt build will finish before I go home so I'm putting this up for review now)
Attachment #8640329 - Flags: review?(dholbert)
(In reply to Brian Birtles (:birtles) from comment #34)
> This hopefully fixes it (a previous version of the patch did but broke other
> tests; not sure if my opt build will finish before I go home so I'm putting
> this up for review now)

Verified that this still fixes the issue for me.
Depends on: 1188522
Comment on attachment 8640329 [details] [diff] [review]
Update SMIL animation styles only when there are pending changes

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

r=me, with nits below addressed as you see fit.


First, an extended-commit-message nit:
> Bug 960465 (specifically part 6, changeset 7d16f2fd8329) changed the way we
> process animation-only style changes. This causes us to update SMIL animations
> more often than is needed.

s/causes/caused/ (since this is talking about a cset in the past)

::: dom/smil/nsSMILAnimationController.cpp
@@ +439,5 @@
>    }
>  
>    // Update last compositor table
>    mLastCompositorTable = currentCompositorTable.forget();
> +  mHasPendingStyleUpdates = true;

This variable (and its accessor) seem confusingly-named -- we don't *necessarily* have pending style updates when we get here. (For example, suppose we only have one <animate> node, and it's targeting a SVG rect's "x" attribute -- then, we definitely don't have any pending style updates. And yet, we'll set this flag which says we do.)

So: This should probably be renamed to something more tentative, like "mMightHavePendingStyleUpdates", and the accessor should be named something similarly-tentative, like "MightHavePendingStyleUpdates".

@@ +723,5 @@
>                                        : eRestyle_SVGAttrAnimations;
>      aTracker.AddPendingRestyle(key.mElement, rshint, nsChangeHint(0));
>    }
> +
> +  mHasPendingStyleUpdates = false;

We should assert that this flag is true, when we enter this function (nsSMILAnimationController::AddStyleUpdatesTo). If it's false, then we shouldn't be here.
Attachment #8640329 - Flags: review?(dholbert) → review+
(BTW: I filed bug 1189185 after poking around in GDB to see what happens in AddStyleUpdatesTo for non-style-related SVG animations.)
It's too late to consider this fix for 40. We'll have to take the fix in a later release. Marking 40 as wontfix.
status-firefox40: --- → wontfix
status-firefox41: --- → affected
status-firefox42: --- → affected
tracking-firefox42: --- → +
https://hg.mozilla.org/mozilla-central/rev/6efdf94d0b97
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment on attachment 8644728 [details] [diff] [review]
Rebased patch for aurora

Approval Request Comment
[Feature/regressing bug #]: bug 960465
[User impact if declined]: Performance degradation on content with SVG (SMIL) animation including nvite.com (bug 1188522)
[Describe test coverage new/current, TreeHerder]: Baked on central for 1 week
[Risks and why]: Minimal
[String/UUID change made/needed]: None
Attachment #8644728 - Flags: approval-mozilla-aurora?
Flags: needinfo?(dbaron)
Is it possible to fix the bug earlier, in the Firefox 41?
[Firefox 42	2015-11-03]
[Firefox 41	2015-09-22]
Travis, comment 43 translated into English asks just that.
Understand you, thanks(that was my second account)
Artem, would you be able to verify the fix on the latest Nightly build? If it looks good, we can uplift the change to Firefox 41. Thanks!
Flags: needinfo?(Atom_linkwww)
Comment on attachment 8644728 [details] [diff] [review]
Rebased patch for aurora

[Triage Comment]
Patch has been in Nightly for a week, we don't know of any negative impact. Let's uplift to m-b.
Attachment #8644728 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
FF Nightly 43.0a1 (2015-08-13) works just as well as the FF 37.0.2
Flags: needinfo?(Atom_linkwww)
Flags: qe-verify+
I was able to reproduce this issue on Firefox 38.0.5 (20150525141253) using Windows 7 64-bit.

Verified fixed on Firefox 41 Beta 3 (20150820142145) and Firefox 42.0a2 (2015-08-24) using Windows 7 64-bit and Ubuntu 14.04 32-bit.
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
status-firefox42: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: