1210090 - Create a DAMP test to measure saving and reading a heap snapshot

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It is fairly slow-ish right now, and there are a few initial low hanging fruits we are plucking (bug 1196461 for example) but it would be nice to watch this to make sure it doesn't get worse and to verify when we do indeed make it better.

bgrins, are there any docs or anything on writing new DAMP tests?

Comment 1

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #0)
> It is fairly slow-ish right now, and there are a few initial low hanging
> fruits we are plucking (bug 1196461 for example) but it would be nice to
> watch this to make sure it doesn't get worse and to verify when we do indeed
> make it better.
> 
> bgrins, are there any docs or anything on writing new DAMP tests?

Curious, how slow are we talking?  If the time dwarfs the other measurements in DAMP we may want to consider a new talos test.

Anyhow there's not many docs, but https://wiki.mozilla.org/Buildbot/Talos/Tests#DAMP and then most any documentation about TART will also apply since it was derived from that.  To add new timings, tests can be added to this array: https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js?offset=800#83, where your test would do something roughly like:

function myTest() {
  return new Promise(resolve => {
    this.addTab(url);
    let tab = getActiveTab(getMostRecentBrowserWindow());
    let target = devtools.TargetFactory.forTab(tab);
    let toolbox = yield gDevTools.showToolbox(target, tool);
    let startRecordTimestamp = performance.now();
    // Do your stuff...
    let stopRecordTimestamp = performance.now();
    this._results.push({name: "mymeasurement.DAMP", value: stopRecordTimestamp - startRecordTimestamp});
  });
}

We will want to coordinate with :jmaher to discuss the new timings, what they measure, expected values, etc to get some advice on how to best proceed.  Also note, as mentioned in the wiki you'll need to get a copy of the tp5 page set to run the tests locally.  Joel or I can get you a copy of that.

Comment 2

[Brian Grinstead [:bgrins]]

Also of note, Jordan put together an ad-hoc measurement suite he was running locally when testing some changes based on the Talos addon layout [0] that I think is nicer in a few ways.  In particular, each test sits in it's own file [1][2].  This would probably be a good jumping off point if we end up wanting/needing to do a new test.

[0]: https://github.com/jsantell/metaperf
[1]: https://github.com/jsantell/metaperf/blob/master/addon/content/adding-new-tests.md
[2]: https://github.com/jsantell/metaperf/blob/master/addon/content/test-profilerFetchData.js

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #0)
> > It is fairly slow-ish right now, and there are a few initial low hanging
> > fruits we are plucking (bug 1196461 for example) but it would be nice to
> > watch this to make sure it doesn't get worse and to verify when we do indeed
> > make it better.
> > 
> > bgrins, are there any docs or anything on writing new DAMP tests?
> 
> Curious, how slow are we talking?  If the time dwarfs the other measurements
> in DAMP we may want to consider a new talos test.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Saving a heap snapshot takes about 2400 ms and reading takes about 600 ms for me locally.

I was under the impression that DAMP was a talos subsuite, is that incorrect?

Is that repo's tests run on every n^th checkin to m-c? Are its results actively monitored the way talos/DAMP results are?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #4)
> Saving a heap snapshot takes about 2400 ms and reading takes about 600 ms
> for me locally.
> 
> I was under the impression that DAMP was a talos subsuite, is that incorrect?
> 
> Is that repo's tests run on every n^th checkin to m-c? Are its results
> actively monitored the way talos/DAMP results are?

DAMP runs on every checkin AFAIK (basically any time the 'g2' suite runs).  The repo I linked to is not deployed anywhere, it was just used for some local testing when he was converting stuff to workers IIRC.

My concern with timing is that we wouldn't want to throw in a measurement that is way bigger than anything else measured in that test, because then regressions on non-snapshots could go unnoticed.  For instance (making up numbers here) - imagine that heap snapshots took 3 seconds, but the typical toolbox open was 300ms, close was 25ms, and reload was 30ms.  Since snapshots would be such a high percentage of the overall results, a 20% regression in toolbox open time for a single tool could potentially end up going unnoticed.

On the other hand, I like the idea of grouping all of our devtools performance measurement stuff into one place.  And I also want to start adding more measurements across the tools like this.  So maybe there is a way to modify the reporting to normalize the effect that a new probe that takes longer than others would have on regression detection.  Joel, is this enough context?  Would something like that be possible within DAMP, or should we consider spawning off a new Talos test?

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It seems a bit crazy that one test affects regression reporting for another test. Why isn't each test considered separately?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #6)
> It seems a bit crazy that one test affects regression reporting for another
> test. Why isn't each test considered separately?

From what I understand, it is far too noisy when measuring a single probe (i.e. complicated.inspector.open.DAMP).  So it takes all DAMP probes together and then does some processing to reliably calculate regressions/improvements.  I'd be very interested in the ability to get alerts on a single probe (or user defined set of probes) though.

Comment 8

[Brian Grinstead [:bgrins]]

Regarding Comment 7 - this isn't a DAMP-specific issue, it's how the Talos and alerting system works across all suites

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I am excited to see more DAMP related performance tests in the works!  While it is not ideal to lump everything together, it is unrealistic to filter the noise and accurately triage the alerts coming in.  For example in September we had 2021 alerts.  Assume half are improvements, and that is still over 1000 alerts (1137 to be exact) that needs some minimal amount of verification.  Overall, there are 22 bugs that account for the regressions in September- that is a lot of work for somebody or a small group of people to deal with- imagine multiplying that by 10!  It is a future possibility, maybe for select tests- but until we get more support to triage these alerts, we need something manageable.

Regarding adding a new measurement to DAMP.  Right now all the metrics we collect from DAMP are collected individually and we take the geometric mean of each measurement to come up with the final DAMP score.  This means that if we have 10 smaller data points and 1 or 2 larger data points the total DAMP score will allow for showing regressions.  We have this situation in smaller suites like SVG, Opacity and A11y.  There is a minor chance that a regression might slip by if all the subtests in DAMP had the right distribution of numbers.

So unless this makes DAMP run noticeably longer (>5 minutes), I would assume it would be ok to just add it.  We will see a shift in the graphs, and can correlate that to the new test being added.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create DAMP tests for saving and reading heap snapshots

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

When I try to actually run the new test locally, I get python errors. First I set up the virtualenv and unzip the tp5n.zip file (which bgrins kindly gave me):

> $ cd testing/talos
> $ virtualenv .
> $ . bin/activate
> $ cd talos
> $ unzip tp5n.zip
> $ cd ..

That goes fine, but then I try and run the tests and things go wrong:

> $ python run_tests.py -e ../../../obj.noindex/dist/bin/firefox -a damp --develop
> Traceback (most recent call last):
>   File "run_tests.py", line 7, in <module>
>     import mozversion
> ImportError: No module named mozversion

I'm following the instructions in https://bugzilla.mozilla.org/show_bug.cgi?id=1210920#c3 and https://bugzilla.mozilla.org/show_bug.cgi?id=1210920#c5.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Er, that second link was supposed to link to https://bugzilla.mozilla.org/show_bug.cgi?id=1210920#c6

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

one step overlooked, here are the full set of commands:
> $ cd testing/talos
> $ virtualenv .
> $ . bin/activate
python setup.py develop
> $ cd talos
> $ cd tests
# wget tp5n.zip
> $ unzip tp5n.zip
> $ cd ..

note the setup.py command as #4.

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8672175 [details] [diff] [review]
Create DAMP tests for saving and reading heap snapshots

Review of attachment 8672175 [details] [diff] [review]:
-----------------------------------------------------------------

I am not an expert in the devtools/damp code, I assume bgrins will catch anything there.  Just one nit, thanks for expanding damp!

::: testing/talos/talos/tests/devtools/addon/content/damp.js
@@ +91,5 @@
> +  },
> +
> +  readHeapSnapshot: function(label) {
> +    let start = performance.now();
> +    let snapshot = ThreadSafeChromeUtils.readHeapSnapshot(this._heapSnapshotFilePath);

what do we use snapshot for?

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Joel Maher (:jmaher) from comment #14)
> what do we use snapshot for?

Unused, but felt weird not using the return value. I suppose I can also measure the time to take a census of a heap snapshot while I'm at it...

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create DAMP tests for saving and reading heap snapshots

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8672738 [details] [diff] [review]
Create DAMP tests for saving and reading heap snapshots

Review of attachment 8672738 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for the update so fast.  I will let bgrins comment on the devtools side of things to sanity check this is useful :)

::: testing/talos/.gitignore
@@ +4,5 @@
> +lib/
> +talos.egg-info
> +talos/tests/tp5n.zip
> +talos/tests/tp5n
> +talos/tests/devtools/damp.manifest.develop

technically we could do *.develop, for now this is fine and if *.develop doesn't work, then we can eventually list out all the manifest files in each directory for each test.

Comment 18

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Ran the setup.py command and got a little further, but still haven't been able to run locally.

Giving up and going with a try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=301cba7c405c
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=301cba7c405c

Comment 19

[Brian Grinstead [:bgrins]]

For some reason there is an issue for me when trying to run talos locally with a relative path to the binary.  Here are updated instructions based on Comment 13 that's working for me:

$ cd testing/talos
$ virtualenv .
$ . bin/activate
$ python setup.py develop
$ cd talos

# Get tests (only first time, if needed)
$ cd tests
$ wget tp5n.zip
$ unzip tp5n.zip
$ cd ..

# Run the test - note that I need to use the full absolute path (not relative), otherwise
# it fails with "Could not find the Mozilla runtime."
$ python run_tests.py -e /path/to/fx-team/objdir.noindex/dist/Nightly.app/Contents/MacOS/firefox  -a damp --develop

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8672738 [details] [diff] [review]
Create DAMP tests for saving and reading heap snapshots

Review of attachment 8672738 [details] [diff] [review]:
-----------------------------------------------------------------

Once I got this running locally I found a few errors.  See below:

Need to add 
    preferences = {'devtools.memory.enabled': True}
to: https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/test.py#360

::: testing/talos/talos/tests/devtools/addon/content/damp.js
@@ +2,5 @@
>  const {devtools} =
>    Components.utils.import("resource://gre/modules/devtools/shared/Loader.jsm", {});
>  const { getActiveTab } = devtools.require("sdk/tabs/utils");
>  const { getMostRecentBrowserWindow } = devtools.require("sdk/window/utils");
> +const { ThreadSafeChromeUtils } = devtools.require("ThreadSafeChromeUtils");

const ThreadSafeChromeUtils = devtools.require("ThreadSafeChromeUtils")

@@ +76,5 @@
>  
> +  saveHeapSnapshot: function(label) {
> +    let tab = getActiveTab(getMostRecentBrowserWindow());
> +    let target = devtools.TargetFactory.forTab(tab);
> +    let toolbox = devtools.getToolbox(target);

Should be gDevTools.getToolbox(target)

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: damp-heap-changes.patch

changes from our session over irc

Comment 22

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Create DAMP tests for saving and reading heap snapshots

Comment 23

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab3ad2b30304

jmaher, is there anything else we need to do before this gets checked in?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #23)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab3ad2b30304
> 
> jmaher, is there anything else we need to do before this gets checked in?

I think we will, since I'm seeing an orange on g2 after this push with "FAIL: Graph server unreachable (5 attempts)".  The new data does seem to be included in http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ab3ad2b30304/try-linux64/try_ubuntu64_hw_test-g2-bm103-tests1-linux-build831.txt.gz.

Here are the new measurements:

16:50:37     INFO -  2015-10-13 16:50:37,906 DEBUG : BROWSER_OUTPUT: simple.memory.open.DAMP: 311.2
16:50:37     INFO -  2015-10-13 16:50:37,906 DEBUG : BROWSER_OUTPUT: simple.memory.reload.DAMP: 28.8
16:50:37     INFO -  2015-10-13 16:50:37,906 DEBUG : BROWSER_OUTPUT: simple.saveHeapSnapshot: 70.3
16:50:37     INFO -  2015-10-13 16:50:37,906 DEBUG : BROWSER_OUTPUT: simple.readHeapSnapshot: 8.7
16:50:37     INFO -  2015-10-13 16:50:37,906 DEBUG : BROWSER_OUTPUT: simple.takeCensus: 6.2
16:50:37     INFO -  2015-10-13 16:50:37,906 DEBUG : BROWSER_OUTPUT: simple.memory.close.DAMP: 21.4

16:50:37     INFO -  2015-10-13 16:50:37,908 DEBUG : BROWSER_OUTPUT: complicated.memory.open.DAMP: 323.6
16:50:37     INFO -  2015-10-13 16:50:37,908 DEBUG : BROWSER_OUTPUT: complicated.memory.reload.DAMP: 47.7
16:50:37     INFO -  2015-10-13 16:50:37,908 DEBUG : BROWSER_OUTPUT: complicated.saveHeapSnapshot: 84.1
16:50:37     INFO -  2015-10-13 16:50:37,908 DEBUG : BROWSER_OUTPUT: complicated.readHeapSnapshot: 8.7
16:50:37     INFO -  2015-10-13 16:50:37,909 DEBUG : BROWSER_OUTPUT: complicated.takeCensus: 5.7
16:50:37     INFO -  2015-10-13 16:50:37,909 DEBUG : BROWSER_OUTPUT: complicated.memory.close.DAMP: 28.3

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: add page ids to graph server for the new measurements (1.0)

:bgrins, do these look like the correct set of new tests, if so, we can land this in the repo and I can get them deployed.  ETA would be tomorrow morning as I need someone with database credentials to do the sql.

Comment 26

[Brian Grinstead [:bgrins]]

Comment on attachment 8673664 [details] [diff] [review]
add page ids to graph server for the new measurements (1.0)

Review of attachment 8673664 [details] [diff] [review]:
-----------------------------------------------------------------

This looks right to me, let's forward to fitzgen for a second set of eyes.

Nick, I was also thinking about something yesterday - if you had were wanting to control the amount of memory used for the heap stuff in this test you could always add your own test page to load for heap snapshots.  We could leave the memory.open, memory.close, and memory.reload in the normal simple/complicated framework and then have separate keys for memory.saveHeapSnapshot, memory.readHeapSnapshot, memory.takeCensus that use a custom test page.  Up to you, I can help get this set up if you'd rather do that.

Comment 27

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8673664 [details] [diff] [review]
add page ids to graph server for the new measurements (1.0)

Review of attachment 8673664 [details] [diff] [review]:
-----------------------------------------------------------------

> Nick, I was also thinking about something yesterday - if you had were wanting to control
> the amount of memory used for the heap stuff in this test you could always add your own
> test page to load for heap snapshots.  We could leave the memory.open, memory.close, and
> memory.reload in the normal simple/complicated framework and then have separate keys for
> memory.saveHeapSnapshot, memory.readHeapSnapshot, memory.takeCensus that use a custom
> test page.  Up to you, I can help get this set up if you'd rather do that.

Can we do this as a follow up? I'd like to land what we have now, and then revisit.

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

landed the graph server changes:
https://hg.mozilla.org/graphs/rev/b43c38e97a79

we still need sql statements inserted before seeing green.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8673383 [details] [diff] [review]
Create DAMP tests for saving and reading heap snapshots

Review of attachment 8673383 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me, let's wait for 1214734 to be resolved and then retrigger some try runs to make sure this is green before checking in

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Bug 1214734 was resolved, retriggering the try truns.

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #30)
> Bug 1214734 was resolved, retriggering the try truns.

Looks like I was beaten to it :) Thanks, Joel!

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

ship it!

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8784802b62fb

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8784802b62fb

Comment 35

[Hallvord R. M. Steen [:hallvors]]

So, I should add some code similar to the attached script here:
/talos/talos/tests/devtools/addons/content/damp.js ?

As a sub-test inside the getTestsForURL() ? But this will make the test "run" (read: fail) on pages it wasn't meant for. How can I add a test which loads a specific URL?

Comment 36

[Brian Grinstead [:bgrins]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #35)
> So, I should add some code similar to the attached script here:
> /talos/talos/tests/devtools/addons/content/damp.js ?
> 
> As a sub-test inside the getTestsForURL() ? But this will make the test
> "run" (read: fail) on pages it wasn't meant for. How can I add a test which
> loads a specific URL?

Instead of adding a new test in getTestsForURL (which is meant for tests that get run on the standard simple/complicated urls), I'd add the new test after this: https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js?offset=800#160

So something like `tests.push(getDebuggerSteppingTest())` and have that function return a single test to run (which includes adding a tab, running the test, and then closing the tab).   The 'init' and 'restore' steps can be identical to those steps for the other tests.

By the way, IIRC something that would make running this easier locally is to set all the other subtests to false here: https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.html#12 and enable your new one in that object.