Closed Bug 1441703 Opened 6 years ago Closed 6 years ago

Split DAMP test in smaller tests

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 7 open bugs)

Details

Attachments

(2 files)

Bug 1441703 - Split DevTools performance test damp.js;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1441703 - Define all DAMP tests in a single file;
59 bytes, text/x-review-board-request
ochameau
: review+
Details 
Attempt to have a single file per test for DAMP.
Here's an attempt for splitting this at: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=4686e0765a74ee170d185a32063b8943bc45588b

This makes damp.js mostly responsible for running tests. Shared helpers are in content/helpers/, tests are in content/tests/

There is still a lot left to be desired, but I want to see if this behaves fine on talos, and if it does, ask for feedback before spending more time on this.
Comment on attachment 8954580 [details]
Bug 1441703 - Split DevTools performance test damp.js;

Still a work in progree, let me know what you think! Not sure why my change impact the runtimes of some of the tests yet.

There is a bunch of things I'm not happy with here, mostly having to pass a "damp" instance around. I could most likely store the single instance in a helper and avoid passing it everywhere, but since I'm not sure using devtools' require for loading those files is a good approach here I prefer to let you have a look before diving further.
Attachment #8954580 - Flags: feedback?(poirot.alex)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #4)
> Comment on attachment 8954580 [details]
> Bug 1441703 - Split damp into smaller tests
> 
> Still a work in progree, let me know what you think!

Looks fantastic! Thanks a lot for picking that up.

> Not sure why my change impact the runtimes of some of the tests yet.

I wish we could understand... DAMP is still subject to lot of vaguely explained changes.
I'm wondering if we get extra noise in whole DAMP because of memory tool tests.
If it somehow measures memory of DAMP ressources by collecting memory data.
DAMP ressources may be really different with additional modules, compartments, wrappers, ...

I could expect an improvement on cold.inspector.open because we are loading Loader.jsm and its deps before we run the test.
But it seems to have very little impact on it.

Otherwise I don't have a clue. One way to investigate that would be to disable a set of tests one by one,
and push to try with the same tests disabled with and without your patch.
Just to see if one particular test brings a lot of noise. May be it can be done easily via the boolean flags from damp.html and *a tons* of try pushed?

> There is a bunch of things I'm not happy with here,

Do you have a list? It looks like a pretty good iteration so far.

> mostly having to pass a "damp" instance around.

It doesn't look so terrible.

> I could most likely store the single instance in a
> helper and avoid passing it everywhere, but since I'm not sure using
> devtools' require for loading those files is a good approach here

That's very interesting approach, but I wasn't expecting this particular one.
I had in mind to use modules rather that plain js file like mochitests and xpcshell.
So that's a very good choice and you should keep using modules. (I wish we could make our mochitest use modules...)
But I was expecting a custom loader, like server one:
  https://searchfox.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#152
Now that I saw your solution, I find it quite appealing.
I'm not sure what would be the benefit of using a custom one.
The ability to use custom set of paths only in the DAMP instance?

Note that you could add an alias, like xpcshell-test here to prevent having to use absolute chrome urls:
  https://searchfox.org/mozilla-central/source/devtools/shared/Loader.jsm#43


Regarding the patch itself, I have a couple of random comments:
* given that all files from /helpers/ are only used by files from /tests/,
I would move helpers into tests folder. Or may be one folder per tool may be useful and put the helper next to the tests?
* before this I would suggest cleaning up all tests to use runTest instead of populating _results manually,
* do not expose `Damp` object as-is. It would be great to expose an object with just what the test uses: runTest, garbageCollect, reloadPage, testSetup, get*Url, testTeardown. (It may be easier to do if damp becomes a module.)
* there is surely things to improve as a followup in startTest and damp.html, but it sounds more related to .ini file support, so I like your simple approach you are using.
* I must warn you that patches are about to land in damp.js, bug 1419350 should land shortly and bug 1154874 should also land very soon.
Attachment #8954580 - Flags: feedback?(poirot.alex) → feedback+
Thanks for the feedback! I'll wait for both bugs you mentioned to land before resuming this.

