KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() seems to be expensive

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: birtles)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
This function seems to perform some expensive hashtable lookup which shows up under refresh driver ticks in Speedometer.  It also deletes a node property which shows up under Node.removeChild() also in Speedometer.

This profile which captures about 100 or so subtests runs of Speedometer demonstrates both issues: http://bit.ly/2rZDaew
(Assignee)

Comment 1

2 years ago
I went through this code to see what hashmap lookups we could avoid. Notes below.

> void
> KeyframeEffectReadOnly::NotifyAnimationTimingUpdated()
> {
>   UpdateTargetRegistration();

If the animation is relevant, does

    EffectSet* effectSet =
      EffectSet::GetOrCreateEffectSet(mTarget->mElement, mTarget->mPseudoType);

  GetOrCreateEffectSet initially does a call to nsPropertyTable::GetProperty which iterates through a linked
  list (unless |aRemove| is true, which it is not in this case) and this part is not showing up in the profile
  so presumably that's ok.

  If GetOrCreateEffectSet ends up creating an EffectSet, then in nsPropertyTable::SetProperty we end up
  calling

    propertyList->mObjectValueMap.Add(aObject, mozilla::fallible));

  i.e. one hashtable insertion.

  This should only happen for the tick case, and only when the transition is begins playing so this seems
  reasonable.


  The next thing that happens is:

    effectSet->AddEffect(*this);

  Which expands to:

    if (mEffects.Contains(&aEffect)) {
      return;
    }

    mEffects.PutEntry(&aEffect);
    MarkCascadeNeedsUpdate();

  That is, we always look up the hashmap, on every tick in this case.

  IDEA 1: Locally track whether we have added ourselves to the EffectSet so that we can avoid calling
  AddEffect in that case.


  Then finally we do:

    UpdateEffectSet(effectSet);

  Since we pass in the effect set, we should avoid a linked-list lookup here.  After that we simply iterate
  through the effect's array of properties and toggle flags on the EffectSet. No problems here.

If the animation is *not* relevant (as we expect it to become for the DeleteNode case), UpdateEffectSet does:

    UnregisterTarget();

  Which first does:

      EffectSet* effectSet =
        EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType);

  i.e. a linked list lookup.

  If we find an effect set we do:

      effectSet->RemoveEffect(*this);

      if (effectSet->IsEmpty()) {
        EffectSet::DestroyEffectSet(mTarget->mElement, mTarget->mPseudoType);
      }

  This involves a hashmap lookup.

  If we implement IDEA 1, we can skip the call to RemoveEffect (and indeed, the whole function).

  IDEA 2: I was thinking we could just detect irrelevant effects when we next iterate the hashmap but
  I think it's probably best if we *do* remove the effect from the EffectSet here since the EffectSet has
  an owning reference to the effect and failing to drop it here would mean we potentially keep the effect
  alive an extra tick and if we fail to get that next tick, the object could stick around for a while. I'm
  also not entirely sure that we don't have code that assumes that all effects in the EffectSet are relevant
  (in fact, I suspect we do have such code).


The next part of KeyframeEffectReadOnly::NotifyAnimationTimingUpdated() that is of interest is this part:

> // Detect changes to "in effect" status since we need to recalculate the
> // animation cascade for this element whenever that changes.
> bool inEffect = IsInEffect();
> if (inEffect != mInEffectOnLastAnimationTimingUpdate) {
>   MarkCascadeNeedsUpdate();
>   mInEffectOnLastAnimationTimingUpdate = inEffect;
> }

  This will result in another lookup of the EffectSet (i.e. iterating over a linked list). That could probably
  be avoided but since that's not what's showing up in the profile that's probably not a big deal.

Finally we do:

> if (mAnimation && !mProperties.IsEmpty() && HasComputedTimingChanged()) {
>   EffectCompositor::RestyleType restyleType =
>     CanThrottle() ?
>     EffectCompositor::RestyleType::Throttled :
>     EffectCompositor::RestyleType::Standard;
>   RequestRestyle(restyleType);
> }

  That expands to

    if (!mTarget) {
      return;
    }
    nsPresContext* presContext = nsContentUtils::GetContextForContent(mTarget->mElement);
    if (presContext && mAnimation) {
      presContext->EffectCompositor()->
        RequestRestyle(mTarget->mElement, mTarget->mPseudoType,
                      aRestyleType, mAnimation->CascadeLevel());
    }

  The most interesting part of EffectCompositor::RequestRestyle expands to:

    auto& elementsToRestyle = mElementsToRestyle[aCascadeLevel];
    PseudoElementHashEntry::KeyType key = { aElement, aPseudoType };

    if (aRestyleType == RestyleType::Throttled) {
      elementsToRestyle.LookupForAdd(key).OrInsert([]() { return false; });
      mPresContext->PresShell()->SetNeedThrottledAnimationFlush();
    } else {
      // Get() returns 0 if the element is not found. It will also return
      // false if the element is found but does not have a pending restyle.
      bool hasPendingRestyle = elementsToRestyle.Get(key);
      if (!hasPendingRestyle) {
        PostRestyleForAnimation(aElement, aPseudoType, aCascadeLevel);
      }
      elementsToRestyle.Put(key, true);
    }

  The LookupForAdd was added back in:

    https://hg.mozilla.org/mozilla-central/rev/1cea8a8c16a5

  to replace a Contains followed by a Put. So if this profile was taken before that changeset landed (June 20)
  then presumably we are doing one less hashmap lookup in some cases.

  IDEA 3: In the second branch, I suspect we can combine the Get and Put using either LookupForAdd or even
  just GetOrInsert.

  (In fact, I suspect Mats meant to do that based in the lines he highlighted in bug 1371048 comment 3 but
  then in bug 1374126 he only fixed the first branch.)

I'll give IDEA 1 and IDEA 3 a go but Ehsan, is there a set of steps you used to generate the profile? i.e. are there certain subtests I can run somehow?
(Assignee)

Comment 2

2 years ago
Needinfo Ehsan for the final paragraph of comment 1.
Flags: needinfo?(ehsan)
I intentionally left the Get+Put in the else-branch as is because I worried
about PostRestyleForAnimation leading to hashtable mutations.  We've had
some re-entrancy issues in this code, IIRC, so I didn't want to take any chances.
But yes, we could use LookupForAdd in the else-branch too if that can't happen.
Or, maybe we can simply do the Put *before* calling PostRestyleForAnimation?
That seems safer if it works.
(Assignee)

Comment 4

2 years ago
Great, thanks Mats! I'll give that a try!
(Assignee)

Updated

2 years ago
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
Posted file Test case
Here's a test case that exercises roughly the same bit of code. However, taking a profile from this test produces fairly different results to the profile in comment 0 so it's probably not so useful.
(Reporter)

Comment 6

2 years ago
Here is how I got the profile:

 * Use Gecko Profiler, set the sampler interval to 0.1 ms (you want to do this on OSX or Linux! On Windows this will mean the sampler has to do busy wait so this will consume one of your cores, possibly skewing your measurements)
 * Set the buffer size to something super large (I use 630MB)
 * Go to http://speedometer2.benj.me/
 * Start the profiler and run the benchmark.
 * Watch the test progress bar at the bottom, when you run about 100 or so tests, capture a profile by pressing Ctrl+Shift+2.  Otherwise the profile gets too large to open.

Unfortunately I didn't spend the time to figure out which subtest these are coming from.
(Reporter)

Comment 7

2 years ago
BTW if you have specific APIs in mind that can trigger this, cloning WebKit and grepping for them in https://github.com/WebKit/webkit/tree/master/PerformanceTests/Speedometer may lead to useful results also.
Flags: needinfo?(ehsan)
(Assignee)

Comment 8

2 years ago
Great, thanks Ehsan. I only have a Windows machine with me this week so I might need to verify this next week.
(Assignee)

Comment 9

