Alexandre Poirot [:ochameau] Reporter
	Description • 7 years ago

It is not clear how we get to the current state where we force loading everything upfront. We load all response content, cookies, headers, ... immediately, without waiting for response/cookies/headers panel to be opened.

But we should lazy load most of the data from the actors. We should only pull what is strictly necessary to display the current UI and fetch additionnal data as user navigate into netmon UI.

Lazy loading only responseContent, which I expect to be the most significant one saves 17% on document reload:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=9519c179cca41b745c87502f994b8d7cca2a74c9&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&framework=1&selectedTimeRange=172800

	Alexandre Poirot [:ochameau] Reporter
	Comment 1 • 7 years ago

I opened a bunch of sub bugs to track individual requests we should do lazily.
But I feel some requests will be harder to lazify due to the recent filter feature which is able to filter by headers and cookies. So I haven't opened bugs to lazy load: request/response headers and cookies. But ideally we should also lazy load that.

	gasolin@mozilla.com
	Comment 2 • 7 years ago

Remove optional NetworkEventUpdates (requestCookies, requestPostData, securityInfo, responseHeaders, responseCookies) from the first run
https://reviewboard.mozilla.org/r/194564/diff/18/

has improvements to 44%(high) for simple & 16%(high) for complex netmonitor requestsFinished DAMP cases
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=132380ae5a4a69e10c29fba6b6a3ab434abf7fe3&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800

Though some non-default columns will use these optional data (ex: requestCookies) , so the plan is

