1371048 - Consider optimizing hashtable lookups in EffectCompositor

Status: RESOLVED WONTFIX

(Core :: DOM: Animation, enhancement, P4)

Description

[Mats Palmgren (inactive)]

In EffectCompositor::MaybeUpdateAnimationRule we lookup twice on "key":
http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/dom/animation/EffectCompositor.cpp#392,398
We could use GetEntry/RemoveEntry instead to avoid the lookup for Remove(),
except that ComposeAnimationRule() might have changed the hashtable in-between
and made the entry pointer stale as a result.

However, if it's rare that ComposeAnimationRule() updates the hashtable
then we could perhaps compare GetGeneration() before/after and use
RemoveEntry() only if they're the same, otherwise Remove()?

Assuming that this code is hot and worth optimizing to this degree of course...

Ditto for EffectCompositor::PreTraverse:
http://searchfox.org/mozilla-central/rev/1a054419976437d0778a2b89be1b00207a744e94/dom/animation/EffectCompositor.cpp#1104,1126

WDYT?

Comment 1

[Brian Birtles (:birtles)]

Thanks for looking into this. I believe that code is only used in the Gecko code path which we're hoping will soon become obsolete when we turn on Stylo for all platforms so I don't think we should worry about optimizing this.

Comment 2

[Mats Palmgren (inactive)]

OK, sounds good to me.

Comment 3

[Mats Palmgren (inactive)]

There are also unnecessary hashtable lookups in these files:

http://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/animation/AnimationTimeline.cpp#39,47
http://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/animation/EffectCompositor.cpp#271-272,282
http://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/animation/EffectCompositor.cpp#394,400
http://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/animation/KeyframeEffectReadOnly.cpp#475,496

Will those also be obsolete with Stylo?

Comment 4

[Brian Birtles (:birtles)]

(In reply to Mats Palmgren (:mats) from comment #3)
> There are also unnecessary hashtable lookups in these files:
> 
> http://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/animation/AnimationTimeline.
> cpp#39,47
No (i.e. *not* obsolete with Stylo)

> http://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/animation/EffectCompositor.
> cpp#271-272,282
No

> http://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/animation/EffectCompositor.
> cpp#394,400
Yes (i.e. *will* be obsolete with Stylo)

> http://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/animation/
> KeyframeEffectReadOnly.cpp#475,496
Yes

Comment 5

[Mats Palmgren (inactive)]

Thanks Brian, I filed bug 1374126 and bug 1374125 to fix those two issues.