Closed Bug 1404917 Opened 7 years ago Closed 7 years ago

Response content should be loaded lazily

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 59

People

(Reporter: ochameau, Assigned: ochameau)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files, 8 obsolete files)

Bug 1404917 - Fetch response content only on-demand.
(deleted), text/x-review-board-request
Honza
: review+
Details
Bug 1404917 - Use request.mimeType instead of request.responseContent.content.mimeType.
(deleted), text/x-review-board-request
Honza
: review+
Details
Bug 1404917 - Move image preview tooltip to file name and remove preview icon.
(deleted), text/x-review-board-request
Honza
: review+
Details 
The root issue comes from here:
http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/devtools/client/netmonitor/src/connector/firefox-data-provider.js#392-399
      case "responseContent":
        this.webConsoleClient.getResponseContent(actor,
          this.onResponseContent.bind(this, {
            contentSize: networkInfo.response.bodySize,
            transferredSize: networkInfo.response.transferredSize,
            mimeType: networkInfo.response.content.mimeType
          }));
        emit(EVENTS.UPDATING_RESPONSE_CONTENT, actor);

Where we force loading response content when the actor emits "responseContent" event.
The main point of this event is to be able to retrieve bodySize, transferredSize and mimeType *without* having to pull the complete response content. So it counter productive to immediately pull it like that.
Instead the call to webConsoleClient.getResponseContent should be lazily done, only once we have to display the response content.
Comment on attachment 8914322 [details]
Bug 1404917 - Fetch response content only on-demand.

This is mostly a trial patch to confirm this impact of lazy loading and it does.
I see a 17% win on DAMP.

Now, is this approach ok to you? If that would require much more work, do not hesitate you, or someone else, to pick it up.
Attachment #8914322 - Flags: feedback?(odvarko)
(In reply to Alexandre Poirot [:ochameau] from comment #1)
> The main point of this event is to be able to retrieve bodySize,
> transferredSize and mimeType *without* having to pull the complete response
> content. So it counter productive to immediately pull it like that.
Yes, I totally agree.

The only thing is that the list of requests displays a little image thumbnail (for images), which needs to fetch the response body immediately.

So, we could:
1) remove the image thumbnail entirely from the list and let the use use the tooltip (lazily)
2) get the response body only for images

I personally vote for #1 since image response tend to be big and we can save a lot of time there.

@Alex, what do you think?

Honza
Flags: needinfo?(poirot.alex)
Attachment #8914322 - Flags: feedback?(odvarko) → feedback+
(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> (In reply to Alexandre Poirot [:ochameau] from comment #1)
> > The main point of this event is to be able to retrieve bodySize,
> > transferredSize and mimeType *without* having to pull the complete response
> > content. So it counter productive to immediately pull it like that.
> Yes, I totally agree.
> 
> The only thing is that the list of requests displays a little image
> thumbnail (for images), which needs to fetch the response body immediately.
> 
> So, we could:
> 1) remove the image thumbnail entirely from the list and let the use use the
> tooltip (lazily)
> 2) get the response body only for images
> 
> I personally vote for #1 since image response tend to be big and we can save
> a lot of time there.

Yes, we should do #1.

Transferring all images is still not good.
If we want this feature, we should introduce a request to only transfer icons instead of fullsize images.

Otherwise any feedback about the patch itself?
* componentWillUpdate?
Is it the best React function to hook into? TBH I choosed this one randomly.
May be we should use componentWillMount to fetch the response content earlier?
* Is it ok to do what I did in the data provider?
* Immutable and requests payload from firefox-data-provider.js
With this patch we still set "responseContent" to the request payload,
which, if I'm following netmonitor codebase correctly, is passed to store and processed by immutable. Is it true?
  ** If yes, I think we should avoid passing very large strings like that.
     I'm afraid these fields will be copied by immutable very easily, whereas we should keep only one copy of it.
  ** If no, can you prove me "responseContent" will never be copied nor by immutable, redux, react, ...?
So, I'm wondering the ResponsePanel React component should call getResponseContent() which won't update redux and instead return the response content via a Promise and update ResponsePanel state?
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Yes, we should do #1.
Excellent

> Transferring all images is still not good.
> If we want this feature, we should introduce a request to only transfer
> icons instead of fullsize images.
Yes (requires new back-end support)