> Do you have a list? It looks like a pretty good iteration so far.

On top of what I mentioned:
- a test is still referenced in too many places (twice in damp.html, once in damp.js)
- I would like to align test filenames and test names. But I would also like all test files for a given tool to start by the name of the tool. Which means renaming custom.inspector to inspector.custom, I don't think this would have an impact on the actual metrics we track, but I need to check.
- I had to keep a helper inside of damp.js (the reloadPage function). If I put it in a module, then I couldn't get the "load" event on the browser element.
- I am still getting URLs from damp.js, (getSimpleUrl etc...), I think I can move them to a helper but I had an issue when I tried in the beginning, so I left it as is in the patch you looked at.

> I could expect an improvement on cold.inspector.open because we are loading Loader.jsm and 
> its deps before we run the test.
> But it seems to have very little impact on it.

That's a good point, I didn't consider that. We have to hope that Loader.jsm has a small enough impact to be ignored here.

> But I was expecting a custom loader, like server one:
>   https://searchfox.org/mozilla-central/source/devtools/client/framework/ToolboxProcess.jsm#152
> Now that I saw your solution, I find it quite appealing.
> I'm not sure what would be the benefit of using a custom one.
> The ability to use custom set of paths only in the DAMP instance?

The main reason I used require here is that it "just worked" :) I guess if we create a new separate loader for the tests, it shouldn't have a huge impact, but we would need to use the "other" require anytime we want to get a file that interacts with the toolbox (so everything I have in toolbox-helpers right now). Maybe it would reduce the impact on the performance of the tests themselves?

> Note that you could add an alias, like xpcshell-test here to prevent having to use absolute chrome urls:
>  https://searchfox.org/mozilla-central/source/devtools/shared/Loader.jsm#43

Nice, I will look into that!

> * given that all files from /helpers/ are only used by files from /tests/,
>  I would move helpers into tests folder. Or may be one folder per tool may be useful and put the helper next to the tests?

I totally agree, I felt the same afterwards. I'll do that in the next iteration.

> * before this I would suggest cleaning up all tests to use runTest instead of populating _results manually,

The only test not using runTest is console.streamlog, but it is computing the "value" from the content page itself. This means we should slightly update the runTest/done API to support tests passing custom values? Or propose an alternative API, eg. recordValue?. It's a bit artificial to force the test to call first runTest() and then done() here, but it would help creating accurate markers for the perf.html profile, and would keep things relatively consistent. We could document it as "Even if your test is passing custom values, try to call runTest() before starting the activity you measure, and test.done() right after, so that we can keep consistent tests and helpful perf.html profiles".

> * do not expose `Damp` object as-is. It would be great to expose an object with just what the test uses: runTest, garbageCollect, reloadPage, testSetup, get*Url, testTeardown. (It may be easier to do if damp becomes a module.)

Agreed, we should have a limited and well defined API that tests can use.

> * there is surely things to improve as a followup in startTest and damp.html, but it sounds more related 
> to .ini file support, so I like your simple approach you are using.

That's related to one of the remaining things I want to address, the fact that a test declaration is spread all over the place. And I haven't looked at damp.html at all for now. We can probably do that in a second step.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Rebased the patch and implemented the feedback from Alex's comment. On top of the previous patch, I worked on:
- a shared head.js module to avoid passing damp around
- tests are declared in a single file "damp-tests.js"

Started a talos comparison at https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=71f3d43c726de65acc3a01f3e00c34ad99e6fef4&newProject=try&newRevision=1eee935b32223af8bff05774401f69e2a91d435a&framework=1
Comment on attachment 8954580 [details]
Bug 1441703 - Split DevTools performance test damp.js;

https://reviewboard.mozilla.org/r/223664/#review232456

Looks good, thanks a lot for this!
I made a lot of comments that are good candidates for followups.
My only concern I would like to see addressed before landing is about test names.
See next paragraph.

