Bug 1245886 (perf-orange)

[meta] Active performance tool intermittents

NEW
Unassigned

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
P3
enhancement
2 years ago
6 months ago

People

(Reporter: vporof, Unassigned)

Tracking

({leave-open, meta})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 obsolete attachments)

(Reporter)

Description

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

Updated

2 years ago
Alias: perf-orange
(Reporter)

Updated

2 years ago
Depends on: 1245757
(Reporter)

Updated

2 years ago
Depends on: 1245742
(Reporter)

Updated

2 years ago
Depends on: 1244277
(Reporter)

Updated

2 years ago
Depends on: 1243574
(Reporter)

Updated

2 years ago
Depends on: 1243573
(Reporter)

Updated

2 years ago
Depends on: 1243262
(Reporter)

Updated

2 years ago
Depends on: 1242067
(Reporter)

Updated

2 years ago
Depends on: 1241570
(Reporter)

Updated

2 years ago
Depends on: 1241475
(Reporter)

Updated

2 years ago
Depends on: 1240445
(Reporter)

Updated

2 years ago
Depends on: 1240441
(Reporter)

Updated

2 years ago
Depends on: 1239924
(Reporter)

Updated

2 years ago
Depends on: 1239923
(Reporter)

Updated

2 years ago
Depends on: 1239921
(Reporter)

Updated

2 years ago
Depends on: 1239565
(Reporter)

Updated

2 years ago
Depends on: 1238664
(Reporter)

Updated

2 years ago
Depends on: 1238454
(Reporter)

Updated

2 years ago
Depends on: 1238439
(Reporter)

Updated

2 years ago
Depends on: 1238395
(Reporter)

Updated

2 years ago
Depends on: 1238219
(Reporter)

Updated

2 years ago
Depends on: 1238191
(Reporter)

Updated

2 years ago
Depends on: 1238168
(Reporter)

Updated

2 years ago
Depends on: 1238131
(Reporter)

Updated

2 years ago
Depends on: 1237897
(Reporter)

Updated

2 years ago
Depends on: 1237894
(Reporter)

Updated

2 years ago
Depends on: 1237793
(Reporter)

Updated

2 years ago
Depends on: 1237439
(Reporter)

Updated

2 years ago
Depends on: 1236948
(Reporter)

Updated

2 years ago
Depends on: 1236946
(Reporter)

Updated

2 years ago
Depends on: 1236877
(Reporter)

Updated

2 years ago
Depends on: 1236804
(Reporter)

Updated

2 years ago
Depends on: 1236776
(Reporter)

Updated

2 years ago
Depends on: 1236741
(Reporter)

Updated

2 years ago
Depends on: 1236243
Depends on: 1254821
(Reporter)

Comment 1

2 years ago
Created attachment 8728424 [details] [diff] [review]
v1

I'm sorry
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8728424 - Flags: review?(jsantell)
(Reporter)

Comment 2

2 years ago
Created attachment 8728510 [details] [diff] [review]
v2

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

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

Comment 4

2 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

2 years ago
Created attachment 8730254 [details] [diff] [review]
v3

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

2 years ago
Depends on: 1257439
(Reporter)

Comment 10

2 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

2 years ago
Depends on: 1252351
(Reporter)

Comment 12

2 years ago
Triaging. Filter on ADRENOCORTICOTROPIC (yes).
Priority: -- → P1
status-firefox47: fixed → ---
(Reporter)

Comment 15

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

Updated

2 years ago
Attachment #8731138 - Attachment is obsolete: true

Updated

6 months ago
Severity: normal → enhancement
Keywords: meta
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.