956388 - Introduce a performance test (like TART) to measure animation performance of entering/exiting customization mode

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

From https://bugzilla.mozilla.org/show_bug.cgi?id=933926#c7:

> (In reply to :Gavin Sharp (email [e-mail]) from comment #6)
> > We need an easy/relatively-generic way to measure animation perf for
> > arbitrary animations like this one. Can we re-purpose infrastructure from
> > TART to apply more broadly? Mark's been thinking about related issues for
> > panel animations.

> Sure. We could start by adding it as an optional test to TART such that 
> devs could test iteratively while developing right now, and if needed, 
> later make it a stand alone one, possibly adding it to talos if you think 
> we should.

> For the animations which we want to test, I'd need to know how to trigger 
> each, and the event which indicates the animation's done. If needed, I 
> could possibly also generalize it by accepting custom trigger functions 
> without rebuilding the addon.

> Let me know how you want to take this forward.

To trigger entering customization mode, you can just call `window.gCustomizeMode.enter();`. The transition is complete when the 'customizationready' event is fired.

To exit customization mode you can call `window.gCustomizeMode.exit();`. The transition is complete when the 'aftercustomization' event is fired.

Avih, can you look in to this?

Comment 1

[Avi Halachmi (:avih)]

I'll take it. Do we want it as a tool for phase one? or do you think it should be a new talos test?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

this seems more in line with a standalone tool vs something that we would run per checking for every platform/branch.  Is this something the average user would experience in a typical browsing session?  If it is a common thing for users to do we should make it a talos test, otherwise, I vote for a standalone tool.

Comment 3

[(dormant account)]

Before we invest into more benchmarking efforts, we should loop back to the product manager to make a call on whether:
a) this animation performs acceptably
b) is worth the engineering effort to improve(as Joel said it's not in the primary browser use-case), track

It might make sense to fix perf concerns by removing the animation. I'm risk-averse on committing engineering time non-primary usecases given that we have so much jank to fix in primary use-cases

Comment 4

[(dormant account)]

Forgot to CC Chad above

Comment 5

[Chad Weiner [:chadw]]

Thanks.  Trying to track down Gavin to discuss.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to Taras Glek (:taras) from comment #3)
> Before we invest into more benchmarking efforts, we should loop back to the
> product manager to make a call on whether:
> a) this animation performs acceptably

It doesn't.

> b) is worth the engineering effort to improve(as Joel said it's not in the
> primary browser use-case), track

Yes.

