Closed
Bug 1419366
Opened 5 years ago
Closed 5 years ago
NetMonitor: Stop using ImmutableJS in filters 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, NeedInfo)
References
Details
(Keywords: good-first-bug)
Attachments
(1 file)
ImmutableJS is slow and we should stop using it in NetMonitor's filters reducer. devtools/client/netmonitor/src/reducers/filters.js It should be replaced by plain JS structures. See bug 1408182 as an example. Honza
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 3•5 years ago
|
||
Another example showing how to remove ImmutableJS is here: bug 1419369 Honza
Reporter | ||
Comment 4•5 years ago
|
||
@mandercs: Hi! Any progress on this bug? Do you need help? It would be great to have this fixed (it's the last piece related to ImmutableJS removal in the Net monitor) Honza
Reporter | ||
Comment 5•5 years ago
|
||
@mandercs: Still interested in this bug? Honza
Flags: needinfo?(mandercs3)
Comment 7•5 years ago
|
||
I was just changed I.Record in filters.js. but TypeError: this.panelWin.Netmonitor is undefined Stack trace: open@resource://devtools/shared/base-loader.js -> resource://devtools/client/netmonitor/panel.js:17:5 async*onLoad@resource://devtools/shared/base-loader.js -> resource://devtools/client/framework/toolbox.js:1667:19 Promise-backend.js:925 Error is thrown at console. So I reverted my changes and still this error is present. I checked hg status also, it does not show any changes regarding net-monitor. What mistake am I doing ? Is net-monitor already broken in current builds ? Thanks, Avi
Flags: needinfo?(odvarko)
Reporter | ||
Comment 8•5 years ago
|
||
(In reply to avi.mathur.engg+github from comment #7) > I was just changed I.Record in filters.js. but > TypeError: this.panelWin.Netmonitor is undefined > Stack trace: > open@resource://devtools/shared/base-loader.js -> > resource://devtools/client/netmonitor/panel.js:17:5 > async*onLoad@resource://devtools/shared/base-loader.js -> > resource://devtools/client/framework/toolbox.js:1667:19 > Promise-backend.js:925 > > Error is thrown at console. So I reverted my changes and still this error is > present. I checked hg status also, it does not show any changes regarding > net-monitor. > > What mistake am I doing ? Is net-monitor already broken in current builds ? No, the Network panel is working properly. Honza
I am trying to adapt it without immutable, but I am having a bit of trouble with understanding why "let { filter }" has the curly brackets. Could you help me with that one?
Flags: needinfo?(odvarko)
Comment 10•5 years ago
|
||
Are you sure the original file is even working? It gives me a lot of error when ran and on top of that the last function should be running at all. It has a predefined parameter as the first parameter. As far as I know that should trow an error, the predefined parameters need to be in the end of the function declaration.
Reporter | ||
Comment 11•5 years ago
|
||
(In reply to Kristiyan from comment #9) > I am trying to adapt it without immutable, but I am having a bit of trouble > with understanding why "let { filter }" has the curly brackets. Could you > help me with that one? See this MDN page: https://developer.mozilla.org/cs/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment (In reply to Kristiyan from comment #10) > Are you sure the original file is even working? It gives me a lot of error > when ran and on top of that the last function should be running at all. It > has a predefined parameter as the first parameter. As far as I know that > should trow an error, the predefined parameters need to be in the end of the > function declaration. Can you attach the error you are seeing? Can you provide some more details about how do you run/build the net panel? Honza
Flags: needinfo?(odvarko)
Comment 12•5 years ago
|
||
Hello, I would like to participate in this issue. On which line(s) of code are the errors present?
Flags: needinfo?(odvarko)
Comment 13•5 years ago
|
||
Hey Nathan, i'm trying to do it my self, but you are free to try as well. It's not about which lines, but rather to adapt filters.js in a way that it doesn't use immutable. We can collaborate on it if you wish, after i catch up on Honza's feedback i will upload my progress so you can take a look at it. Message me for more info if you need :).
Comment 14•5 years ago
|
||
Sorry, I am quite new to this. How do I run the net panel? I am not really sure how I am supposed to test.
Comment 15•5 years ago
|
||
Could you as well take a look at the last function with the default parameter, I can't see a way of this working, I spend a good 2 hours on trying to find if it's possible to have a default parameter and then an empty one, I dot see this(function filters(state = new Filters(), action)) happening and i don't want to switch them in case it works somehow and I mess it up.
Assignee | ||
Comment 16•5 years ago
|
||
@Kristiyan, This is the net-monitor - https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor How you run it - Open Firefox Nightly, press F12 and select "Network" tab In the above link - check the "Filtering requests" section. So after making changes to the filters reducer, you can check if filtering works fine even after your changes.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•5 years ago
|
||
Hi Honza, I have taken a stab at this issue. Please let me know if this looks fine.
Reporter | ||
Comment 19•5 years ago
|
||
mozreview-review |
Comment on attachment 8937364 [details] Bug 1419366 - Stop using ImmutableJS in filters reducer. https://reviewboard.mozilla.org/r/208044/#review214146 Thanks for working on this, please see my inline comments. Honza ::: devtools/client/netmonitor/src/reducers/filters.js:21 (Diff revision 1) > - .reduce((o, tag) => Object.assign(o, { [tag]: false }), {}) > -); > + overrideParams = Object.keys(overrideParams) > + .filter(key => allFilterTypes.includes(key)) > + .reduce((obj, key) => { > + obj[key] = overrideParams[key]; > + return obj; > + }, {}); I don't see the filtering as part of the previous implementation, or was it done as part of ImmutableJS impl? Is it kind of a safety-check for filtering-out obsolete filter IDs (coming from old prefs)? Can you creat some explanatory comments? ::: devtools/client/netmonitor/src/reducers/filters.js:23 (Diff revision 1) > + .reduce((obj, key) => { > + obj[key] = overrideParams[key]; > + return obj; > + }, {}); > + let filterTypes = allFilterTypes > + .reduce((o, tag) => Object.assign(o, { [tag]: false }), {}); This line needs indentation. Also, why you are using an empty object as the third argument? ::: devtools/client/netmonitor/src/reducers/filters.js:56 (Diff revision 1) > - if (!newState.includes(true)) { > + for (const key in newState) { > + includesTrue = includesTrue || newState[key] === true; > + } > + if (!includesTrue) { > newState = new FilterTypes({ all: true }); > } Could we simplify the code above? Remove the for-loop and use something like: `if (!newState.values().includes(true))` ... ::: devtools/client/netmonitor/src/selectors/requests.js:45 (Diff revision 1) > filters => r => { > - const matchesType = filters.requestFilterTypes.some((enabled, filter) => { > - return enabled && Filters[filter] && Filters[filter](r); > - }); > + let matchesType = false; > + for (const key in filters.requestFilterTypes) { > + let enabled = filters.requestFilterTypes[key]; > + matchesType = matchesType || (enabled && Filters[key] && Filters[key](r)); > + } Can we avoid for-loop and use rather something like: `filters.requestFilterTypes.values().some(...)` ::: devtools/client/netmonitor/src/selectors/requests.js:55 (Diff revision 1) > const getTypeFilterFn = createSelector( > state => state.filters, > filters => r => { > - const matchesType = filters.requestFilterTypes.some((enabled, filter) => { > - return enabled && Filters[filter] && Filters[filter](r); > - }); > + let matchesType = false; > + for (const key in filters.requestFilterTypes) { > + let enabled = filters.requestFilterTypes[key]; The same as above, can we avoid for-loop...
Attachment #8937364 -
Flags: review?(odvarko) → review-
Reporter | ||
Comment 20•5 years ago
|
||
(In reply to Nathan Zengamambu from comment #12) > Hello, I would like to participate in this issue. On which line(s) of code > are the errors present? Nathan, Kristiyan, I am assigning this to Abhinav since he submitted a patch already, but ping me (email) if you are interested in another bug and I'll find something for you! Honza
Assignee: mandercs3 → abhinav.koppula
Flags: needinfo?(odvarko)
Assignee | ||
Comment 21•5 years ago
|
||
mozreview-review-reply |
Comment on attachment 8937364 [details] Bug 1419366 - Stop using ImmutableJS in filters reducer. https://reviewboard.mozilla.org/r/208044/#review214146 > I don't see the filtering as part of the previous implementation, or was it done as part of ImmutableJS impl? > > Is it kind of a safety-check for filtering-out obsolete filter IDs (coming from old prefs)? Can you creat some explanatory comments? Yes, this was taken care of as part of ImmutableJS impl, since I had to add this piece of code as the below test was failing with the following failures: TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The button should not have a 'checked' class. - Got true, expected false TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The button should set 'aria-pressed' = false. - Got true, expected false TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The button should not have a 'checked' class. - Got true, expected false TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The button should set 'aria-pressed' = false. - Got true, expected false TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_filter-04.js | The filters preferences were saved directly after the click and only with the valid. - Got ["html","js","bogus","alsobogus"], expected ["html","js"] The above piece of code was added more specifically to ensure that this test passes as before.
Assignee | ||
Comment 22•5 years ago
|
||
mozreview-review-reply |
Comment on attachment 8937364 [details] Bug 1419366 - Stop using ImmutableJS in filters reducer. https://reviewboard.mozilla.org/r/208044/#review214146 > This line needs indentation. > > Also, why you are using an empty object as the third argument? Reg the need for the empty object as third argument: So I was keeping it similar to what it was before. Before my change we had: const FilterTypes = I.Record(["all"] .concat(FILTER_TAGS) .reduce((o, tag) => Object.assign(o, { [tag]: false }), {}) ); Also, if I omit the third parameter of empty object, it results in this -> https://snag.gy/HEePxT.jpg Your thoughts on this?
Comment hidden (mozreview-request) |
Reporter | ||
Comment 24•5 years ago
|
||
mozreview-review |
Comment on attachment 8937364 [details] Bug 1419366 - Stop using ImmutableJS in filters reducer. https://reviewboard.mozilla.org/r/208044/#review214448 (In reply to Abhinav Koppula from comment #22) > Reg the need for the empty object as third argument: > So I was keeping it similar to what it was before. Before my change we had: > const FilterTypes = I.Record(["all"] > .concat(FILTER_TAGS) > .reduce((o, tag) => Object.assign(o, { [tag]: false }), {}) > ); > Also, if I omit the third parameter of empty object, it results in this -> > https://snag.gy/HEePxT.jpg > Your thoughts on this? Ah, I overlooked a bracket, it's the initial value for `reduce` accumulator, not third arg for `Object.assign`, so it's correct! R+ Thanks! Honza
Attachment #8937364 -
Flags: review?(odvarko) → review+
Assignee | ||
Comment 25•5 years ago
|
||
TRY push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f3dedeae1a118d9b2013493ba40e4aed577cce1
Status: NEW → ASSIGNED
Assignee | ||
Updated•5 years ago
|
Keywords: checkin-needed
Comment 26•5 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/de10180be095 Stop using ImmutableJS in filters reducer. r=Honza
Keywords: checkin-needed
Comment 27•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/de10180be095
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•