Closed Bug 1366989 Opened 3 years ago Closed 2 years ago

Low performance when displaying and interacting with many animations

Categories

(DevTools :: Inspector: Animations, enhancement, P2)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: pbro, Assigned: daisuke)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

Attachments

(3 files, 2 obsolete files)

STR:

- I found this to be more visible with bug 1309468.

- Open the test case URL
data:text/html,<body><script>for (let i = 0; i < 50; i ++){document.body.animate({background: ["red","yellow"]}, {duration:1000, fill: "forwards"})}</script>

- Open the animation inspector.

- Hit the rewind button ==> Notice how the scrubber (the red line) only moves back to the left starting point after a visible delay.

- Hit the play button ==> Same here, see how the scrubber moves toward the right after the animations themselves have played.

This is because we refresh the whole panel when those actions happen, and that takes time.
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P2
Assignee: nobody → dakatsuka
Comment on attachment 8903022 [details]
Bug 1366989 - Part 1: Expose a unique ID for each Animation instance.

https://reviewboard.mozilla.org/r/174792/#review179854

I really don't have a good sense for the security implications of this. Please ask bz for review after updating the comment as below. You'll need a DOM peer review anyway.

::: commit-message-4d4ed:1
(Diff revision 1)
> +Bug 1366989 - Part 1: Expose animation address as internal unique id. r?birtles
> +
> +Now, AnimationInspector refresh whole animation timeline pane by timings that
> +new animation added or click pause button and so on. It takes much time.
> +To avoid this, although want to avoid to re-render animation’s component that
> +already exist, there was no way to get whether which animation corresponds to
> +which UI component.
> +In this patch, expose the animation instance address as internal unique id.
> +In AnimationInspector, holding this id makes it possible to restrict re-render
> +of already rendered animation.

Bug 1366989 - Part 1: Expose a unique ID for each Animation instance

Currently the animation inspector re-generates the entire animation timeline whenever an animation is added, changed, etc. In order to do a more minimal update we would like to identify existing animations by assigning them a unique ID. We cannot, however, simply use the Javascript object identity of the Animation object since the Animations are serialized and sent from the DevTools server to client.

This patch introduces a chrome-only property that provides a unique ID for each Animation object. This ID is based on the Animation's address in memory which alleviates the need to store a separate ID member on the Animation object.
Attachment #8903022 - Flags: review?(bbirtles)
This looks pretty good Daisuke, but I think there's one potential problem with it: backward compatibility. It's frustrating, but we do have to take it into account if we want remote debugging to keep on working reasonably. The reason being that you don't control the version of actors you are currently talking to.

In the case of this bug, there might not be any internalUniqueId field in the state of a player actor. In which case, your UI code would fail because it relies on animation.state.internalUniqueId being defined.
I think there's an easy fix to this. If you detect, on the client, that there's no ID defined, then you assign a new one, this way the animations would be refreshed, just like they are today.

Also, it would help me if you could describe in a comment the solution you are putting in place here. Reading the code makes sense, but I'm afraid of missing important details here. In particular, what's the relationship of this solution with the caching of animations we do on the actor side, and the actorIDs which we could also use to identify animations.
Comment on attachment 8903022 [details]
Bug 1366989 - Part 1: Expose a unique ID for each Animation instance.

https://reviewboard.mozilla.org/r/174792/#review179922
Attachment #8903022 - Flags: review?(bugs) → review+
Thanks Patrick!

Oh, I had forgot the backward compatibility,,
Yes, will add the fallback way according to your advice.

Also, will add the description later, sorry, please wait for a while.
I had not known that we can use actorID is as unique ID for animations.
I tested locally, the behavior looks working well.

Olli, Brian, I'm so sorry, 
may drop patch 1 which you reviewed.
Also, drop patch 2 as well if pass all tests.

Thanks.
Attachment #8903022 - Attachment is obsolete: true
Attachment #8903023 - Attachment is obsolete: true
Attachment #8903023 - Flags: review?(pbrosset)
Patrick, 
also, I wrote the description as commit message.
https://reviewboard.mozilla.org/r/174796/diff/3#index_header
Comment on attachment 8903024 [details]
Bug 1366989 - Part 1: Avoid to refresh whole panel.