> Otherwise any feedback about the patch itself?
Ah, sorry, I meant to provide feedback before.

> * componentWillUpdate?
> Is it the best React function to hook into? TBH I choosed this one randomly.
> May be we should use componentWillMount to fetch the response content
> earlier?
I believe that `componentWillUpdate` is the right choice. We want to fetch the response-content when the response-info has been received (not sooner). And using e.g. `componentWillMount` or `componentDidMount` could be executed too soon.

> * Is it ok to do what I did in the data provider?
Looks ok, I am just not sure about the new events you are introducing 
`UPDATING_RESPONSE_CONTENT_INTO` and `UPDATING_RESPONSE_CONTENT_INFO` -> typo?

And the `RECEIVED_RESPONSE_CONTENT` is emmited twice in onResponseContent();

Essentially, I think we don't need any new events, all we need is to make 
sure the `RECEIVED_RESPONSE_CONTENT` is fired as soon as the response body
is received. These events are there only for tests.

Speaking of tests, this will need to be updated as well.


> * Immutable and requests payload from firefox-data-provider.js
> With this patch we still set "responseContent" to the request payload,
> which, if I'm following netmonitor codebase correctly, is passed to store
> and processed by immutable. Is it true?
Correct. List of requests is stored in src/reducers/requests.js reducer.
See the `Request` immutable record.
See also the `UPDATE_REQUEST` case where the record is updated with incoming data.

>   ** If yes, I think we should avoid passing very large strings like that.
>      I'm afraid these fields will be copied by immutable very easily,
> whereas we should keep only one copy of it.
Good point.

>   ** If no, can you prove me "responseContent" will never be copied nor by
> immutable, redux, react, ...?
> So, I'm wondering the ResponsePanel React component should call
> getResponseContent() which won't update redux and instead return the
> response content via a Promise and update ResponsePanel state?
Hm, I don't like having 'requests state' spread across components as local state. All should be stored in the app store (not Apple store, though ;-), so we can reuse it in any component as needed in the future.

E.g. we could rather introduce a new fresh 'responses' reducer that uses simple map storing all response keyed by request id. Consequently, the `getResponseContent()` would update this new 'mutable' reducer.

Btw. the Console HTTPi is broken (even with my patch for bug 1404138) I don't see the response there.

Honza
@Harald: This bug report shows nice analysis that removing the little thumbnail preview feature can help us get better performance (by not requesting response content immediately). We were discussing that couple weeks ago. We can put the feature back as soon as we teach the back-end to send just small icons. How does that sound?

Honza
Flags: needinfo?(hkirschner)
Sounds like a good short term win while we teach the backend to only send what we need for the first UI in a non-blocking fashion. LGTM

I don’t see specific numbers here but much improvement would we expect for the most common/best case?

Related for future perf work, is it possible to have Telemetry report how long it takes from network resource being fetched to it being displayed in the panel?
Flags: needinfo?(hkirschner)
(In reply to :Harald Kirschner :digitarald from comment #8)
> I don’t see specific numbers here but much improvement would we expect for
> the most common/best case?

DAMP reports a 17% win on the page reload scenario (This DAMP probe is quite good and should be trustable).
This patch is only for response content, we would have also significant wins by doing same patch for request/response headers/cookies. Also this patch still make the response content go through immutable, so there may be even more to win on the current patch.

> Related for future perf work, is it possible to have Telemetry report how
> long it takes from network resource being fetched to it being displayed in
> the panel?

I was waiting for OKR discussion to settle before trying to setup performance telemetry.
We have much more important telemetry to add first, like: panel startup, panel refresh after reload, ...
I'm having hard time getting the full mutable reducer solution to work. I think I'm really close, what I miss now is having react to update after I update the mutation store (which is just a Map). I don't know why it doesn't update and how to make it update things.

Otherwise, with the current patch, I see duplicated strings. I see 6 copies of a given response content. I imagine it may relate to the number of time I click in the netmon interface. I don't know exactly yet, but there is still many copies, which is really bad as it can be MB size string!
Comment on attachment 8915200 [details]
Bug 1404917 - mutable reducer

Honza, Could you scan this to see why react doesn't update when I do an addRequestData ADD_REQUEST_DATA action?
Attachment #8915200 - Flags: feedback?(odvarko)
Attached patch lazy-response.patch (obsolete) (deleted) — — Splinter Review 
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Comment on attachment 8915200 [details]
> Bug 1404917 - mutable reducer
> 
> Honza, Could you scan this to see why react doesn't update when I do an
> addRequestData ADD_REQUEST_DATA action?
See the attached patch. The main problem was that the new 'requestsData' reducer didn't return different state. Returning a new state (different object) is what triggers the update.

This all is great, thanks for working on it Alex!

I am doing some changes to the FirefoxDataProvider in:

1) Bug 1395759 - Intermittent devtools/client/webconsole/new-console-output/test/mochitest
2) PATCH Bug 1005755 - A button to pause/stop the HTTP traffic in the network monitor view