2 years ago
(In reply to Brian Birtles (:birtles) from comment #1)
> If the animation is relevant, does
> 
>     EffectSet* effectSet =
>       EffectSet::GetOrCreateEffectSet(mTarget->mElement,
> mTarget->mPseudoType);
> 
>   GetOrCreateEffectSet initially does a call to nsPropertyTable::GetProperty
> which iterates through a linked
>   list (unless |aRemove| is true, which it is not in this case) and this
> part is not showing up in the profile
>   so presumably that's ok.

It turns out this part is completely wrong. GetProperty *does* do a hashmap lookup:

    auto entry = static_cast<PropertyListMapEntry*>
                            (propertyList->mObjectValueMap.Search(aObject));

Nevertheless, IDEA 1 will still probably help with this but it might be that there is more we can do to keep pointers to effect sets and pass them along.
(Assignee)

Comment 13

2 years ago
These are just WIP patches for now. I need to try and verify what difference they make next week once I have a Linux machine. I suspect we don't want the last patch (it seems quite intrusive and probably only helps in a less common cases).
Agree. The last one makes the functions bit odd.  Though I suspect the less common cases happen on the benchmark, e.g. creating tons of very short duration animations there, etc.
(Assignee)

Comment 15

2 years ago
I've had trouble trying to get stable results for this test scenario, even without any patches applied so I can't say how much these patches help, but in theory, anyway, they should help.
(Assignee)

Updated

2 years ago
Attachment #8882772 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8882773 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8882774 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8883443 [details]
Bug 1376594 - Track locally whether an effect is part of an EffectSet to avoid hashmap lookups;

https://reviewboard.mozilla.org/r/154346/#review159440

::: dom/animation/KeyframeEffectReadOnly.cpp:981
(Diff revision 1)
> +  }
> +
>    EffectSet* effectSet =
>      EffectSet::GetEffectSet(mTarget->mElement, mTarget->mPseudoType);
> +  MOZ_ASSERT(effectSet, "If mInEffectSet is true, there must be an EffectSet"
> +                        " on the target elenent");

Nit: element.
Attachment #8883443 - Flags: review?(hikezoe) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8883444 [details]
Bug 1376594 - Coalesce two hashmap lookups in EffectCompositor::RequestRestyle;

https://reviewboard.mozilla.org/r/154348/#review159964

::: dom/animation/EffectCompositor.cpp:278
(Diff revision 2)
>    } else {
> -    // Get() returns 0 if the element is not found. It will also return
> -    // false if the element is found but does not have a pending restyle.
> -    bool hasPendingRestyle = elementsToRestyle.Get(key);
> -    if (!hasPendingRestyle) {
> +    auto p = elementsToRestyle.LookupForAdd(key);
> +    bool skipRestyle =  false;
> +    // Update hashtable first in case PostRestyleForAnimation mutates it.
> +    // (It shouldn't, but just to be sure.)
> +    if (p) {

I'd probably write this as:
  if (auto p = elementsToRestyle.LookupForAdd(key)) ...
instead, just to make sure 'p' isn't used later in the block
(after the PostRestyleForAnimation call).

::: dom/animation/EffectCompositor.cpp:279
(Diff revision 2)
> +      if (p.Data()) {
> +        skipRestyle = true;
> +      }

I think it would be slightly easier to follow if you replace this
if-statement with an unconditional assignment here:
  skipRestyle = p.Data();

::: dom/animation/EffectCompositor.cpp:283
(Diff revision 2)
> +    if (p) {
> +      if (p.Data()) {
> +        skipRestyle = true;
> +      }
> +      p.Data() = true;
> +    } else {

... and then add "skipRestyle = false;" here, and remove
the initialization in the declaration.

Then it's easier to see that 'skipRestyle' will have the current
value in the table if it exists, or 'false' otherwise.
Attachment #8883444 - Flags: review?(mats) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

2 years ago
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/82561d1913cb
Track locally whether an effect is part of an EffectSet to avoid hashmap lookups; r=hiro
https://hg.mozilla.org/integration/autoland/rev/5dd25eb4898c
Coalesce two hashmap lookups in EffectCompositor::RequestRestyle; r=mats+5168

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/82561d1913cb
https://hg.mozilla.org/mozilla-central/rev/5dd25eb4898c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.