Closed
Bug 1404913
Opened 7 years ago
Closed 5 years ago
Netmonitor UI should fetch all data lazily
Categories
(DevTools :: Netmonitor, enhancement, P3)
DevTools
Netmonitor
Tracking
(firefox57 fix-optional)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox57 | --- | fix-optional |
People
(Reporter: ochameau, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
1.07 KB,
patch
|
Details | Diff | Splinter Review |
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
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Priority: -- → P3
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.
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.
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?
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
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
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko) → needinfo?(gasolin)
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??
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.
Flags: needinfo?(gasolin)
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
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
Updated•6 years ago
|
Product: Firefox → DevTools
Comment 11•5 years ago
|
||
Looks like no dependencies remain here, closing for now.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(odvarko)
Resolution: --- → FIXED
Comment 12•5 years ago
|
||
Yes, agree. The Net panel is now loading all data lazily, thanks Harald.
Honza
Flags: needinfo?(odvarko)
You need to log in
before you can comment on or make changes to this bug.
Description
•