(In reply to Julian Descottes [:jdescottes][:julian] from comment #6)
> - I would like to align test filenames and test names. But I would also like
> all test files for a given tool to start by the name of the tool. Which
> means renaming custom.inspector to inspector.custom, I don't think this
> would have an impact on the actual metrics we track, but I need to check.

It is weird to have the name in damp-tests.js not matching the one in perfherder.
As-is, I find it more confusing than helpful.
Do you plan to followup to change the test names on talos to match the new convention you are using?
I'm not sure we can rename without loosing the data. But may be. That would be a question for :jmaher.
In the meantime I would revert to original test names, to match perfherder ones and keep the same names when using --subtest command argument.

> > * before this I would suggest cleaning up all tests to use runTest instead of populating _results manually,
> 
> The only test not using runTest is console.streamlog, but it is computing
> the "value" from the content page itself. This means we should slightly
> update the runTest/done API to support tests passing custom values? Or
> propose an alternative API, eg. recordValue?. It's a bit artificial to force
> the test to call first runTest() and then done() here, but it would help
> creating accurate markers for the perf.html profile, and would keep things
> relatively consistent. We could document it as "Even if your test is passing
> custom values, try to call runTest() before starting the activity you
> measure, and test.done() right after, so that we can keep consistent tests
> and helpful perf.html profiles".

I didn't realized this test was special.
I saw your logTestResult helper, it looks good.

::: testing/talos/talos/tests/devtools/addon/content/tests/debugger/complicated.js:9
(Diff revision 4)
> +
> +"use strict";
> +
> +const { closeToolboxAndLog } = require("chrome://damp/content/tests/toolbox/toolbox-helpers");
> +const { openDebuggerAndLog, reloadDebuggerAndLog } = require("chrome://damp/content/tests/debugger/debugger-helpers");
> +const { testSetup, testTeardown, COMPLICATED_URL } = require("chrome://damp/content/tests/head");

What about using relative paths everywhere?

::: testing/talos/talos/tests/devtools/addon/content/tests/debugger/complicated.js:24
(Diff revision 4)
> +  let toolbox = await openDebuggerAndLog("complicated", EXPECTED);
> +  await reloadDebuggerAndLog("complicated", toolbox, EXPECTED);
> +  await closeToolboxAndLog("complicated.jsdebugger", toolbox);
> +
> +  await testTeardown();
> +};

Don't you want to have mochitests written that way now ? :-p

::: testing/talos/talos/tests/devtools/addon/content/tests/head.js:38
(Diff revision 4)
> +  return damp.reloadPage(onReload);
> +};
> +
> +exports.runTest = function(label) {
> +  return damp.runTest(label);
> +};

Comment for followups:
`runTest` naming sounds weird now that I see it in this head file. It looks like it allows to run a test file, but not really. But I don't have any better name in mind.

::: testing/talos/talos/tests/devtools/addon/content/tests/head.js:50
(Diff revision 4)
> +  return damp.testTeardown();
> +};
> +
> +exports.logTestResult = function(name, value) {
> +  damp._results.push(name, value);
> +};

It looks weird to have this module be only wrappers around damp.

* we could move `reloadPage` implementation within head.js. Same for `garbageCollect`, but it is used from damp.js...
* I'm wondering if we should only use modules and also make damp.js be a module?
* but runTest, testSetup, testTeardown and logTestResult are all specific to `Damp`, specific to the test runner. So it makes sense to delegate to Damp and keep Damp object to focus on the test runner itself.

This is good candidate for a followup.