https://reviewboard.mozilla.org/r/174796/#review182244

Looks good. I think one more pass needed, but then we should be good.

::: commit-message-c9ff5:3
(Diff revision 3)
> +Currently the animation inspector re-generates the entire animation timeline
> +whenever an animation is added, changed, etc.
> +To avoid this, averts to re-render the component which no needs.
> +
> +In this implementation, premises the actorID can be used as unique id for each
> +animations. The mechanism is below.
> +
> +At initial time, renders all actors  as normally. In this time, holds actorID
> +and related components to componentsMap.
> +Next, in case of that needs to update the UI, gets animation actors from server,
> +and compares actorID of both the actors and componentsMap. If retrieved actorID
> +exists in componentsMap, updates the view area only without re-rendering.
> +For example, supposes, has an animation (actid-1) when opens the inspector, and
> +a new animation (actid-2) was added a little later.
> +At initial rendering, holds "actid-1” of first animation as key and related
> +components to compoentsMap. Next, when “actid-2” animation is added to document,
> +can get animation actors that are “actid-1” and “actid-2” from server. Because
> +“actid-1” is already held in componentsMap, updates “actid-1”’s view area. This
> +is because TimeScale will be updated. Then "actid-2” render as normally since
> +componentMap does not have the actorID. After rendered, holds “actid-2” and
> +related components.
> +
> +However, even if actorID exists, if keyframes (trakcs) and effect timing
> +(state) differ, re-render that. Also, if iterationCount of effect timing
> +represents Infinity, do re-rendering. Because the display area expands by the
> +end of the currently displayed time.
> +
> +And, if actotID in componentsMap is not in retrieved actors, removes related
> +components.

Thank you for the long and detailed description.

::: commit-message-c9ff5:18
(Diff revision 3)
> +and compares actorID of both the actors and componentsMap. If retrieved actorID
> +exists in componentsMap, updates the view area only without re-rendering.
> +For example, supposes, has an animation (actid-1) when opens the inspector, and
> +a new animation (actid-2) was added a little later.
> +At initial rendering, holds "actid-1” of first animation as key and related
> +components to compoentsMap. Next, when “actid-2” animation is added to document,

typo: compoentsMap -> componentsMap

::: commit-message-c9ff5:25
(Diff revision 3)
> +“actid-1” is already held in componentsMap, updates “actid-1”’s view area. This
> +is because TimeScale will be updated. Then "actid-2” render as normally since
> +componentMap does not have the actorID. After rendered, holds “actid-2” and
> +related components.
> +
> +However, even if actorID exists, if keyframes (trakcs) and effect timing

typo: trakcs -> tracks

::: commit-message-c9ff5:30
(Diff revision 3)
> +However, even if actorID exists, if keyframes (trakcs) and effect timing
> +(state) differ, re-render that. Also, if iterationCount of effect timing
> +represents Infinity, do re-rendering. Because the display area expands by the
> +end of the currently displayed time.
> +
> +And, if actotID in componentsMap is not in retrieved actors, removes related

typo: actotID -> actorID

::: devtools/client/animationinspector/components/animation-time-block.js:71
(Diff revision 3)
>          "class": "summary",
> -        "preserveAspectRatio": "none",
> +        "preserveAspectRatio": "none"
> -        "style": `left: ${ x - (state.delay > 0 ? delayW : 0) }%`
>        }
>      });
> +    this.updateSummaryGraphViewBox();

passing summaryEl as a parameter to the function would avoid having to do querySelector inside the function.

