1245886 - (perf-orange) [meta] Active performance tool intermittents

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P3)

Description

[Victor Porof [:vporof][:vp]]

Meta bug to track intermittents that need to be looked at.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: v1

I'm sorry

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: v2

Cleaned up events.js a little bit.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8728510 [details] [diff] [review]
v2

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

Wow. Looks great. Overall, r+ -- I looked at everything, but if anything needs a closer look, let me know. For the currently disabled tests, let's do follow ups and I can help out too, and get this giant patch landed. Should be ok as we're not landing any other changes to this yet.

VERY EXCITING

::: devtools/client/performance/performance-controller.js
@@ -266,5 @@
> -    // Emit another stop event here, as a lot of tests use
> -    // the RECORDING_STOPPED event, but in the case of a UI click on a button,
> -    // the RECORDING_STOPPED event happens from the server, where this request may
> -    // not have yet finished, so listen to this in tests that fail because the `stopRecording`
> -    // request is not yet completed. Should only be used in that scenario.

Oh my god I forgot about this

@@ +416,5 @@
>     * Stores a recording internally.
>     *
>     * @param {PerformanceRecordingFront} recording
>     */
> +  _addRecordingIfUnknown: function (recording) {

Not a fan of this method name, but I don't care too much.

::: devtools/client/performance/test/browser.ini
@@ +31,5 @@
> +[browser_perf-details-04-toolbar-buttons.js]
> +[browser_perf-details-05-preserve-view.js]
> +[browser_perf-details-06-rerender-on-selection.js]
> +[browser_perf-details-07-bleed-events.js]
> +# [browser_perf-details-gc-snap.js] TODO

Add bug number bug 1161817

@@ -83,5 @@
>  [browser_perf-overview-selection-02.js]
>  [browser_perf-overview-selection-03.js]
>  [browser_perf-overview-time-interval.js]
>  [browser_perf-private-browsing.js]
> -skip-if = os == 'linux' # bug 1210140

These work on Linux now? nice

::: devtools/client/performance/test/browser_perf-docload.js
@@ +22,2 @@
>    yield startRecording(panel);
> +  yield reload(target);

way better

::: devtools/client/performance/test/head.js
@@ +10,5 @@
> +// Performance tests are much heavier because of their reliance on the
> +// profiler module, memory measurements, frequent canvas operations etc. Many of
> +// of them take longer than 30 seconds to finish on try server VMs, even though
> +// they superficially do very little.
> +requestLongerTimeout(2);

Yuck -- is there any downside to this if a test does not need to take longer?

@@ +45,5 @@
> +
> +  DevToolsUtils.testing = true;
> +
> +  // Make sure all the prefs are reverted to their defaults once tests finish.
> +  let stopObservingPrefs = PrefUtils.whenUnknownPrefChanged("devtools.performance", pref => {

Nice

::: devtools/client/performance/test/helpers/event-utils.js
@@ +1,1 @@
> +/* Any copyright is dedicated to the Public Domain.

Most of these could probably live in devtools/client/framework/test/shared-head, or maybe a new devtools/client/testing/*.js modules, as these seem pretty handy to be used elsewhere. (Follow up)

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Jordan Santell [:jsantell] [@jsantell] (Please needinfo) from comment #3)
> 
> ::: devtools/client/performance/test/browser.ini
> @@ +31,5 @@
> > +[browser_perf-details-04-toolbar-buttons.js]
> > +[browser_perf-details-05-preserve-view.js]
> > +[browser_perf-details-06-rerender-on-selection.js]
> > +[browser_perf-details-07-bleed-events.js]
> > +# [browser_perf-details-gc-snap.js] TODO
> 
> Add bug number bug 1161817

Will file a new bug for re-enabling these.

> 
> ::: devtools/client/performance/test/head.js
> @@ +10,5 @@
> > +// Performance tests are much heavier because of their reliance on the
> > +// profiler module, memory measurements, frequent canvas operations etc. Many of
> > +// of them take longer than 30 seconds to finish on try server VMs, even though
> > +// they superficially do very little.
> > +requestLongerTimeout(2);
> 
> Yuck -- is there any downside to this if a test does not need to take longer?
> 

I don't see any downsides other than accidentally writing long tests. But we already have this issue, as most of our tests take longer than usual to finish.

Comment 5

[Victor Porof [:vporof][:vp]]

Attachment: v3

Final cleanup, added more assertions and listening to more events for the console tests. Landing.

Comment 6

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/220192149ce4f79c75611295db062f8fd5933b8b
Bug 1245886 - Refactor performance tests, r=jsantell

Comment 7

[Victor Porof [:vporof][:vp]]

Attachment: v4

Added commit message and landed: https://hg.mozilla.org/integration/fx-team/rev/220192149ce4

Final try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4a00b4aea8e&group_state=expanded

Comment 8

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/4116cafbb87dd251990450f8f90911beecbe0859
Bug 1245886 - Force GC at the end of all tests, r=me

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/220192149ce4

Comment 10

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/8671dfbbff2dfa2fa6c410f4c0799f4b7c2e7484
Bug 1245886 - Manually stop the profiler module at the end of all tests, r=me

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4116cafbb87d

Comment 12

[Victor Porof [:vporof][:vp]]

Triaging. Filter on ADRENOCORTICOTROPIC (yes).

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8671dfbbff2d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9539d658b1a9
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc74c5ca31af
https://hg.mozilla.org/releases/mozilla-aurora/rev/60828c17117a

Comment 15

[Victor Porof [:vporof][:vp]]

Can't work on this right now, busy with Tofino.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:julienw, maybe it's time to close this bug?

Comment 17

[Julien Wajsberg [:julienw]]

We do have some intermittent bugs but we have other ways to track them (especially being bugged about them every week, but also through keywords). Let's close this bug.