Bug 1245886 (perf-orange)

[meta] Active performance tool intermittents

RESOLVED FIXED

Status

enhancement
P3
normal
RESOLVED FIXED
3 years ago
4 months ago

People

(Reporter: vporof, Unassigned)

Tracking

(Depends on 4 bugs, {meta})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 obsolete attachments)

(Reporter)

Description

3 years ago
Meta bug to track intermittents that need to be looked at.
(Reporter)

Updated

3 years ago
Alias: perf-orange
(Reporter)

Updated

3 years ago
Depends on: 1245757
(Reporter)

Updated

3 years ago
Depends on: 1245742
(Reporter)

Updated

3 years ago
Depends on: 1244277
(Reporter)

Updated

3 years ago
Depends on: 1243574
(Reporter)

Updated

3 years ago
Depends on: 1243573
(Reporter)

Updated

3 years ago
Depends on: 1243262
(Reporter)

Updated

3 years ago
Depends on: 1242067
(Reporter)

Updated

3 years ago
Depends on: 1241570
(Reporter)

Updated

3 years ago
Depends on: 1241475
(Reporter)

Updated

3 years ago
Depends on: 1240445
(Reporter)

Updated

3 years ago
Depends on: 1240441
(Reporter)

Updated

3 years ago
Depends on: 1239924
(Reporter)

Updated

3 years ago
Depends on: 1239923
(Reporter)

Updated

3 years ago
Depends on: 1239921
(Reporter)

Updated

3 years ago
Depends on: 1239565
(Reporter)

Updated

3 years ago
Depends on: 1238664
(Reporter)

Updated

3 years ago
Depends on: 1238454
(Reporter)

Updated

3 years ago
Depends on: 1238439
(Reporter)

Updated

3 years ago
Depends on: 1238395
(Reporter)

Updated

3 years ago
Depends on: 1238219
(Reporter)

Updated

3 years ago
Depends on: 1238191
(Reporter)

Updated

3 years ago
Depends on: 1238168
(Reporter)

Updated

3 years ago
Depends on: 1238131
(Reporter)

Updated

3 years ago
Depends on: 1237897
(Reporter)

Updated

3 years ago
Depends on: 1237894
(Reporter)

Updated

3 years ago
Depends on: 1237793
(Reporter)

Updated

3 years ago
Depends on: 1237439
(Reporter)

Updated

3 years ago
Depends on: 1236948
(Reporter)

Updated

3 years ago
Depends on: 1236946
(Reporter)

Updated

3 years ago
Depends on: 1236877
(Reporter)

Updated

3 years ago
Depends on: 1236804
(Reporter)

Updated

3 years ago
Depends on: 1236776
(Reporter)

Updated

3 years ago
Depends on: 1236741
(Reporter)

Updated

3 years ago
Depends on: 1236243
(Reporter)

Comment 1

3 years ago
Posted patch v1 (obsolete) — Splinter Review
I'm sorry
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8728424 - Flags: review?(jsantell)
(Reporter)

Comment 2

3 years ago
Posted patch v2 (obsolete) — Splinter Review
Cleaned up events.js a little bit.
Attachment #8728424 - Attachment is obsolete: true
Attachment #8728424 - Flags: review?(jsantell)
Attachment #8728510 - Flags: review?(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)
Attachment #8728510 - Flags: review?(jsantell) → review+
(Reporter)

Updated

3 years ago
Keywords: leave-open
Summary: Active performance tool intermittents → [meta] Active performance tool intermittents
(Reporter)

Comment 4

3 years ago
(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.
(Reporter)

Comment 5

3 years ago
Posted patch v3 (obsolete) — Splinter Review
Final cleanup, added more assertions and listening to more events for the console tests. Landing.
Attachment #8728510 - Attachment is obsolete: true
Attachment #8730254 - Flags: review+
(Reporter)

Updated

3 years ago
Depends on: perf-leak20
(Reporter)

Comment 10

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

Updated

3 years ago
Depends on: perf-e10s-leak
(Reporter)

Comment 12

3 years ago
Triaging. Filter on ADRENOCORTICOTROPIC (yes).
Priority: -- → P1
(Reporter)

Comment 15

3 years ago
Can't work on this right now, busy with Tofino.
Assignee: vporof → nobody
Status: ASSIGNED → NEW
(Reporter)

Updated

3 years ago
Attachment #8731138 - Attachment is obsolete: true
Severity: normal → enhancement
Keywords: meta
Priority: P1 → P3

Updated

10 months ago
Product: Firefox → DevTools
The leave-open keyword is there and there is no activity for 6 months.
:julienw, maybe it's time to close this bug?
Flags: needinfo?(felash)
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.
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Flags: needinfo?(felash)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.