::: devtools/client/animationinspector/components/animation-timeline.js:468
(Diff revision 3)
>        TimeScale.addAnimation(state);
>      }
>  
>      this.drawHeaderAndBackground();
>  
>      for (let animation of this.animations) {

This for loop was already quite long before, but only had one code path. It was basically always only creating timeBlocks.
Now it's quite a lot longer and has these 2 branches, with a `continue` somewhere lost. So my fear is that it is too hard to read and follow.
Can you extract the 2 main logical branches into sub functions? So that the function is basically just a for loop with a if/else and then the rest is done in smaller functions that are easier to understand.

::: devtools/client/animationinspector/components/animation-timeline.js:492
(Diff revision 3)
> +        // representation for Infinity.
> +        if (animation.state.iterationCount &&
> +            animationEffectTimingEquals(renderedComponents.state, animation.state) &&
> +            renderedComponents.tracks.toString() === tracks.toString()) {
> +          const timeBlock = renderedComponents.timeBlock;
> +          timeBlock.updateViewBox(animation);

This makes me think we should migrate to React as soon as possible :)
So far, we've inspired ourselves from React by having component like modules. These components have a constructor, an init and a render method. And that's it. So the interface between components is simple and they are relatively independent.
Now that you're trying to optimize things, we see that our simple approach isn't enough, because here you want to reuse an existing instance of a component, but update it a little bit.
So you're introducing a new method to the API of our components.
So that's tying things together a little bit more.

With React, I think we'd just be rendering things again, but we could be smart inside shouldComponentUpdate to avoid unnecessary renders.
But at least components would stay nicely decoupled and with a small API surface only.

I've been thinking of a way to do this differently but couldn't find it.
I would just suggest to rename this to `update`. This way components can both have `render` and `update`. The first one actually generating DOM, while the second one just taking a new state and updating its DOM accordingly.

::: devtools/client/animationinspector/components/animation-timeline.js:792
(Diff revision 3)
> + * Check the equality given states as effect timing.
> + * @param {Object} state of animation.
> + * @param {Object} same to avobe.
> + * @return {boolean} true: same effect timing
> + */
> +function animationEffectTimingEquals(stateA, stateB) {

Suggestion to rename this to `areTimingEffectsEqual`
Attachment #8903024 - Flags: review?(pbrosset)
Comment on attachment 8903025 [details]
Bug 1366989 - Part 2: Modify tests to correspond with changing the animation-timeline.

https://reviewboard.mozilla.org/r/174798/#review182254

::: devtools/client/animationinspector/test/browser_animation_empty_on_invalid_nodes.js
(Diff revision 3)
> -  let onUpdated = panel.once(panel.UI_UPDATED_EVENT);
>    yield selectNodeAndWaitForAnimations(stillNode, inspector);
> -  yield onUpdated;

I wonder if we shouldn't keep an event in this case still.
Right now there will be no animations, no changes, which is good, but the test won't really wait for anything.
The various wait functions that `selectNodeAndWaitForAnimations` calls will just return early without waiting, if I'm not mistaken.
So we won't know whether it's empty because it's supposed to be empty, or if it's being loaded with animations.
What if we regress one day and we do send a panel UI_UPDATED_EVENT, but async, later? This test won't catch it I think.

Can you think of a way to solve this?

::: devtools/client/animationinspector/test/browser_animation_empty_on_invalid_nodes.js
(Diff revision 3)
> -  onUpdated = panel.once(panel.UI_UPDATED_EVENT);
>    yield selectNodeAndWaitForAnimations(commentNode, inspector);
> -  yield onUpdated;

Same here.
Attachment #8903025 - Flags: review?(pbrosset)
Comment on attachment 8903025 [details]
Bug 1366989 - Part 2: Modify tests to correspond with changing the animation-timeline.

https://reviewboard.mozilla.org/r/174798/#review182254

> I wonder if we shouldn't keep an event in this case still.
> Right now there will be no animations, no changes, which is good, but the test won't really wait for anything.
> The various wait functions that `selectNodeAndWaitForAnimations` calls will just return early without waiting, if I'm not mistaken.
> So we won't know whether it's empty because it's supposed to be empty, or if it's being loaded with animations.
> What if we regress one day and we do send a panel UI_UPDATED_EVENT, but async, later? This test won't catch it I think.
> 
> Can you think of a way to solve this?

Thank you very much!

I brought this event listener to selectNodeAndWaitForAnimations.
Because I’d like to hide such the event related codes from test code (browser_xxx.js) into head.js since following reasons.

* There are similar event related codes in various tests.
* If we changed the event timing, we need to update at many points.
* Most of tests is not test for the event timing.
* Test codes will be simple.

As result, I want to write tests as like below.

```
yield openAnimationInspector();
test…

yield selectNodeAndWaitForAnimations();
test…

yield mouseClickAndWaitForAnimations();
test…
```

How do you think??
If it is ok for you, I will file a new bug for this refactoring.
Comment on attachment 8903024 [details]
Bug 1366989 - Part 1: Avoid to refresh whole panel.

I'm sorry. 
I found a bug that the "delay" sign does not move in case of updated animation has "delay".
So, please let me cancel the review.
Attachment #8903024 - Flags: review?(pbrosset)
Comment on attachment 8903024 [details]
Bug 1366989 - Part 1: Avoid to refresh whole panel.

https://reviewboard.mozilla.org/r/174796/#review185320

::: devtools/client/animationinspector/components/animation-time-block.js:157
(Diff revisions 3 - 5)
> +    if (endDelayEl) {
> +      this.updateEndDelayBounds(endDelayEl);
> +    }
>    },
>  
> -  updateSummaryGraphViewBox: function () {
> +  updateSummaryGraphViewBox: function (summaryEl) {

Please add some jsdoc to explain what the function does exactly.

::: devtools/client/animationinspector/components/animation-time-block.js:170
(Diff revisions 3 - 5)
>                             `${totalDisplayedDuration} ` +
>                             `${1 + strokeHeightForViewBox * 2}`);
>      summaryEl.setAttribute("style", `left: ${ x - (state.delay > 0 ? delayW : 0) }%`);
>    },
>  
> +  updateDelayBounds: function (delayEl) {

here too please

::: devtools/client/animationinspector/components/animation-time-block.js:176
(Diff revisions 3 - 5)
> +    const {delayX, delayW} = TimeScale.getAnimationDimensions(this.animation);
> +    delayEl.style.left = `${ delayX }%`;
> +    delayEl.style.width = `${ delayW }%`;
> +  },
> +
> +  updateEndDelayBounds: function (endDelayEl) {

and here

::: devtools/client/animationinspector/components/animation-time-block.js:144
(Diff revision 5)
>        });
> +      this.updateEndDelayBounds(endDelayEl);
> +    }
> +  },
> +
> +  update: function (animation) {

Some jsdoc needed here to explain what `update` is intended for, and how it compares to `render`.
Attachment #8903024 - Flags: review?(pbrosset) → review+
(In reply to Daisuke Akatsuka (:daisuke) from comment #22)
> * There are similar event related codes in various tests.
> * If we changed the event timing, we need to update at many points.
> * Most of tests is not test for the event timing.
> * Test codes will be simple.
> 
> As result, I want to write tests as like below.
> 
> ```
> yield openAnimationInspector();
> test…
> 
> yield selectNodeAndWaitForAnimations();
> test…
> 
> yield mouseClickAndWaitForAnimations();
> test…
> ```
Yes, this looks great. Moving stuff to head.js to avoid repetition in all tests is a very welcome change.
Comment on attachment 8903025 [details]
Bug 1366989 - Part 2: Modify tests to correspond with changing the animation-timeline.

https://reviewboard.mozilla.org/r/174798/#review185332

::: devtools/client/animationinspector/test/head.js:257
(Diff revision 5)
>  }
>  
>  /**
>   * Wait for all AnimationTargetNode instances to be fully loaded
>   * (fetched their related actor and rendered), and return them.
> + * This method should be called after emit "animation-timeline-rendering-completed",

Rephrase to "This method should be called after "animation-timeline-rendering-completed" is emitted"

::: devtools/client/animationinspector/test/head.js:258
(Diff revision 5)
>  
>  /**
>   * Wait for all AnimationTargetNode instances to be fully loaded
>   * (fetched their related actor and rendered), and return them.
> + * This method should be called after emit "animation-timeline-rendering-completed",
> + * since get the all AnimationTargetNode instances from getAnimationTargetNodes().

"since we get all the AnimationTargetNode instances using getAnimationTargetNodes()"
Attachment #8903025 - Flags: review?(pbrosset) → review+
Comment on attachment 8907521 [details]
Bug 1366989 - Part 3: Add test for updating the bounds of summary graph, delay and endDelay element.

https://reviewboard.mozilla.org/r/179230/#review185336

::: devtools/client/animationinspector/test/browser_animation_timeline_add_animation.js:43
(Diff revision 1)
> +  is(previousSummaryGraphEl, currentSummaryGraphEl, ".summary element should be reused");
> +  is(previousDelayEl, currentDelayEl, ".delay element should be reused");
> +  is(previousEndDelayEl, currentEndDelayEl, ".end-delay element should be reused");
> +
> +  ok(currentSummaryGraphWidth > previousSummaryGraphWidth,
> +     "Reused .summary element viewBox width should be long");

s/long/longer

::: devtools/client/animationinspector/test/browser_animation_timeline_add_animation.js:45
(Diff revision 1)
> +  is(previousEndDelayEl, currentEndDelayEl, ".end-delay element should be reused");
> +
> +  ok(currentSummaryGraphWidth > previousSummaryGraphWidth,
> +     "Reused .summary element viewBox width should be long");
> +  ok(currentDelayBounds.left < previousDelayBounds.left,
> +     "Reused .delay element should move to left");

to *the* left

::: devtools/client/animationinspector/test/browser_animation_timeline_add_animation.js:47
(Diff revision 1)
> +  ok(currentSummaryGraphWidth > previousSummaryGraphWidth,
> +     "Reused .summary element viewBox width should be long");
> +  ok(currentDelayBounds.left < previousDelayBounds.left,
> +     "Reused .delay element should move to left");
> +  ok(currentDelayBounds.width < previousDelayBounds.width,
> +     "Reused .delay element should be shorten");

s/shorten/shorter

::: devtools/client/animationinspector/test/browser_animation_timeline_add_animation.js:49
(Diff revision 1)
> +  ok(currentDelayBounds.left < previousDelayBounds.left,
> +     "Reused .delay element should move to left");
> +  ok(currentDelayBounds.width < previousDelayBounds.width,
> +     "Reused .delay element should be shorten");
> +  ok(currentEndDelayBounds.left < previousEndDelayBounds.left,
> +     "Reused .end-delay element should move to left");

to *the* left

::: devtools/client/animationinspector/test/browser_animation_timeline_add_animation.js:51
(Diff revision 1)
> +  ok(currentDelayBounds.width < previousDelayBounds.width,
> +     "Reused .delay element should be shorten");
> +  ok(currentEndDelayBounds.left < previousEndDelayBounds.left,
> +     "Reused .end-delay element should move to left");
> +  ok(currentEndDelayBounds.width < previousEndDelayBounds.width,
> +     "Reused .end-delay element should be shorten");

s/shorten/shorter

::: devtools/client/animationinspector/test/browser_animation_timeline_add_animation.js:60
(Diff revision 1)
> +  info("Add a new animation to the page and check the time again");
> +  let onPlayerAdded = controller.once(controller.PLAYERS_UPDATED_EVENT);
> +  let onRendered = waitForAnimationTimelineRendering(panel);
> +
> +  yield executeInContent("devtools:test:setAttribute", {
> +    selector: `div:nth-child(${nth})`,

Why not give these elements unique IDs in your test HTML file? I find that the nth selector makes this test a bit hard to follow.

::: devtools/client/animationinspector/test/doc_add_animation.html:53
(Diff revision 1)
> +          animation.currentTime = 100;
> +        }
> +      }
> +    });
> +
> +    document.querySelectorAll("div:nth-child(n+2)").forEach(div => {

If you add unique IDs instead, this part would also be easier to understand.
Attachment #8907521 - Flags: review?(pbrosset) → review+
I'll land if there are no error which related to this bug in try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b647a2832a0a2a1595904a0aebb5942a18211de
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f391fb90181c
Part 1: Avoid to refresh whole panel. r=pbro
https://hg.mozilla.org/integration/autoland/rev/7d8f842fe47c
Part 2: Modify tests to correspond with changing the animation-timeline. r=pbro
https://hg.mozilla.org/integration/autoland/rev/5fc5a88872ad
Part 3: Add test for updating the bounds of summary graph, delay and endDelay element. r=pbro
Duplicate of this bug: 1336940
Depends on: 1412622
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.