Closed
Bug 1245886
(perf-orange)
Opened 8 years ago
Closed 5 years ago
[meta] Active performance tool intermittents
Categories
(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P3)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: vporof, Unassigned)
References
Details
(Keywords: meta)
Attachments
(4 obsolete files)
Meta bug to track intermittents that need to be looked at.
Reporter | ||
Updated•8 years ago
|
Alias: perf-orange
Updated•8 years ago
|
Updated•8 years ago
|
Depends on: perf-e10s-timeout
Reporter | ||
Comment 1•8 years ago
|
||
I'm sorry
Reporter | ||
Comment 2•8 years ago
|
||
Cleaned up events.js a little bit.
Attachment #8728424 -
Attachment is obsolete: true
Attachment #8728424 -
Flags: review?(jsantell)
Attachment #8728510 -
Flags: review?(jsantell)
Comment 3•8 years ago
|
||
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•8 years ago
|
Keywords: leave-open
Summary: Active performance tool intermittents → [meta] Active performance tool intermittents
Reporter | ||
Comment 4•8 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•8 years ago
|
||
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•8 years ago
|
Depends on: perf-tests-reenable
No longer depends on: 1236243, 1236741, 1236776, 1236804, 1236877, 1236946, 1236948, 1237439, 1237793, 1237894, 1237897, 1238131, 1238168, 1238191, 1238219, 1238395, 1238454, 1238664, 1239565, 1239921, 1239923, 1239924, 1240441, 1240445, 1241475, 1242067, 1243262, 1244277, 1245742, 1245757, 1246079, 1246350, 1246351, 1246353, 1246430, 1238439, 1241570, 1243573, 1243574
No longer depends on: 1236243, 1236741, 1236776, 1236804, 1236877, 1236946, 1236948, 1237439, 1237793, 1237894, 1237897, 1238131, 1238168, 1238191, 1238219, 1238395, 1238454, 1238664, 1239565, 1239921, 1239923, 1239924, 1240441, 1240445, 1241475, 1242067, 1243262, 1244277, 1245742, 1245757, 1246079, 1246350, 1246351, 1246353, 1246430, 1238439, 1241570, 1243573, 1243574
Reporter | ||
Comment 6•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/220192149ce4f79c75611295db062f8fd5933b8b Bug 1245886 - Refactor performance tests, r=jsantell
Reporter | ||
Comment 7•8 years ago
|
||
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
Attachment #8730254 -
Attachment is obsolete: true
Attachment #8731138 -
Flags: review+
Reporter | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4116cafbb87dd251990450f8f90911beecbe0859 Bug 1245886 - Force GC at the end of all tests, r=me
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/220192149ce4
Reporter | ||
Updated•8 years ago
|
Depends on: perf-leak20
Reporter | ||
Comment 10•8 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
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4116cafbb87d
Reporter | ||
Updated•8 years ago
|
Depends on: perf-e10s-leak
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8671dfbbff2d
Comment 14•8 years ago
|
||
bugherder uplift |
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
status-firefox47:
--- → fixed
Updated•8 years ago
|
status-firefox47:
fixed → ---
Reporter | ||
Comment 15•8 years ago
|
||
Can't work on this right now, busy with Tofino.
Assignee: vporof → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•8 years ago
|
Attachment #8731138 -
Attachment is obsolete: true
Updated•6 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 16•5 years ago
|
||
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)
Comment 17•5 years ago
|
||
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
Closed: 5 years ago
Flags: needinfo?(felash)
Resolution: --- → FIXED
Updated•5 years ago
|
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•