But in general, what I was trying to get at in the other bug was that it would be useful to have a general purpose developer tool (doesn't need to be as polished as a devtool we would ship - the initial audience here is "my team") for measuring arbitrary animation performance a little more consistently.

Comment 7

[(dormant account)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #6)

> Yes.
> 
> But in general, what I was trying to get at in the other bug was that it
> would be useful to have a general purpose developer tool (doesn't need to be
> as polished as a devtool we would ship - the initial audience here is "my
> team") for measuring arbitrary animation performance a little more
> consistently.

Thanks for clarifying. I think this makes a lot of sense as an adhoc framework to hook arbitrary animations into for irregular tests...Up to Avi to figure out whether that's simple enough to implement.

Comment 8

[Avi Halachmi (:avih)]

I talked to gavin on irc. There are two main issue to consider when "generalizing":
- What's parametric and what's hardcoded.
- What kind of interface do we use, since triggering and capturing animation-end will have to use custom chunks of code. This doesn't lend itself too well to "generic".

We figured we'll start by making TART also able to measure the customize animation. This will require some refactoring since right now TART is built around tab animations. That's what I'll be working on now.

Once we have that, when we need to measure yet another kind of animation, then we'll hopefully have enough data to understand how to generalize it. So we'll approach it one step at a time.

The current step will not be generic from a user point of view, but hopefully reasonably generic from a developer point of view (to modify TART code).

Comment 9

[Avi Halachmi (:avih)]

Attachment: Add customize-mode-animation test to TART

The patch is for the talos repo and modifies the TART addon by adding the customize test - which is disabled by default at the addon (and bumps TART version to 1.55).

This works well in stand-alone mode, and can also be deployed relatively easily to talos, should we so wish (looks like we won't though, according to comments 2, 3).

To test in stand alone mode - same instructions as for TART:
- Install the addon.
- Set ASAP mode (layout.frame_rate=0, docshell.event_starvation_delay_hint=1 (int)).
- Visit chrome://tart/content/tart.html
- Uncheck all test, check "Customize".
- Click start.

Had to workaround some issues (bug 957179, bug 957202, bug 957210), but managed it in such a way that if they're fixed, this test would still work the same (with some code and animations redundancy).

It didn't end up generic though, since so far it didn't seem it was worth it. Maybe when we add yet another animation test in the future.

For reference, if we want to deploy to talos, then with this patch, we either enable the customize test by default such that it becomes another sub-test of TART, or create a new test entry at test.py, identical to the TART test (and uses the same addon) but with a different URI: chrome://tart/content/tart.html#auto&tests=["customize"] (which will run only this test regardless if it's enabled/disabled by default at the addon itself).

Comment 10

[Avi Halachmi (:avih)]

Attachment: TART.v1.55.xpi

For testing, here's a ready to install addon (the patch applied, then zipped, renamed to xpi).

Comment 11

[:Gavin Sharp [email: [e-mail]]]

If this is easy to add to Talos, we should do that too.

Comment 12

[Avi Halachmi (:avih)]

Attachment: bug956388.v3.patch

Add Australis-customize test to talos (CART).

Extends the TART addon to also have a customize test (disabled by default, so doesn't affect TART), add talos code to include the cart test.

Comment 13

[Avi Halachmi (:avih)]

Attachment: TART.v1.6.xpi

TART.v1.6.xpi created directly from patch v3 and can be used stand-alone.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8366770 [details] [diff] [review]
bug956388.v3.patch

Review of attachment 8366770 [details] [diff] [review]:
-----------------------------------------------------------------

this looks pretty straightforward.

::: talos/page_load_test/tart/addon/content/tart.js
@@ +374,5 @@
> +        let tabViewDeck = document.getElementById("tab-view-deck");
> +        let cstyle = window.getComputedStyle(tabViewDeck);
> +        return 1000 * parseFloat(cstyle.transitionDuration, 10);
> +      } catch (e) {
> +        return 150; // Value at the time of writing

how will we ensure this is kept up to date?  can you document what this really means?

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

landed on talos repo:
https://hg.mozilla.org/build/talos/rev/317bf97698b2

waiting on database definitions to be deployed to live databases, and then we can test this on try server for a little bit of time before doing the final deployment to inbound.

Comment 16

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #14)
> Comment on attachment 8366770 [details] [diff] [review]
> bug956388.v3.patch
> 
> Review of attachment 8366770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this looks pretty straightforward.
> 
> ::: talos/page_load_test/tart/addon/content/tart.js
> @@ +374,5 @@
> > +        let tabViewDeck = document.getElementById("tab-view-deck");
> > +        let cstyle = window.getComputedStyle(tabViewDeck);
> > +        return 1000 * parseFloat(cstyle.transitionDuration, 10);
> > +      } catch (e) {
> > +        return 150; // Value at the time of writing
> 
> how will we ensure this is kept up to date?  can you document what this
> really means?

150 is the fallback value of this function at the "catch" clause. The function reads the actual duration from the style, but if it can't for some reason, it falls back to 150 which is the current duration.

I think it's a reasonable approach, but I'm open to other approaches.

Comment 17

[Matt Brubeck (:mbrubeck)]

When this is enabled it should be documented on:
https://wiki.mozilla.org/Buildbot/Talos/Tests

Comment 18

[Avi Halachmi (:avih)]

Note that the commit message accidentally states bug 943859 instead of this bug.

Depending the currently pending talos.zip update.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: talos.json modification to turn on cart (1.00

Comment 20

[Avi Halachmi (:avih)]

Comment on attachment 8368162 [details] [diff] [review]
talos.json modification to turn on cart (1.00

Review of attachment 8368162 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks :)

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae1b3332213

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ae1b3332213

Comment 23

[Matt Brubeck (:mbrubeck)]

I added some basic docs:
https://wiki.mozilla.org/Buildbot/Talos/Tests#TART.2FCART

...though more detail is welcome, of course.