Closed
Bug 1419368
Opened 6 years ago
Closed 6 years ago
NetMonitor: Stop using ImmutableJS in timing-markers reducer
Categories
(DevTools :: Netmonitor, enhancement, P2)
DevTools
Netmonitor
Tracking
(firefox59 fixed)
RESOLVED
FIXED
Firefox 59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: Honza, Assigned: abhinav.koppula, Mentored)
References
Details
(Whiteboard: good-first-bug)
Attachments
(1 file)
ImmutableJS is slow and we should stop using it in NetMonitor's timing-markers reducer. devtools/client/netmonitor/src/reducers/timing-markers.js It should be replaced by plain JS structures. See bug 1408182 as an example. Honza
Reporter | ||
Updated•6 years ago
|
Mentor: odvarko
Priority: -- → P2
Whiteboard: good-first-bug
Comment hidden (mozreview-request) |
Reporter | ||
Comment 2•6 years ago
|
||
mozreview-review |
Comment on attachment 8932961 [details] Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer. https://reviewboard.mozilla.org/r/203964/#review209436 ::: devtools/client/netmonitor/src/reducers/timing-markers.js (Diff revision 1) > } > > function clearTimingMarkers(state) { > - return state.withMutations(st => { > + return new TimingMarkers(); > - st.remove("firstDocumentDOMContentLoadedTimestamp"); > - st.remove("firstDocumentLoadTimestamp"); Shouldn't we delete these two props just like before?
Assignee | ||
Comment 3•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8932961 [details] Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer. https://reviewboard.mozilla.org/r/203964/#review209436 > Shouldn't we delete these two props just like before? Hi Honza, So I tried this: ``` state = { ...state }; delete state.firstDocumentDOMContentLoadedTimestamp; delete state.firstDocumentLoadTimestamp; return state; ``` but adding the above snippet was resulting in breaking of the functionality of TimingMarkers and also 2 tests were failing. However on just having `return new TimingMarkers();` fixes both of the above problems. I am not sure how the above snippet should be modified to make the functionality work. Can you help me here?
Reporter | ||
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8932961 [details] Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer. https://reviewboard.mozilla.org/r/203964/#review209756 (In reply to Abhinav Koppula from comment #3) > I am not sure how the above snippet should be modified to make the > functionality work. Can you help me here? Does fixing my two inline comments help with this issue? Honza ::: devtools/client/netmonitor/src/reducers/timing-markers.js:23 (Diff revision 1) > +} > > function addTimingMarker(state, action) { > if (action.marker.name === "document::DOMContentLoaded" && > state.firstDocumentDOMContentLoadedTimestamp === -1) { > - return state.set("firstDocumentDOMContentLoadedTimestamp", > + state.firstDocumentDOMContentLoadedTimestamp = action.marker.unixTime / 1000; You are not cloning the state here ::: devtools/client/netmonitor/src/reducers/timing-markers.js:28 (Diff revision 1) > - action.marker.unixTime / 1000); > } > > if (action.marker.name === "document::Load" && > state.firstDocumentLoadTimestamp === -1) { > - return state.set("firstDocumentLoadTimestamp", > + state.firstDocumentLoadTimestamp = action.marker.unixTime / 1000; The same as above, you need to clone the state object here. Please prefer spread operator before Object.assign()
Attachment #8932961 -
Flags: review?(odvarko) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8932961 [details] Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer. https://reviewboard.mozilla.org/r/203964/#review209756 Hi Honza, So after addressing the inline comments also, the functionality doesn't work if we use `delete` to remove the two keys(firstDocumentDOMContentLoadedTimestamp and firstDocumentLoadTimestamp). So I reverted my patch and found out that this is how the functionality works with ImmutableJS. I just logged the values before and after the mutation initialValue - firstDocumentDOMContentLoadedTimestamp = 1512137925747.967 timing-markers.js:36:3 initialValue - firstDocumentLoadTimestamp = 1512137930776.788 timing-markers.js:37:3 finalValue - firstDocumentDOMContentLoadedTimestamp = -1 timing-markers.js:42:3 finalValue - firstDocumentLoadTimestamp = -1 timing-markers.js:43:3``` I think this might explain why using `new TimingMarkers()` restores the original functionality.
Reporter | ||
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8932961 [details] Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer. https://reviewboard.mozilla.org/r/203964/#review210188 Thanks, looks good, just one inline comment Honza ::: devtools/client/netmonitor/src/reducers/timing-markers.js:23 (Diff revision 2) > +} > > function addTimingMarker(state, action) { > if (action.marker.name === "document::DOMContentLoaded" && > state.firstDocumentDOMContentLoadedTimestamp === -1) { > - return state.set("firstDocumentDOMContentLoadedTimestamp", > + state = { ...state }; Please, clone the state just once at the beginning of the method (not within the conditions). ::: devtools/client/netmonitor/src/reducers/timing-markers.js:30 (Diff revision 2) > + return state; > } > > if (action.marker.name === "document::Load" && > state.firstDocumentLoadTimestamp === -1) { > - return state.set("firstDocumentLoadTimestamp", > + state = { ...state }; So, this should be removed.
Attachment #8932961 -
Flags: review?(odvarko) → review-
Comment hidden (mozreview-request) |
Reporter | ||
Comment 9•6 years ago
|
||
mozreview-review |
Comment on attachment 8932961 [details] Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer. https://reviewboard.mozilla.org/r/203964/#review210204 Looks good! R+ assuming try is green Thanks Abhinav! Honza
Attachment #8932961 -
Flags: review?(odvarko) → review+
Assignee | ||
Comment 10•6 years ago
|
||
Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4cb85dd039886a188488adbebd8ef085a72b481c
Assignee | ||
Comment 11•6 years ago
|
||
Hi Honza, There are 2 failures but looks like they are intermittent and not related to this. Can you confirm once from your side?
Flags: needinfo?(odvarko)
Comment 13•6 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ed1e9dcdee4a NetMonitor: Stop using ImmutableJS in timing-markers reducer. r=Honza
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
Comment 14•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ed1e9dcdee4a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•