1. When file a new addRequest action -> create an entry in state.requests and an entry in state.requestActors
2. when networkEventUpdate get non-first-run needed data (ex: requestCookies, securityInfo) -> log a event ready state in state.requestActors (or maybe we don't need this if actor request fail is fine)
3. When user open related panel that use those data -> check the request readiness from state.requestActors -> do getRequestData, update the state.requests and marked state.requestActors as fetched.


Or, if we can neglect the networkEventUpdate ready state(that might not the case since we need show some of them when certain option column is shown), we can do:

When user open related panel that use those data -> do getRequestData directly, update the state.requests

I'll test with bug 1404929 first.

	Alexandre Poirot [:ochameau] Reporter
	Comment 3 • 7 years ago

(In reply to Fred Lin [:gasolin] from comment #2)
> Remove optional NetworkEventUpdates (requestCookies, requestPostData,
> securityInfo, responseHeaders, responseCookies) from the first run
> https://reviewboard.mozilla.org/r/194564/diff/18/
> 
> has improvements to 44%(high) for simple & 16%(high) for complex netmonitor
> requestsFinished DAMP cases

You are seeing what I saw back then in comment 0.

> 1. When file a new addRequest action -> create an entry in state.requests
> and an entry in state.requestActors
> 2. when networkEventUpdate get non-first-run needed data (ex:
> requestCookies, securityInfo) -> log a event ready state in
> state.requestActors (or maybe we don't need this if actor request fail is
> fine)
> 3. When user open related panel that use those data -> check the request
> readiness from state.requestActors -> do getRequestData, update the
> state.requests and marked state.requestActors as fetched.
> 
> 
> Or, if we can neglect the networkEventUpdate ready state(that might not the
> case since we need show some of them when certain option column is shown),
> we can do:
> 
> When user open related panel that use those data -> do getRequestData
> directly, update the state.requests
> 
> I'll test with bug 1404929 first.

You should base bug 1404929 on top of bug 1404917.

It introduces helpers to fetch data lazily:
  await connector.requestData(request.id, "responseContent");

About the readiness and networkEventUpdate ready state, yes, may be we can ignore that.
In bug 1404917, I chose to have a "responseContentAvailable" field on the request,
we may add one such attribute but each field. Or have only one that will be true once we received all the networkEventUpdate?

	gasolin@mozilla.com
	Comment 4 • 7 years ago

> You should base bug 1404929 on top of bug 1404917.
> 
> It introduces helpers to fetch data lazily:
>   await connector.requestData(request.id, "responseContent");
> 

I choose not to start from `responseContent` but from the `optional` networkEventUpdate because of change `responseContent` might affect the presence on the first-run page.

> About the readiness and networkEventUpdate ready state, yes, may be we can
> ignore that.
> In bug 1404917, I chose to have a "responseContentAvailable" field on the
> request,

Sounds like a good idea, will take a look on that

> we may add one such attribute but each field. Or have only one that will be
> true once we received all the networkEventUpdate?

Some of networkEventUpdate are optional so we won't get all networkEventUpdate events for each request. That means we can't have only one attribute for this purpose.

We will always get these networkEventUpdate events (requestHeaders,requestCookies, eventTimings, responseContent)
which is defined in the rdpRequestMap
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/connector/firefox-data-provider.js#319

	gasolin@mozilla.com
	Comment 5 • 7 years ago

I was wondering if my assumption (some networkEventUpdate type are optional) is right so I logged the networkEventUpdate.
I found all networkEventUpdate type will be received.
But also surprisingly found that some networkEventUpdate types are sent twice for the same actor(request).

Honza, do you have idea why eventUpdate for the same request will send twice?

The following sample is digested from the first 5 requests(actor) from http://www.bbc.com/news
(the networkEventUpdate receive order is re-grouped by actor for easier read. Per actor received event list is still with the receive order)

>>> 1 actor is server1.conn0.child1/netEvent32
>>> server1.conn0.child1/netEvent32: requestHeaders
>>> server1.conn0.child1/netEvent32: requestCookies
>>> server1.conn0.child1/netEvent32: responseStart
>>> server1.conn0.child1/netEvent32: securityInfo
>>> server1.conn0.child1/netEvent32: responseHeaders
>>> server1.conn0.child1/netEvent32: responseCookies
>>> server1.conn0.child1/netEvent32: eventTimings
>>> server1.conn0.child1/netEvent32: requestHeaders
>>> server1.conn0.child1/netEvent32: requestCookies
>>> server1.conn0.child1/netEvent32: responseStart
>>> server1.conn0.child1/netEvent32: securityInfo
>>> server1.conn0.child1/netEvent32: responseHeaders
>>> server1.conn0.child1/netEvent32: responseCookies
>>> server1.conn0.child1/netEvent32: eventTimings
>>> server1.conn0.child1/netEvent32: responseContent
>>> 2 actor is server1.conn0.child1/netEvent33
>>> server1.conn0.child1/netEvent33: requestHeaders
>>> server1.conn0.child1/netEvent33: requestCookies
>>> server1.conn0.child1/netEvent33: responseStart
>>> server1.conn0.child1/netEvent33: eventTimings
>>> server1.conn0.child1/netEvent33: securityInfo
>>> server1.conn0.child1/netEvent33: responseHeaders
>>> server1.conn0.child1/netEvent33: responseCookies
>>> server1.conn0.child1/netEvent33: requestHeaders
>>> server1.conn0.child1/netEvent33: requestCookies
>>> server1.conn0.child1/netEvent33: responseStart
>>> server1.conn0.child1/netEvent33: eventTimings
>>> server1.conn0.child1/netEvent33: securityInfo
>>> server1.conn0.child1/netEvent33: responseHeaders
>>> server1.conn0.child1/netEvent33: responseCookies
>>> server1.conn0.child1/netEvent33: responseContent
>>> 3 actor is server1.conn0.child1/netEvent35
>>> server1.conn0.child1/netEvent35: requestHeaders
>>> server1.conn0.child1/netEvent35: requestCookies
>>> server1.conn0.child1/netEvent35: responseStart
>>> server1.conn0.child1/netEvent35: securityInfo
>>> server1.conn0.child1/netEvent35: responseHeaders
>>> server1.conn0.child1/netEvent35: responseCookies
>>> server1.conn0.child1/netEvent35: eventTimings
>>> server1.conn0.child1/netEvent35: responseContent
>>> 4 actor is server1.conn0.child1/netEvent36
>>> server1.conn0.child1/netEvent36: requestHeaders
>>> server1.conn0.child1/netEvent36: requestCookies
>>> server1.conn0.child1/netEvent36: responseStart
>>> server1.conn0.child1/netEvent36: eventTimings
>>> server1.conn0.child1/netEvent36: securityInfo
>>> server1.conn0.child1/netEvent36: responseHeaders
>>> server1.conn0.child1/netEvent36: responseCookies
>>> server1.conn0.child1/netEvent36: responseContent

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 6 • 7 years ago

(In reply to Fred Lin [:gasolin] from comment #5)
> I was wondering if my assumption (some networkEventUpdate type are optional)
> is right so I logged the networkEventUpdate.
> I found all networkEventUpdate type will be received.
> But also surprisingly found that some networkEventUpdate types are sent
> twice for the same actor(request).
> 
> Honza, do you have idea why eventUpdate for the same request will send twice?
This is weird, I never noticed that (and I was logging a lot these things).
Where exactly did you put the console.log statement?

Honza

	Alexandre Poirot [:ochameau] Reporter
	Comment 7 • 7 years ago

(In reply to Fred Lin [:gasolin] from comment #5)
> I was wondering if my assumption (some networkEventUpdate type are optional)
> is right so I logged the networkEventUpdate.
> I found all networkEventUpdate type will be received.
> But also surprisingly found that some networkEventUpdate types are sent
> twice for the same actor(request).
> 
> Honza, do you have idea why eventUpdate for the same request will send twice?

That is surprising. May be a bug on the actor side?

You can debug that here:
http://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole.js#2144
See first if, from the actor side, we also do have two requestHeaders per request?

If that's the case, track down the two callsites of createNetworkEvent():
(which is the only one function calling `addRequestHeaders`)
http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#900
http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#1162

May be we get a "http-on-examine-cached-response" *and* an "observeActivity" call for some requests??

	gasolin@mozilla.com
	Comment 8 • 7 years ago

I use this script to log all networkEventUpdate type from the first 5 actor. 
Though I can't reproduce the same `send twice events` this morning.
All events get 8 non-repeated networkEventUpdate types now.

	Alexandre Poirot [:ochameau] Reporter
	Comment 9 • 7 years ago

Now that responseContent is landed, let's look at how much it would improve netmonitor perf to load other fields lazily.

# request and response cookies
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f71c357ac4fe&newProject=try&newRevision=660f5c1a4cfff4b89154ee730b011236e7fd118d&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1
 complicated.requestsFinished  -8.64%
 simple.requestsFinished  -14.63%

# request and response headers
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f71c357ac4fe&newProject=try&newRevision=e7d5de936ce1223fbcf597ee096f4d00f3d83139&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1
 complicated.requestsFinished  -13.23%
 simple.requestsFinished  -24.29%

So I think it is really worth doing something about them.
Note that we don't *have to* strip features related to them.
We could, with some additional work, just ensure that all these data is fetched only when strictly necessary:
 * when a filter related to any of these fields is used (I imagine filter box isn't a fulltext search where we process all fields? We do have a keyword right?)
 * when cookies columns are displayed

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 10 • 7 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #9)
> We could, with some additional work, just ensure that all these data is
> fetched only when strictly necessary:
>  * when a filter related to any of these fields is used (I imagine filter
> box isn't a fulltext search where we process all fields? We do have a
> keyword right?)
>  * when cookies columns are displayed

Yes precisely, we were discussing this at our meeting today.

Honza