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

VERIFIED FIXED in Firefox 41

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
2 years ago
9 months ago

People

(Reporter: Artem, Assigned: birtles)

Tracking

(Depends on: 1 bug, {perf, regression})

38 Branch
mozilla42
perf, regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox38.0.5-, firefox39+ wontfix, firefox40+ wontfix, firefox41+ verified, firefox42+ verified, firefox-esr38-)

Details

(URL)

Attachments

(7 attachments)

(Reporter)

Description

2 years ago
Created attachment 8615987 [details]
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
(Reporter)

Updated

2 years ago
Summary: version 38.0.5 (38+) slow bug → version 38.0.5 (38+) slow bug (version 37.0.5 works fast)
(Reporter)

Updated

2 years ago
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)
(Reporter)

Updated

2 years ago
(Reporter)

Comment 1

2 years ago
Slow animation after version 38
The same (slow) situation in cases:
- safe mode
- when hardware acceleration disabled 
- after updating the graphic drivers

Comment 2

2 years ago
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)
(Reporter)

Comment 3

2 years ago
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)
(Reporter)

Comment 4

2 years ago
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
(Reporter)

Comment 5

2 years ago
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: --- → ?

Updated

2 years ago
Status: UNCONFIRMED → NEW
tracking-firefox38.0.5: --- → +
tracking-firefox39: ? → +
tracking-firefox40: ? → +
tracking-firefox41: ? → +
Ever confirmed: true
Keywords: regression
(Reporter)

Comment 7

2 years ago
Created attachment 8617902 [details]
Test speed for 37.0.2 and 38.0.5(animate mouseover slow).html

In 38+ If you hover mouse on element with animation effect the page Then starts to be slowly
(Reporter)

Comment 8

2 years ago
Created attachment 8617904 [details]
Test speed for 37.0.2 and 38.0.5.html

In 38+ If you hover mouse on element with animation effect the page Then starts to be slowly
youtu.be/k1urnpoiO5E

Comment 9

2 years ago
Mention - if remove all <animate> / <animateTransform> tags the page not so slow any more
(Reporter)

Comment 10

2 years ago
Created attachment 8617932 [details]
Test speed for 37.0.2 and 38.0.5 - another tamplate.html

 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
(Reporter)

Comment 11

2 years ago
http://stackoverflow.com/questions/30760923/firefox-8-8-8-0-5-9beta-version-problems-slower-speed-response-svg-anima

Updated

2 years ago
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)

Updated

2 years ago
Keywords: regressionwindow-wanted
(Reporter)

Comment 12

2 years ago
Created attachment 8620613 [details]
Regression.jpg

Is this was meant ? Is there enough information, or try again with the inbound (Bisection) instead Nightlies
(Reporter)

Comment 13

2 years ago
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

Comment 14

2 years ago
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4b118b959ffd&tochange=a9dd7c71cc68
Blocks: 960465
Flags: needinfo?(dbaron)
Keywords: regressionwindow-wanted → perf

Updated

2 years ago
Component: Graphics → CSS Parsing and Computation

Comment 15

2 years ago
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
Comment hidden (off-topic)
Comment hidden (off-topic)
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.)

Comment 19

2 years ago
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
(Reporter)

Comment 20

2 years ago
sounds highpromising xD
I'll try to adapt the project to ccs transiction
(Reporter)

Comment 21

2 years ago
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
Comment hidden (off-topic)
Comment hidden (off-topic)
(Assignee)

Comment 25

2 years ago
(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.
Comment hidden (off-topic)
Comment hidden (off-topic)
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)
(Assignee)

Comment 29

2 years ago
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)
(Assignee)

Comment 30

2 years ago
(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: ? → -
(Assignee)

Comment 32

2 years ago
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.
(Assignee)

Comment 33

2 years ago
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
(Assignee)

Comment 34

2 years ago
Created attachment 8640329 [details] [diff] [review]
Update SMIL animation styles only when there are pending changes

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)
(Assignee)

Comment 35

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7bb3129a1274
(Assignee)

Comment 36

2 years ago
(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: --- → +

Comment 40

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6efdf94d0b97
https://hg.mozilla.org/mozilla-central/rev/6efdf94d0b97
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Assignee)

Comment 42

2 years ago
Created attachment 8644728 [details] [diff] [review]
Rebased patch for aurora
(Assignee)

Comment 43

2 years ago
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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(dbaron)

Comment 44

2 years ago
Is it possible to fix the bug earlier, in the Firefox 41?
[Firefox 42	2015-11-03]
[Firefox 41	2015-09-22]

Comment 45

2 years ago
Travis, comment 43 translated into English asks just that.
(Reporter)

Comment 46

2 years ago
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)

Updated

2 years ago
Blocks: 1179832
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/036b07451306
status-firefox41: affected → fixed
(Reporter)

Comment 50

2 years ago
FF Nightly 43.0a1 (2015-08-13) works just as well as the FF 37.0.2
Flags: needinfo?(Atom_linkwww)
Flags: qe-verify+
Stop tracking.
tracking-firefox38.0.5: + → -
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
Blocks: 1315874
You need to log in before you can comment on or make changes to this bug.