All these changes should help and this bug should depend on it (I'll set the deps).

Also, if testing proves that using ImmutableJS is slowing things down significantly I would vote for avoiding ImmutableJS in the net panel entirely. Data in the net panel are rather static (requests are only appended) and it can be even better perf win.

Honza
Comment on attachment 8915200 [details]
Bug 1404917 - mutable reducer

I am assuming that this is a prototype. Of course, final patch would have to use different naming for reducer, actions (something like 'responseData' instead of 'requestsData'), introducing state selector, code clean up in the Response panel, etc.

But, it definitely goes in the right direction!

Honza
Attachment #8915200 - Flags: feedback?(odvarko) → feedback+
Attached patch lazy-response-fixes (obsolete) (deleted) — — Splinter Review 
Attaching the correct patch.

Honza
So speaking only about memory, response content strings seem *not* duplicated by ImmutableJS.
I can't see anything wrong solely with ImmutableJS.
With both patches (with and without mutable reducer), you "only" get 3 copies of it. Two being done by the actors.
* One in the parent process, done by devtools/shared/webconsole/network-monitor.js which listen for network request in the parent process,
* Another one in the content process, done by the console actor living in this process and piping the content to the client via getResponseContent request,
* And the last in the parent process again, done by netmonitor client which does the getResponseContent call.

I do see many other strings being duplicated, it looks like image data URI.
I also see many response content duplicated when toggling the response panel on and off for the same request.
But I can't prove it is because of ImmutableJS as I get similar result between the two patches.

Anyway, we can followup on this duplicated strings. It sounds independant and less important than getting most things loaded lazily.

Now I'm wondering, should I stick to the first patch, or is there any value with the dedicated/mutable reducer?
I improved the first patch to fix context menus. Instead of querying responseContent, I now call getResponseContent.
And I made another patch to query request.mimeType instead of request.responseContent.content.mimeType.

Finally, the last patch is a very gross way to demonstrate how to have only one copy of response content string,
but I'll open a dedicated bug about that later.
Attachment #8915566 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #21)
> So speaking only about memory, response content strings seem *not*
> duplicated by ImmutableJS.
> I can't see anything wrong solely with ImmutableJS.

It's good to see this library appears to function as designed, at least!

ImmutableJS is meant to possibly _reduce_ copying that might occur without it, so at least it's not _adding_ copies.
Blocks: 1406314
Attachment #8915630 - Attachment is obsolete: true
(In reply to Alexandre Poirot [:ochameau] from comment #21)
> So speaking only about memory, response content strings seem *not*
> duplicated by ImmutableJS.
> I can't see anything wrong solely with ImmutableJS.
Nice, this is great confirmation.

> With both patches (with and without mutable reducer), you "only" get 3
> copies of it. Two being done by the actors.
> * One in the parent process, done by
> devtools/shared/webconsole/network-monitor.js which listen for network
> request in the parent process,
> * Another one in the content process, done by the console actor living in
> this process and piping the content to the client via getResponseContent
> request,
> * And the last in the parent process again, done by netmonitor client which
> does the getResponseContent call.
I see, make sense.

> I do see many other strings being duplicated, it looks like image data URI.
> I also see many response content duplicated when toggling the response panel
> on and off for the same request.
> But I can't prove it is because of ImmutableJS as I get similar result
> between the two patches.
> 
> Anyway, we can followup on this duplicated strings. It sounds independant
> and less important than getting most things loaded lazily.
Agree

> Now I'm wondering, should I stick to the first patch, or is there any value
> with the dedicated/mutable reducer?
I don't see much value in the new mutable reducer.

> I improved the first patch to fix context menus. Instead of querying
> responseContent, I now call getResponseContent.
> And I made another patch to query request.mimeType instead of
> request.responseContent.content.mimeType.
Yep, I like that.

> Finally, the last patch is a very gross way to demonstrate how to have only
> one copy of response content string,
> but I'll open a dedicated bug about that later.

Thanks for working on this Alex,
Honza
Attachment #8915200 - Attachment is obsolete: true
Honza, I rebased on top of bug 1005755 and made the patch more generic in order to help supporting fetching headers and cookies lazily as well.
Assignee: nobody → poirot.alex
Any ETA on reviewing this?
BTW, this bug and bug 1404913 are flagged P3, but it should rather be P1 or at least P2.
Flags: needinfo?(odvarko)
(In reply to Alexandre Poirot [:ochameau] from comment #28)
> Any ETA on reviewing this?
> BTW, this bug and bug 1404913 are flagged P3, but it should rather be P1 or
> at least P2.
P2 sounds good. We reserve P1 for "Security hole / causing data-loss or crashes / embarrassing / intermittent with failure rate > 30 per week". We did not really define what "embarrassing" meant here. But enhancements related to a thing we are working on currently are usually P2s.
See https://docs.google.com/document/d/1sZAj77LDLH4oqR9TA4RlFubk_Kio0MHXfiVTjZldiTs/edit
(In reply to Alexandre Poirot [:ochameau] from comment #28)
> Any ETA on reviewing this?
> BTW, this bug and bug 1404913 are flagged P3, but it should rather be P1 or
> at least P2.
Sorry for the delay and don't worry this is on top of my list
and I consider it important. I already fixed both of the blockers
1005755, 1395759 and I have been also looking at the patches.

I am not done since preparation of Firebug EOL campaign is eating
a lot of my time these days. But, I'll finish the review next week.

Honza
Flags: needinfo?(odvarko)
Comment on attachment 8914322 [details]
Bug 1404917 - Fetch response content only on-demand.

https://reviewboard.mozilla.org/r/185602/#review198570

Some more comments:

1) Since response body is not fetched by default, what about removing the icon (little image-response preview) from the File column? This could also safe some CPU cycles since we don't have to calculate `responseContentDataUri` (see also `formDataURI`)
2) The FirefoxDataProvider is firing `EVENTS.PAYLOAD_READY`, and also calling custom `updateRequest` callback, when request payload is ready (i.e. fetched from the backend). Currently it explicitelly  waits for 'response-content', this should be changed.
3) I am not sure about impact on tests

Honza

::: devtools/client/netmonitor/src/components/response-panel.js:43
(Diff revision 4)
>    displayName: "ResponsePanel",
>  
>    propTypes: {
>      request: PropTypes.object.isRequired,
>      openLink: PropTypes.func,
> +    connector: PropTypes.object.isRequired,

Params panel already has the connector prop passed in, so the patch can be simplified.

::: devtools/client/netmonitor/src/components/response-panel.js:122
(Diff revision 4)
>      return null;
>    },
>  
> +  // componentWillReceiveProps is the only method called when switching to the
> +  // ResponsePanel *and* when being on it and switching between two requests.
> +  componentWillReceiveProps(nextProps) {

componentWillReceiveProps is only called when the props have changed and when this is not an initial rendering. 

In order to cover also the initial rendering you can combine with componentDidUpdate. You migt see how the ParamsPanel does it:
http://searchfox.org/mozilla-central/rev/dd47bee6468de7e1221b4d006342ad6b9813d0e5/devtools/client/netmonitor/src/components/ParamsPanel.js#52-58

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:355
(Diff revision 4)
> +    emit(info.event, id);
> +
> +    return promise;
> +  }
> +
> +  /**

There is already `requestData` in the provider, which could be utilized by this patch, I guess.
Attachment #8914322 - Flags: review?(odvarko)
Comment on attachment 8915629 [details]
Bug 1404917 - Use request.mimeType instead of request.responseContent.content.mimeType.

https://reviewboard.mozilla.org/r/186818/#review198572

This looks good to me

R+

Honza
Attachment #8915629 - Flags: review?(odvarko) → review+
Comment on attachment 8914322 [details]
Bug 1404917 - Fetch response content only on-demand.

https://reviewboard.mozilla.org/r/185602/#review199016

::: devtools/client/netmonitor/src/components/ResponsePanel.js:137
(Diff revision 6)
> +    }
> +  }
> +
> +  // componentWillReceiveProps is the only method called when switching to the
> +  // ResponsePanel *and* when being on it and switching between two requests.
> +  componentWillReceiveProps(nextProps) {

The comment should be updated I guess.

Also, please use the following comment for a comment placed in front of a method.

/**
 *
 */

::: devtools/client/netmonitor/src/components/ResponsePanel.js:148
(Diff revision 6)
> +         !nextProps.request.responseContent.content)) {
> +      // This method will set `props.request.responseContent.content`
> +      // asynchronously and introduce another render.
> +      this.props.connector.requestData(nextProps.request.id, "responseContent");
> +    }
> +  }

