Closed Bug 1434849 Opened 2 years ago Closed 2 years ago

DAMP should track HAR export

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: ochameau, Assigned: Honza)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Blocks: 1434855
Priority: -- → P3
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
Flags: needinfo?(poirot.alex)
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!
Flags: needinfo?(poirot.alex)
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
(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 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?
Attachment #8955188 - Flags: review?(poirot.alex) → review+
(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
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f749f8d424c4
Track HAR export in DAMP; r=ochameau
https://hg.mozilla.org/mozilla-central/rev/f749f8d424c4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.