1807003 - consider to optimize Animation::Cancel

Status: RESOLVED FIXED

(Core :: CSS Transitions and Animations, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

When running the React part of speedometer 2.1, in its DeletingAllItems subtest animation cancelling is taking some time.
https://share.firefox.dev/3Vk15Cz
https://share.firefox.dev/3PJhAXC
It shows up as part of DOM-category.

Could we perhaps avoid some allocations and when the events are finally dispatched, check first if there are listeners?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

And looks like also https://searchfox.org/mozilla-central/rev/e6b709df9b93858364f02ab89f40d78762693db8/dom/base/Element.cpp#1969-1977 plays a part here.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Another profile https://share.firefox.dev/3j1aMbO

Comment 3

[Daniel Holbert [:dholbert]]

FWIW here's a version of one of those profiles, with mozilla::dom::Animation::Cancel focused, and with stack inverted. This shows that all of the time is spent in PR_Now, called by Event::ConstructorInit.

(I don't immediately see where Event::ConstructorInit calls PR_Now; it's probably inlined somewhere.)

In any case: agreed that it'd be nice to avoid dispatching the event in the first place if there are no listeners, if that's something that's easy to detect (not sure).

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #3)

FWIW here's a version of one of those profiles, with mozilla::dom::Animation::Cancel focused, and with stack inverted. This shows that all of the time is spent in PR_Now, called by Event::ConstructorInit.

Here's that link: https://share.firefox.dev/3JjrOwr

(This is from smaug's very first profile in comment 0 BTW.)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

There is no PR_Now() anymore (bug 1807812), I'll reprofile.
The code mentioned in comment 1 is still there.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

https://share.firefox.dev/3RcJdJk is a new one, focused on Animation::Cancel

At least there is EffectSet::DestroyEffectSet. That relates to the same code mentioned in comment 1.
I think we should remove property usage and move the relevant stuff to be part of the
nsExtendedDOMSlots https://searchfox.org/mozilla-central/rev/3a13c5ad9c1dbd065e484a399af75eb98c235def/dom/base/FragmentOrElement.h#157

I guess there could be a helper struct which had (strong?) pointer to each of the properties and nsExtendedDOMSlots could have UniquePtr pointing to that struct.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1807003 - Add a warning to stderr for cc asserts. r=smaug

An earlier version of the patch triggered and it wasn't trivial to see
the message.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1807003 - Centralize animation data in slots. r=#firefox-animation-reviewers!,smaug!

This should both be faster and simpler. Also will allow us in the future
to animate more pseudos without having to add a gazillion properties.

I think we should try to clear more stuff (maybe the whole animation
data) on unbind, but that's a bit tangential.

Depends on D169859

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60ea99a626a7
Add a warning to stderr for cc asserts. r=mccr8

Comment 10

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/60ea99a626a7

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e185891cbfd
Centralize animation data in slots. r=smaug,firefox-animation-reviewers,boris

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/8e185891cbfd

Comment 13

[Mayank Bansal]

Did this improve AWFY-Inferno-TodoMVC by 31% ?
https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&series=mozilla-central,3735844,1,13&series=mozilla-central,3735844,1,13&series=autoland,3912889,1,13&timerange=5184000&zoom=1676217310774,1676633129097,130.93007596856472,272.84116685079124\

(it also improved some subtests by 99% !)
And some improvement in VueJS-TodoMVC by 6.3%.