Closed
Bug 1419533
Opened 7 years ago
Closed 7 years ago
Add performance test for inspector layout panel
Categories
(DevTools :: Inspector, enhancement, P3)
Tracking
(firefox59 fixed)
RESOLVED
FIXED
Firefox 59
| Tracking | Status | |
|---|---|---|
| firefox59 | --- | fixed |
People
(Reporter: jdescottes, Assigned: jdescottes)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
Follow up to Bug 1403871. Add a new DAMP test to cover the perf improvement from this bug. Initial STRs: - open the inspector - switch to layout panel - close inspector - go to http://searchfox.org/mozilla-central/rev/f54c1723befe6bcc7229f005217d5c681128fcad/dom/canvas/CanvasRenderingContext2D.cpp#5489 - open the inspector
| Comment hidden (mozreview-request) |
|
||
Comment 2•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8930632 [details] Bug 1419533 - Add talos test for devtools inspector layout tab; https://reviewboard.mozilla.org/r/201758/#review207428 Looks good, thanks for the test! Would you mind if I start trying to merge that on top of custom test page later? ::: testing/talos/talos/tests/devtools/addon/content/damp.js:428 (Diff revision 1) > + sendSyncMessage("setup-test-done"); > + }); > + }` > + ) + ")()", false); > + messageManager.addMessageListener("setup-test-done", resolve); > + messageManager.sendAsyncMessage("setup-test"); "setup-test" event is unecessary (no need to delay creation of nodes), you can only rely on "setup-test-done". ::: testing/talos/talos/tests/devtools/addon/content/damp.js:445 (Diff revision 1) > + start = performance.now(); > + yield this.closeToolbox(null); > + this._results.push({ > + name: "inspector.layout.close", > + value: performance.now() - start > + }); Is it any useful? Did that perf issue happen to also slow down toolbox close? I would prefer to ignore toolbox close as it is very noisy today.
Attachment #8930632 -
Flags: review?(poirot.alex) → review+
|
Assignee | |
Comment 3•7 years ago
|
||
Thanks for the review! (In reply to Alexandre Poirot [:ochameau] from comment #2) > Comment on attachment 8930632 [details] > Bug 1419533 - Add talos test for devtools inspector layout tab; > > https://reviewboard.mozilla.org/r/201758/#review207428 > > Looks good, thanks for the test! > > Would you mind if I start trying to merge that on top of custom test page > later? Do you mean trying to include that test case in the custom page? Sure! I was hesitant about it, since I needed quite a lot of nodes to trigger the slowdown compared to the ~100 we currently have in the custom test page. > > ::: testing/talos/talos/tests/devtools/addon/content/damp.js:428 > (Diff revision 1) > > + sendSyncMessage("setup-test-done"); > > + }); > > + }` > > + ) + ")()", false); > > + messageManager.addMessageListener("setup-test-done", resolve); > > + messageManager.sendAsyncMessage("setup-test"); > > "setup-test" event is unecessary (no need to delay creation of nodes), you > can only rely on "setup-test-done". > Ah good catch! > ::: testing/talos/talos/tests/devtools/addon/content/damp.js:445 > (Diff revision 1) > > + start = performance.now(); > > + yield this.closeToolbox(null); > > + this._results.push({ > > + name: "inspector.layout.close", > > + value: performance.now() - start > > + }); > > Is it any useful? > Did that perf issue happen to also slow down toolbox close? > > I would prefer to ignore toolbox close as it is very noisy today. Well when testing locally in Bug 1403871, it was slowing down the close, but I have not reproduced that with this test. Maybe because my local test was with a much bigger page (80k nodes I think). I will remove it.
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 5•7 years ago
|
||
ongoing compare against baseline: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=060e4a803aa0c6c508ea5debdb2bff0309e9b795&newProject=try&newRevision=38a18e731b6c619fc573849637991a3f7b837a0f&framework=1 Expect DAMP summary to increase since we are adding a new test.
| Comment hidden (mozreview-request) |
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ea934ee0f03d Add talos test for devtools inspector layout tab;r=ochameau
|
||
Comment 8•7 years ago
|
||
Backed out for ESlint failures on /devtools/addon/content/damp.js https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9f16ad3a6352c90cb3613b69e42cf96913a173d1 https://hg.mozilla.org/integration/autoland/rev/9f16ad3a6352c90cb3613b69e42cf96913a173d1 Log: https://treeherder.mozilla.org/logviewer.html#?job_id=148105507&repo=autoland&lineNumber=247
Flags: needinfo?(jdescottes)
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Comment 10•7 years ago
|
||
Sorry about that! We switched from Task to async/await since I first sent this to review and needed to update the code.
Flags: needinfo?(jdescottes)
|
||
Comment 11•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a431116645e6 Add talos test for devtools inspector layout tab;r=ochameau
|
||
Comment 12•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/a431116645e6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
|
|
||
Comment 13•7 years ago
|
||
| bugherder landing | ||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a431116645e6
|
||
Comment 14•7 years ago
|
||
I have reproduced this bug with Nightly 59.0a1 (2017-11-21) on Windows 8.1 , 64 Bit ! This bug's fix is Verified with latest Nightly ! Build ID 20171201100115 User Agent Mozilla/5.0 (Windows NT 6.3; WOW64; rv:59.0) Gecko/20100101 Firefox/59.0 [bugday-20171129]
|
||
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•