Closed Bug 1320790 Opened 3 years ago Closed 3 years ago

gcli test helpers are slowing down inspector tests

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

It is especially slow when running on linux32 debug or other slow build machines.
And it seems to be used only by one test, a gcli one.
Attached patch patch v1Splinter Review
I'm lost in DAMP!

I don't get why this patch highlight a 5 to 8% win on DAMP, with a remarkable win on netmonitor!
I first thought my base revision on mozilla-central was broken, but I compared again various revisions and the result is the same.
Here is the one I think is the real base revision, the one I have in my local branch:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=05328d3102ef&newProject=try&newRevision=65e61d97720d&framework=1&showOnlyImportant=0&showOnlyConfident=1

But as I used push-to-try --tip, I also compared against whatever was the tip at that time:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=8387a4ada9a5c4cab059d8fafe0f8c933e83c149&newProject=try&newRevision=65e61d97720d81104293ffdf83654c2ecbfa2b8f&framework=1&filter=damp

And also used perfherder automatic base commit finder, which found something on inbound:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=a7a8cd0dd192&newProject=try&newRevision=65e61d97720d&framework=1

I don't see how changes made to /inspector/test/head.js makes any difference on DAMP!
And it is even more surprising to see the exact same difference against various base commits.
Or is it just that my try push ran while the build machines were exceptionally fast?!
I just repushed to try the same commit again and will compare the two runs,
and also pushed an empty commit to compare against it.
I'm afraid that with such behavior I can't use DAMP to highlight the value of all my perf tweaks based on profiler results.
Here is the profiler data that highlights helpers.js while running browser_inspector_initialization.js.
If you expand the tree under "SimpleTest.waitForFocus/..." measured at 421ms,
you end up seeing helpers.js, cli.js and various gcli deps being loaded.

This patch is only usefull on top of bug 1320149, as, otherwise, gcli is loaded by the toolbox anyway. But once we stop loading it by default, I imagine there may be a win to stop loading it in tests.

Now, I can't prove with numbers that this patch brings something.
Should I wait or move forward?
Depends on: 1320149
Brian, Any thought on my experiences around DAMP?
Do you think I should move forward with this patch anyway?

I'm wondering if that could help reduce the timeout intermittents we have on linux32 or windows vm...
Flags: needinfo?(bgrinstead)
Yeah, it's a test only change so it's not going to reflect on DAMP.  If it ends up lowering the # of intermittent timeouts then that would be a great success on its own -- no need to see DAMP changes here.
Flags: needinfo?(bgrinstead)
Assignee: nobody → poirot.alex
Comment on attachment 8815718 [details]
Bug 1320790 - Only inject gcli test helper in the inspect that use it.

https://reviewboard.mozilla.org/r/96560/#review96766
Attachment #8815718 - Flags: review?(pbrosset) → review+
Looks like autoland failed here.
Flags: needinfo?(poirot.alex)
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cd56360c0615
Only inject gcli test helper in the inspect that use it. r=pbro
https://hg.mozilla.org/mozilla-central/rev/cd56360c0615
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Looks like it succeed here, but keeps failing to land in bug 1320084...
Flags: needinfo?(poirot.alex)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.