Closed Bug 1371048 Opened 3 years ago Closed 3 years ago

Consider optimizing hashtable lookups in EffectCompositor

Categories

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

enhancement

Tracking

()

RESOLVED WONTFIX

People

(Reporter: mats, Unassigned)

Details

(Keywords: perf)

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?
Flags: needinfo?(bbirtles)
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.
Flags: needinfo?(bbirtles)
OK, sounds good to me.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
(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
Flags: needinfo?(bbirtles)
Thanks Brian, I filed bug 1374126 and bug 1374125 to fix those two issues.
You need to log in before you can comment on or make changes to this bug.