::: testing/talos/talos/tests/devtools/addon/content/tests/toolbox/toolbox-helpers.js:3
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Two comments about this module:
- its location looks weird, especially today with just panels-in-background test in toolbox folder. I'm wondering if it should be next to head.js
- its method are used in all tests, like head.js symbols. In mochitests, we would have put such helpers into head.js, or used loadSubScript to expose them easily. Now I imagine you want to avoid 10k lines modules and it makes sense... May be we could do that within head.js:
const { openToolboxAndLog, ... } = require(".../toolbox-helpers");
exports.openToolboxAndLog = openToolboxAndLog;
exports.[...] = [...];

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/bulklog.js:13
(Diff revision 4)
> +const { runTest, testSetup, testTeardown, SIMPLE_URL } = require("chrome://damp/content/tests/head");
> +
> +const getMostRecentWindow = function() {
> +  const Services = require("Services");
> +  return Services.wm.getMostRecentWindow("navigator:browser");
> +};

* This method is used in bulklog, objectexpand and toolbox-helpers, May be we should export it from toolbox-helpers? or head.js?
* Any particular reason to require Services from module's header? (this is a pseudo module so there isn't much value in lazy loading it. also it is most likely already loaded by firefox)
* I would have called this helper getBrowserWindow or getMostRecentBrowserWindow. Just to highlight that it is firefox top level window and no any content, nor toolbox, nor panel one.

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/bulklog.js:51
(Diff revision 4)
> +  ) + ")()", true);
> +
> +  // Kick off the logging
> +  messageManager.sendAsyncMessage("do-logs");
> +
> +  let test = runTest("console.bulklog");

I didn't realized but `messageManager.sendAsyncMessage("do-logs");` should probably be execute after `runTest` call. But this should be done in a followup.

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/custom.js:15
(Diff revision 4)
> +
> +module.exports = async function() {
> +  // These numbers controls the number of console api calls we do in the test
> +  let sync = 250, stream = 250, async = 250;
> +  let params = `?sync=${sync}&stream=${stream}&async=${async}`;
> +  let url = CUSTOM_URL.replace(/\$TOOL/, "console") + params;

Now that CUSTOM_URL is defined somewhere else, this replace trick looks non obvious. May be CUSTOM_URL could be `PAGES_BASE_URL + "custom/"` and each test create its own url from it. This is also a good candidate for followup.

::: testing/talos/talos/tests/devtools/addon/content/tests/webconsole/objectexpand.js:48
(Diff revision 4)
> +  ) + ")()", true);
> +
> +  // Kick off the logging
> +  messageManager.sendAsyncMessage("do-dir");
> +
> +  let test = runTest("console.objectexpand");

Same comment than bulklog about `runTest` and `sendAsyncMessage` call order.
Attachment #8954580 - Flags: review?(poirot.alex) → review+
Comment on attachment 8957478 [details]
Bug 1441703 - Define all DAMP tests in a single file;

https://reviewboard.mozilla.org/r/226386/#review232318

Actually, my previous comment about test names is related to this patch rather than the first one.