The code in componentDidMount() and componentWillReceiveProps() should be moved into an extra function to avoid duplication.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:412
(Diff revision 6)
> +   * Fetches additional request data lazily.
> +   *
> +   * @param {string} id request id
> +   * @param {string} type request data to retrieve
> +   */
> +  requestDataX(id, type) {

What's the purpose of this method? It isn't used anywhere.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:420
(Diff revision 6)
> +    let key = id + "-" + type;
> +    if (this.pendingRequestData.has(key)) {
> +      return this.pendingRequestData.get(key);
> +    }
> +
> +    let info = LazyDataInfo[type];

LazyDataInfo is undefined

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:525
(Diff revision 6)
>        if (updateRequest) {
>          await updateRequest(actor, payloadFromQueue, true);
>        }
>  
> +      if (!isLazyRequest) {
> -      emit(EVENTS.PAYLOAD_READY, actor);
> +        emit(EVENTS.PAYLOAD_READY, actor);

I am worried about tests here. The 'PAYLOAD_READY' event is designed mainly for tests to ensure that entire RDP communication (fetching data) is done before a test moves forwared or finishes. Is that still ensured in case when response content is fetched lazily?
Attachment #8914322 - Flags: review?(odvarko) → review-
Comment on attachment 8922666 [details]
Bug 1404917 - Make FirefoxDataProvider.requestData more generic.

https://reviewboard.mozilla.org/r/193792/#review199026

This one looks good to me.

Honza
Attachment #8922666 - Flags: review?(odvarko) → review+
Comment on attachment 8922668 [details]
Bug 1404917 - Move image preview tooltip to file name and remove preview icon.

https://reviewboard.mozilla.org/r/193796/#review199030

This patch removes the preview entirelly including the tooltip.

Can we remove the little preview thumbnail only?

The tooltip is still useful and if the user hovers mouse over the file column (or any provided placeholder instead of the thumbnail) it should trigger response lazy load and display it.

Honza
Attachment #8922668 - Flags: review?(odvarko) → review-
Comment on attachment 8922667 [details]
Bug 1404917 - fix tests

https://reviewboard.mozilla.org/r/193794/#review199048

Looks good to me, but I am seeing some test failures in try, so I guess it needs more work.

Honza
Attachment #8922667 - Flags: review?(odvarko)
Thanks for working on this Alex, it'll be great perf improvement!

I noticed that there are also some eslint errors (long lines, unused vars, etc.),
so these needs fixes as well.

Honza
Comment on attachment 8915629 [details]
Bug 1404917 - Use request.mimeType instead of request.responseContent.content.mimeType.

https://reviewboard.mozilla.org/r/186818/#review203368

This patch looks good.
Attachment #8915629 - Flags: review+
Comment on attachment 8914322 [details]
Bug 1404917 - Fetch response content only on-demand.

https://reviewboard.mozilla.org/r/185602/#review203374

Looks good!

I am seeing some eslint errors, but they will be catched by try.

See couple of inline comments.

Thanks Alex!

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:431
(Diff revision 9)
>    }
>  
>    /**
> -   * Wrapper method for requesting HTTP details data from the backend.
> +   * Wrapper method for requesting HTTP details data for the payload.
> +   *
> +   * It is specifitic to all requests done from `onNetworkEventUpdate`, for data that are

typo specifitic -> specific

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:478
(Diff revision 9)
>        let { updateRequest } = this.actions;
>        if (updateRequest) {
>          await updateRequest(actor, payloadFromQueue, true);
>        }
>  
>        emit(EVENTS.PAYLOAD_READY, actor);

Just a note that PAYLOAD_READY event should be fired just once when all requested data are fetched (i.e. no current RDP communication)

::: devtools/client/webconsole/new-console-output/store.js:185
(Diff revision 9)
> +
> +        // /!\ This is terrible, but it allows ResponsePanel to be able to call
> +        // `dataProvider.requestData` to fetch response content lazily.
> +        // `proxy.networkDataProvider` is put by NewConsoleOutputWrapper on `serviceContainer`
> +        // which allow NetworkEventMessage to expose requestData on the fake `connector` object
> +        // it hands over to ResponsePanel.

The comment is exceeding max line length

::: devtools/client/webconsole/new-console-output/store.js:187
(Diff revision 9)
> +        // `dataProvider.requestData` to fetch response content lazily.
> +        // `proxy.networkDataProvider` is put by NewConsoleOutputWrapper on `serviceContainer`
> +        // which allow NetworkEventMessage to expose requestData on the fake `connector` object
> +        // it hands over to ResponsePanel.
> +        proxy.networkDataProvider = dataProvider;
>        }

Yeah, this looks a bit hacky, but the simplest atm.
Attachment #8914322 - Flags: review?(odvarko) → review+
Comment on attachment 8922668 [details]
Bug 1404917 - Move image preview tooltip to file name and remove preview icon.

https://reviewboard.mozilla.org/r/193796/#review203378

Looks good to me. Tested and works weell.
R+ assuming Try is green
Thanks!
Attachment #8922668 - Flags: review?(odvarko) → review+
Comment on attachment 8922667 [details]
Bug 1404917 - fix tests

https://reviewboard.mozilla.org/r/193794/#review203654

Looks reasonable to me, but please see my inline comments.
R+ assuming try is green

Thanks for working on this!
Honza

::: devtools/client/netmonitor/test/browser_net_autoscroll.js:61
(Diff revision 6)
>    let requestsContainerHeaders = requestsContainer.firstChild;
>    let headersHeight = requestsContainerHeaders.offsetHeight;
>    is(requestsContainer.scrollTop, headersHeight, "Did not scroll.");
>  
> +  // Stop doing requests.
> +  yield ContentTask.spawn(tab.linkedBrowser, {}, function* () {

The callback doesn't have to be a generator

::: devtools/client/netmonitor/test/browser_net_content-type.js:283
(Diff revision 6)
>      yield waitDOM;
> +    yield onResponseContent;
> +
> +    // Waiting for RECEIVED_RESPONSE_CONTENT isn't enough.
> +    // DOM may not be fully updated yet and checkVisibility(json) may still fail.
> +    yield new Promise(done => setTimeout(done));

You can use `waitForTick` or just `wait` for these cases.

::: devtools/client/netmonitor/test/browser_net_headers-alignment.js:44
(Diff revision 6)
>         "Headers for columns number " + columnNumber + " are aligned."
>      );
>    }
>  
> +  // Stop doing requests.
> +  yield ContentTask.spawn(tab.linkedBrowser, {}, function* () {

No need for generator function

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_netmonitor_shows_reqs_in_webconsole.js:9
(Diff revision 6)
>   * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> +Services.scriptloader.loadSubScript(
> +  "chrome://mochitests/content/browser/devtools/client/netmonitor/test/shared-head.js", this);

What about appending shared-head to webconsole's browser.ini? (so, we don't have to do it in every test)
Attachment #8922667 - Flags: review?(odvarko) → review+
Finally managed to get a green try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=55cfcf02655c48a3346784364be207df32cfd65a&selectedJob=143637865
(I don't know why, eslint fails with issues in /services/ but I checked locally and don't see any eslint issue in /devtools/)
Attachment #8922666 - Attachment is obsolete: true
Attachment #8922667 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d8a6d24cec9
Use request.mimeType instead of request.responseContent.content.mimeType. r=Honza
https://hg.mozilla.org/integration/autoland/rev/7dcfe8d12d6f
Fetch response content only on-demand. r=Honza
https://hg.mozilla.org/integration/autoland/rev/3e7a6e920c6b
Move image preview tooltip to file name and remove preview icon. r=Honza
(In reply to Noemi Erli[:noemi_erli] from comment #84)
> Backed out 3 changesets (bug 1404917)for failing clipboard in
> devtools/client/netmonitor/src/har/test/browser_net_har_copy_all_as_har.js
> r=backout on a CLOSED TREE

HAR codebase depends on responseContent being available synchronously:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/har/har-builder.js#332

This codebase should also be converted to use connector.requestData.
It already does some similar lazy fetching, with "fetchData" which translate LongString clients into strings.
Flags: needinfo?(poirot.alex)
Comment on attachment 8927795 [details]
Bug 1404917 - Fetch responceContent for HAR lazily.

https://reviewboard.mozilla.org/r/199088/#review204090

::: devtools/client/netmonitor/src/har/har-automation.js:171
(Diff revision 1)
>      let items = this.collector.getItems();
>      let form = this.toolbox.target.form;
>      let title = form.title || form.url;
>  
>      let options = {
> +      requestData: null,

Honza, I still need to figure this out.
Is this code tested?
(we will see I launched try with this patch)
Can I assume the netmonitor panel is already opened when this code runs?
I don't see how I could get access to connector's requestData function otherwise?
Just before landing, I pushed again to Talos and got this result:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=443f4336da9e450177c44a7b89ce1d88ffe7310d&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTimeRange=172800

The network monitor test clearly overlaps with all next tests, making them slower.
With this patch there is a 15% on many tests, including tests on other panels.
May be that's just because there is less GC work? or it may hide something wrong in netmonitor, like actor still computing something even if we closed and reopened the toolbox...
(In reply to Alexandre Poirot [:ochameau] from comment #90)
> Honza, I still need to figure this out.
> Is this code tested?
I am not seeing any test covering automated HAR export (all tests for HAR are in netmonitor/src/har/test)

> (we will see I launched try with this patch)
> Can I assume the netmonitor panel is already opened when this code runs?
No, but the toolbox is opened.

> I don't see how I could get access to connector's requestData function
> otherwise?
Automated export implements its own object that is responsible for fetching data from the backend - HarCollector. You should use this one to fetch the response body. Ideally, HarCollector is based on (or entirely replaced by) FirefoxDataProvider, which would make the `requestData` method easily accessible (I imagine HarCollector could create internal instance of FirefoxDataProvider and use it to fetch all the data). All that can be done in another bug report. For now you can just use `HarCollector.getData`, but note that HarCollector doesn't use lazy fetching the response body should be fetched automatically (if `devtools.netmonitor.har.includeResponseBodies` pref is on).

Honza
(In reply to Alexandre Poirot [:ochameau] from comment #91)
> Just before landing, I pushed again to Talos and got this result:
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=mozilla-
> central&newProject=try&newRevision=443f4336da9e450177c44a7b89ce1d88ffe7310d&o
> riginalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec6
> 6500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTi
> meRange=172800
> 
> The network monitor test clearly overlaps with all next tests, making them
> slower.
> With this patch there is a 15% on many tests, including tests on other
> panels.
> May be that's just because there is less GC work? or it may hide something
> wrong in netmonitor, like actor still computing something even if we closed
> and reopened the toolbox...
If I understand correctly Net tests are still running when next tests start?
This could mean that net tests are finishing too soon
(before RDP traffic is all done).

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #93)
> (In reply to Alexandre Poirot [:ochameau] from comment #90)
> > Honza, I still need to figure this out.
> > Is this code tested?
> I am not seeing any test covering automated HAR export (all tests for HAR
> are in netmonitor/src/har/test)

Yes, try was green (except the data uri failure, which are unrelated to HAR)

> > I don't see how I could get access to connector's requestData function
> > otherwise?
> Automated export implements its own object that is responsible for fetching
> data from the backend - HarCollector. You should use this one to fetch the
> response body. Ideally, HarCollector is based on (or entirely replaced by)
> FirefoxDataProvider, which would make the `requestData` method easily
> accessible (I imagine HarCollector could create internal instance of
> FirefoxDataProvider and use it to fetch all the data). All that can be done
> in another bug report. For now you can just use `HarCollector.getData`, but
> note that HarCollector doesn't use lazy fetching the response body should be
> fetched automatically (if `devtools.netmonitor.har.includeResponseBodies`
> pref is on).

Ah, I didn't realized it worked that way. I wasn't aware of HarCollector module.
I should keep checking for file.responseContent in buildContent
and only call requestData if it is not set, like this:
  https://reviewboard.mozilla.org/r/199088/diff/1-2/
Could you confirm it still work fine? I don't know exactly how this feature works.
Comment on attachment 8927795 [details]
Bug 1404917 - Fetch responceContent for HAR lazily.

https://reviewboard.mozilla.org/r/199088/#review204114

Tested on my machine and automated HAR export works fine
R+
Thanks Alex!

Honza
Attachment #8927795 - Flags: review?(odvarko) → review+
Comment on attachment 8927804 [details]
Bug 1404917 - Fix copy image as Data URI context menu.

https://reviewboard.mozilla.org/r/199100/#review204116

Looks good, R+

Honza
Attachment #8927804 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #93)
> Automated export implements its own object that is responsible for fetching
> data from the backend - HarCollector. You should use this one to fetch the
> response body. Ideally, HarCollector is based on (or entirely replaced by)
> FirefoxDataProvider, which would make the `requestData` method easily
> accessible (I imagine HarCollector could create internal instance of
> FirefoxDataProvider and use it to fetch all the data). All that can be done
> in another bug report.

A follow up created:
Bug 1416748 - Automated HAR export should use FirefoxDataProvider

Honza
Attachment #8927795 - Attachment is obsolete: true
Attachment #8927804 - Attachment is obsolete: true
(In reply to Jan Honza Odvarko [:Honza] from comment #94)
> If I understand correctly Net tests are still running when next tests start?
> This could mean that net tests are finishing too soon
> (before RDP traffic is all done).

May be. That, or we overload Firefox GC and make it slow for a couple of tests running after it...
It is always hard to know if we are waiting for everything correctly.
To me, requestsFinished should be enough to wait for everything...
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 685b71751dac -d 7f3e2d8c9991: rebasing 433648:685b71751dac "Bug 1404917 - Use request.mimeType instead of request.responseContent.content.mimeType. r=Honza"
rebasing 433649:359901c9f77c "Bug 1404917 - Fetch response content only on-demand. r=Honza"
merging devtools/client/webconsole/new-console-output/test/mochitest/head.js
warning: conflicts while merging devtools/client/webconsole/new-console-output/test/mochitest/head.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
It's just a few days before the next branch, should we land these large patch after the upcoming branch?
Flags: needinfo?(poirot.alex)
Merge day (2017-11-13) has been branched out yesterday. Now Nightly is on Fx59.
> Merge day (2017-11-13) has been branched out yesterday. Now Nightly is on
> Fx59.

Thanks, that make sense
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/652c97b47537
Use request.mimeType instead of request.responseContent.content.mimeType. r=Honza
https://hg.mozilla.org/integration/autoland/rev/291c051b7861
Fetch response content only on-demand. r=Honza
https://hg.mozilla.org/integration/autoland/rev/e1515b5c14e2
Move image preview tooltip to file name and remove preview icon. r=Honza
Last push to DAMP on linux 64:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=f71c357ac4fe5b9d30780b61f9200b78a073609e&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTimeRange=172800

5% win on DAMP summary
Many >8% on many tests, most likely overlap of netmonitor.reload on tests running after it.

But here is the important numbers:
-14.31% on complicated.requestsFinished
-10% on simple.jsdebugger.reload
-8% on simple.inspector.reload
Flags: needinfo?(poirot.alex)
Confirming comment 112 with results from Perfherder:

== Change summary for alert #10548 (as of Tue, 14 Nov 2017 06:49:38 GMT) ==

Improvements:

  5%  damp summary linux64 opt e10s     286.87 -> 272.04
  2%  damp summary linux64 pgo e10s     249.59 -> 244.43

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10548
Blocks: 1404929
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: