1434849 - DAMP should track HAR export

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

HAR export is currently very slow. It can take up to 10s against duckduckgo.com.
Today, there is nothing in DAMP covering anything related to HAR.
We should introduce a test script to cover it.

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Alex, is there a good example of existing test to be used as source of inspiration?
Any other tips how to implement the test?

Honza

Comment 2

[Alexandre Poirot [:ochameau]]

I would add a new subtest between reload and close, right here:
https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js#878
After `await requestsDone;`. So that we test HAR export performances against simple and complicated documents (and the new custom document coming from bug 1419327).

You may take inspiration from bug 1391688 and bug 1419533. But these two bug uses custom document for the purpose of their tests, whereas I think it is better to hook into simple/complicated.

So you should simple call an helper method right after `requestsDone`, this method will do something like this:
  netmonitor() {
    ...
    await requestsDone;
    this.copyHar(label + ".netmonitor", toolbox)
    ...
  }
  ...
  async copyHar(label, toolbox) {
    let test = this.runTest(label + ".copyHar");

    // Do whatever is necessary to copy har into clipboard
    await ...;

    test.done();
  }

Note that this is some documentation about contributing a new DAMP test:
  http://docs.firefox-dev.tools/tests/writing-perf-tests.html
Please let me know if/how this doc can be improved!

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1434849 - Track HAR export in DAMP;

Review commit: https://reviewboard.mozilla.org/r/224348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224348/

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #2)
> I would add a new subtest between reload and close, right here:

Thanks for the pointers Alex!
Honza

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8955188 [details]
Bug 1434849 - Track HAR export in DAMP;

https://reviewboard.mozilla.org/r/224348/#review231214

Thanks Honza, it looks good.

I'm only concerned about its execution time.
It takes 3s, it is pretty significant for just one test:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=73bfcd923dc43b4b46ad4c95edb2160f2664a65b&newProject=try&newRevision=3049cd3c196bec9bdcf236d76d6fd3e05242d65c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=har&framework=1
Do you have immediate plan to speed up har export?

Here is a profile of exportHar subtest:
  https://perfht.ml/2tl8Joo
It highlights FirefoxDataProvider.updateRequest. I'm wondering if the usage of Object.assign is an issue here. Do you know if FirefoxDataProvider has to be immutable? Isn't it redundant with what will happen in the reducers?

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8955188 [details]
> Bug 1434849 - Track HAR export in DAMP;
> 
> https://reviewboard.mozilla.org/r/224348/#review231214
> 
> Thanks Honza, it looks good.
> 
> I'm only concerned about its execution time.
> It takes 3s, it is pretty significant for just one test:
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=try&originalRevision=73bfcd923dc43b4b46ad4c95e
> db2160f2664a65b&newProject=try&newRevision=3049cd3c196bec9bdcf236d76d6fd3e052
> 42d65c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignatur
> e=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&filter=har&framework=1
> Do you have immediate plan to speed up har export?
No, not at the moment.

> Here is a profile of exportHar subtest:
>   https://perfht.ml/2tl8Joo
> It highlights FirefoxDataProvider.updateRequest. I'm wondering if the usage
> of Object.assign is an issue here. Do you know if FirefoxDataProvider has to
> be immutable? Isn't it redundant with what will happen in the reducers?
The assign in this methods isn't used for immutability, it's there for
merging results from fetch functions into one payload. I am a bit surprised
that this particular assign would represent a bottleneck.

Honza

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f749f8d424c4
Track HAR export in DAMP; r=ochameau

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f749f8d424c4