Closed Bug 1371048 Opened 3 years ago Closed 3 years ago
Consider optimizing hashtable lookups in Effect
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?
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.
OK, sounds good to me.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
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?
(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
You need to log in before you can comment on or make changes to this bug.