Bug 1376594 - Track locally whether an effect is part of an EffectSet to avoid hashmap lookups; r?hiro draft
authorBrian Birtles <birtles@gmail.com>
Wed, 05 Jul 2017 10:29:58 +0900
changeset 605983 b2a8081b12557cabf59fb7d9e51786ed1f2ef016
parent 605863 a418121d46250f91728b86d9eea331029c264c30
child 605984 d3e1ccba9fa09207554ff18b4a371dba7e02e769
push id67572
push userbbirtles@mozilla.com
push dateMon, 10 Jul 2017 06:46:44 +0000
reviewershiro
bugs1376594
milestone56.0a1
Bug 1376594 - Track locally whether an effect is part of an EffectSet to avoid hashmap lookups; r?hiro MozReview-Commit-ID: IEeAmyR9ZlS
dom/animation/KeyframeEffectReadOnly.cpp
dom/animation/KeyframeEffectReadOnly.h
--- a/dom/animation/KeyframeEffectReadOnly.cpp
+++ b/dom/animation/KeyframeEffectReadOnly.cpp
@@ -952,31 +952,39 @@ KeyframeEffectReadOnly::UpdateTargetRegi
 
   // Animation::IsRelevant() returns a cached value. It only updates when
   // something calls Animation::UpdateRelevance. Whenever our timing changes,
   // we should be notifying our Animation before calling this, so
   // Animation::IsRelevant() should be up-to-date by the time we get here.
   MOZ_ASSERT(isRelevant == IsCurrent() || IsInEffect(),
              "Out of date Animation::IsRelevant value");
 
-  if (isRelevant) {
+  if (isRelevant && !mInEffectSet) {
     EffectSet* effectSet =
       EffectSet::GetOrCreateEffectSet(mTarget->mElement, mTarget->mPseudoType);
     effectSet->AddEffect(*this);
+    mInEffectSet = true;
     UpdateEffectSet(effectSet);
-  } else {
+  } else if (!isRelevant && mInEffectSet) {
     UnregisterTarget();
   }
 }
 
 void
 KeyframeEffectReadOnly::UnregisterTarget()
 {
+  if (!mInEffectSet) {
+    return;
+  }
+
   EffectSet* effectSet =
     EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType);
+  MOZ_ASSERT(effectSet, "If mInEffectSet is true, there must be an EffectSet"
+                        " on the target element");
+  mInEffectSet = false;
   if (effectSet) {
     effectSet->RemoveEffect(*this);
 
     if (effectSet->IsEmpty()) {
       EffectSet::DestroyEffectSet(mTarget->mElement, mTarget->mPseudoType);
     }
   }
 }
@@ -1829,16 +1837,20 @@ KeyframeEffectReadOnly::ContainsAnimated
   }
 
   return false;
 }
 
 void
 KeyframeEffectReadOnly::UpdateEffectSet(EffectSet* aEffectSet) const
 {
+  if (!mInEffectSet) {
+    return;
+  }
+
   EffectSet* effectSet =
     aEffectSet ? aEffectSet
                : EffectSet::GetEffectSet(mTarget->mElement,
                                          mTarget->mPseudoType);
   if (!effectSet) {
     return;
   }
 
--- a/dom/animation/KeyframeEffectReadOnly.h
+++ b/dom/animation/KeyframeEffectReadOnly.h
@@ -412,16 +412,21 @@ protected:
 
   // The non-animated values for properties in this effect that contain at
   // least one animation value that is composited with the underlying value
   // (i.e. it uses the additive or accumulate composite mode).
   nsDataHashtable<nsUint32HashKey, StyleAnimationValue> mBaseStyleValues;
   nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue>
     mBaseStyleValuesForServo;
 
+  // True if this effect is in the EffectSet for its target element. This is
+  // used as an optimization to avoid unnecessary hashmap lookups on the
+  // EffectSet.
+  bool mInEffectSet = false;
+
   // We only want to record telemetry data for "ContentTooLarge" warnings once
   // per effect:target pair so we use this member to record if we have already
   // reported a "ContentTooLarge" warning for the current target.
   bool mRecordedContentTooLarge = false;
   // Similarly, as a point of comparison we record telemetry whether or not
   // we get a "ContentTooLarge" warning, but again only once per effect:target
   // pair.
   bool mRecordedFrameSize = false;