::: testing/talos/talos/tests/devtools/addon/content/damp.js
(Diff revision 1)
> -        if (!name.includes(filter)) {
> -          delete tests[name];
> -        }
> -      }
> -      if (Object.keys(tests).length == 0) {
> -        dump("ERROR: Unable to find any test matching '" + filter + "'\n");

Could you keep this warning message when no test matched?
Attachment #8957478 - Flags: review?(poirot.alex) → review+
Thanks for the reviews, Alex! I'll file follow ups & address the comments. Let's discuss about the naming issue.

(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Comment on attachment 8954580 [details]
> Bug 1441703 - Split DevTools performance test damp.js;
> 
> https://reviewboard.mozilla.org/r/223664/#review232456
> 
> Looks good, thanks a lot for this!
> I made a lot of comments that are good candidates for followups.
> My only concern I would like to see addressed before landing is about test
> names.
> See next paragraph.
> 
> (In reply to Julian Descottes [:jdescottes][:julian] from comment #6)
> > - I would like to align test filenames and test names. But I would also like
> > all test files for a given tool to start by the name of the tool. Which
> > means renaming custom.inspector to inspector.custom, I don't think this
> > would have an impact on the actual metrics we track, but I need to check.
> 
> It is weird to have the name in damp-tests.js not matching the one in
> perfherder.
> As-is, I find it more confusing than helpful.
> Do you plan to followup to change the test names on talos to match the new
> convention you are using?
> I'm not sure we can rename without loosing the data. But may be. That would
> be a question for :jmaher.
> In the meantime I would revert to original test names, to match perfherder
> ones and keep the same names when using --subtest command argument.
> 

That's a good point. I tried having something consistent across all tests, so that adding a new test would be easy. Same as you, I assumed changing the test labels would imply data loss, so I discarded this option. 

The test labels don't follow a clear pattern today: 
- we have "webconsole" and "console" in the perfherder labels
- the tests named "*.debugger" use labels containing ".jsdebugger"
- the memory test uses the labels "*.memory", "*.saveHeapSnapshot", "*.readHeapSnapshot", but is named "*.saveAndReadHeapSnapshot" (so none of the above :) )
- the tool name is sometimes the first part of the label ("inspector.layout"), sometimes the last part ("simple.inspector")

This is why I didn't want to follow them, but we can find a compromise. I propose to:
- swap "saveAndReadHeapSnapshot" for "memory": the test name doesn't match any label today anyway, and for filtering purposes "memory" seems like a better term
- rename the "debugger" tests to "jsdebugger" to match perfherder
- keep the simple and complicated tests in dedicated folders so that we can have simple.inspector match the test file "simple/inspector.js" (I think it's important to make adding new tests painless, not having to worry about filenames and file structure is important IMO)
- for webconsole and console I have no solution, we will have simple/webconsole.js and console/bluklog.js which is a bit inconsistent but that's not a huge issue

Let me know if that sounds ok for you!
Flags: needinfo?(poirot.alex)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #15)
> The test labels don't follow a clear pattern today: 
> - we have "webconsole" and "console" in the perfherder labels
> - the tests named "*.debugger" use labels containing ".jsdebugger"
> - the memory test uses the labels "*.memory", "*.saveHeapSnapshot",
> "*.readHeapSnapshot", but is named "*.saveAndReadHeapSnapshot" (so none of
> the above :) )
> - the tool name is sometimes the first part of the label
> ("inspector.layout"), sometimes the last part ("simple.inspector")

I didn't realized how much it was inconsistant!

> This is why I didn't want to follow them, but we can find a compromise. I
> propose to:
> - swap "saveAndReadHeapSnapshot" for "memory": the test name doesn't match
> any label today anyway, and for filtering purposes "memory" seems like a
> better term
> - rename the "debugger" tests to "jsdebugger" to match perfherder
> - keep the simple and complicated tests in dedicated folders so that we can
> have simple.inspector match the test file "simple/inspector.js" (I think
> it's important to make adding new tests painless, not having to worry about
> filenames and file structure is important IMO)
> - for webconsole and console I have no solution, we will have
> simple/webconsole.js and console/bluklog.js which is a bit inconsistent but
> that's not a huge issue

Sounds good!

We can ping Talos expert after that to try to rename tests without loosing data.
Worse case scenario we can include something in devtools dashboard to accomodate the test rename,
I already had to do that when they flagged the tests differently when stylo became the default.
Flags: needinfo?(poirot.alex)
Blocks: 1444819
Blocks: 1444820
Blocks: 1444821
Blocks: 1444823
Blocks: 1444824
Blocks: 1444826
Blocks: 1444833
Blocks: 1444836
Comment on attachment 8954580 [details]
Bug 1441703 - Split DevTools performance test damp.js;

https://reviewboard.mozilla.org/r/223664/#review232456

> What about using relative paths everywhere?

I logged a follow up to address that: https://bugzilla.mozilla.org/show_bug.cgi?id=1444821

We don't use relative paths a lot in devtools, so I'm not sure we should do it here. But maybe define a custom path as you suggested earlier. I temporarily did it and it was looking ok but I'd prefer to discuss it in its own bug.

> Comment for followups:
> `runTest` naming sounds weird now that I see it in this head file. It looks like it allows to run a test file, but not really. But I don't have any better name in mind.

Good point, logged https://bugzilla.mozilla.org/show_bug.cgi?id=1444820

> It looks weird to have this module be only wrappers around damp.
> 
> * we could move `reloadPage` implementation within head.js. Same for `garbageCollect`, but it is used from damp.js...
> * I'm wondering if we should only use modules and also make damp.js be a module?
> * but runTest, testSetup, testTeardown and logTestResult are all specific to `Damp`, specific to the test runner. So it makes sense to delegate to Damp and keep Damp object to focus on the test runner itself.
> 
> This is good candidate for a followup.

I logged https://bugzilla.mozilla.org/show_bug.cgi?id=1444836 

I think the runner could definitely either be in head.js or in its own module, whatever feels most logical. I had issues with moving reloadPage to a module loaded via require. I could not get the "load" event on the browser for some reason... I didn't investigate too much, we will see about that in the follow up.

> Two comments about this module:
> - its location looks weird, especially today with just panels-in-background test in toolbox folder. I'm wondering if it should be next to head.js
> - its method are used in all tests, like head.js symbols. In mochitests, we would have put such helpers into head.js, or used loadSubScript to expose them easily. Now I imagine you want to avoid 10k lines modules and it makes sense... May be we could do that within head.js:
> const { openToolboxAndLog, ... } = require(".../toolbox-helpers");
> exports.openToolboxAndLog = openToolboxAndLog;
> exports.[...] = [...];

Good point, merged it with head.js for now. It's not that much code anyway for now, we can split again later.
The imports are quite massive now however in the tests themselves:
  const { openToolbox, closeToolboxAndLog, getBrowserWindow, runTest, testSetup,
        testTeardown, SIMPLE_URL } = require("chrome://damp/content/tests/head");
        
I am wondering if we shouldn't simply do
  const head = require("chrome://damp/content/tests/head");
  
And do head.runTest etc... We usually deconstruct when importing but here, it's not really nice to read.

> * This method is used in bulklog, objectexpand and toolbox-helpers, May be we should export it from toolbox-helpers? or head.js?
> * Any particular reason to require Services from module's header? (this is a pseudo module so there isn't much value in lazy loading it. also it is most likely already loaded by firefox)
> * I would have called this helper getBrowserWindow or getMostRecentBrowserWindow. Just to highlight that it is firefox top level window and no any content, nor toolbox, nor panel one.

Good point! Implemented all suggestions here.

> I didn't realized but `messageManager.sendAsyncMessage("do-logs");` should probably be execute after `runTest` call. But this should be done in a followup.

Logged https://bugzilla.mozilla.org/show_bug.cgi?id=1444823

> Now that CUSTOM_URL is defined somewhere else, this replace trick looks non obvious. May be CUSTOM_URL could be `PAGES_BASE_URL + "custom/"` and each test create its own url from it. This is also a good candidate for followup.

Agreed. I simply export PAGES_BASE_URL url now and do for instance:
  let url = PAGES_BASE_URL + "custom/console/index.html" + params;
Comment on attachment 8957478 [details]
Bug 1441703 - Define all DAMP tests in a single file;

https://reviewboard.mozilla.org/r/226386/#review232318

> Could you keep this warning message when no test matched?

Good catch, I forgot about this! Also logged https://bugzilla.mozilla.org/show_bug.cgi?id=1444826 to work on error logging for DAMP.
All comments should now be addressed or moved to follow ups. Retriggered talos comparisons.

Regarding the naming I ended up going against what I proposed. After moving all the simple/complicated/custom tests to dedicated folders, I was left with only 3 "tool" folders (toolbox, inspector and console), the helpers had been moved to a helpers folder again. Overall it felt less clear and organized. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1444824 to see if we can rename the labels.

This means that I kept the current test names, except for the memory tests which are now *.memory. I didn't rename debugger tests to jsdebugger because our devtools folder for the debugger is called "debugger" and not "jsdebugger" and I feel like it's better to be consistent with the names of our tool folders in devtools/.
Looks like there have been two bugs modifying damp.js again in the last few days:
- bug 1443964
- bug 1434849

Needs rebase.
Now rebased on the latest central, backported Bug 1443964 and Bug 1434849. Waiting for talos comparisons before landing (try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb1d64bd4f4ac82a87ae6b8b13f4413c479dd599)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a5c57836317
Split DevTools performance test damp.js;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a4d12775fbf5
Define all DAMP tests in a single file;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/6a5c57836317
https://hg.mozilla.org/mozilla-central/rev/a4d12775fbf5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
I have been monitoring the results on http://firefox-dev.tools/performance-dashboard/

It is possible that the split had a global impact on our tests. Approximate numbers, I just guessed them from looking at the charts.

inspector:
- cold open: maybe 2% faster, not significant
- complicated open: not significant
- complicated reload: 8% faster
- complicated close: 4% faster?
- custom open: 3% faster
- custom reload: 4% faster
- custom select node: 4% faster
- custom deselect node: 5% faster
- custom close: 3% faster
- simple open: 2% faster
- simple reload: 2% faster
- simple close: 5% faster
- mutations: not significant
- layout: maybe 1/2% faster?

console:
- complicated open: 2% faster 
- complicated reload: 3% faster
- complicated close: 15% faster
- custom open: not significant
- custom reload: 2% faster
- custom close: too noisy to tell
- simple open: 3% faster
- simple reload: not significant
- simple close: 4% faster
- object expand: 4% faster
- object expanded: not significant
- open with cache: 8% faster
- bulklog: 3% faster
- streamlog: no impact (but capped at 16ms from RAF)

netmonitor:
- complicated open: maybe 1% faster?
- complicated reload: 2% faster
- complicated requests finished: 2% faster
- complicated close: 2% faster
- simple open: 2% faster?
- simple reload: 5% faster
- simple requests finished: 7% faster
- simple close: 8% faster

debugger:
- complicated open: 2% faster
- complicated reload: 3% faster
- complicated close: no significant impact
- custom open: 4% faster
- custom reload: 3% faster
- custom pause: 4% faster
- custom step in: no significant impact?
- custom step over: 4% faster
- custom step out: 5% faster
- custom close: maybe a bit slower?
- simple open: 3% faster
- simple reload: 2% faster
- simple close: 3% faster

It's either the split or another change in https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6ff60a083701d08c52702daf50f28e8f46ae3a1c&tochange=8f1b2f872f0ea358a0412eb8b8687f08d47f6621 . I don't see another good candidate from looking briefly at the list.

The impact is higher that what I saw with my linux talos tests. If we want to address this we can try to create a dedicated loader and see if performances go back to their previous state.
Blocks: 1445718
(In reply to Julian Descottes [:jdescottes][:julian] from comment #34)
> - Task.jsm vs baseline:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=d8e0150f06dac2520bfc44f9c8a298b5
> 7e9d9f6a&newProject=try&newRevision=5d46468ddc5a937214075f24342014e835e4a085&
> framework=1

The only significant change here is custom.webconsole.open, regressed by 2.6% but with medium confidence

> - DAMP split vs baseline:
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=5d46468ddc5a937214075f24342014e8
> 35e4a085&newProject=try&newRevision=a0ad5db7a293696775579ff6032da5b313935d46&
> framework=1

Whereas this one highlight more changes:
  complicated.inspector.reload 4.7% faster
  complicated.takeCensus 24% faster
  console.openwithcache.open 4.3% faster
  simple.saveHeapSnapshot 19% faster
  simple.styleeditor.open 5% slower
with all high confidence.

I'm wondering if that is cause by a significant change in memory usage, makng the heapsnapshot significantly faster and releasing a lot of pressure on the GC and Firefox overall. If these tests have such effect on others, it would be worth doing something about them!
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: