1419366 - NetMonitor: Stop using ImmutableJS in filters reducer

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 1

[mandercs3]

Hi Jan, I'm free to take a look at this.

Comment 2

[[e-mail]]

Cool! If you need any help, please let us know!

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Another example showing how to remove ImmutableJS is here: bug 1419369

Honza

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@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

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@mandercs: Still interested in this bug?
Honza

Comment 6

[avi.mathur.engg+github]

I am interested in this one. Can I go ahead ?

Comment 7

[avi.mathur.engg+github]

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

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 9

[Kristiyan]

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?

Comment 10

[Kristiyan]

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.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 12

[Nathan Zengamambu]

Hello, I would like to participate in this issue. On which line(s) of code are the errors present?

Comment 13

[Kristiyan]

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

[Kristiyan]

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

[Kristiyan]

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.

Comment 16

[Abhinav Koppula]

@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 17

[Abhinav Koppula]

Attachment: Bug 1419366 - Stop using ImmutableJS in filters reducer.

Review commit: https://reviewboard.mozilla.org/r/208044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208044/

Comment 18

[Abhinav Koppula]

Hi Honza,
I have taken a stab at this issue. Please let me know if this looks fine.

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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...

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 21

[Abhinav Koppula]

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.

Comment 22

[Abhinav Koppula]

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 23

[Abhinav Koppula]

Comment on attachment 8937364 [details]
Bug 1419366 - Stop using ImmutableJS in filters reducer.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208044/diff/1-2/

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 25

[Abhinav Koppula]

TRY push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f3dedeae1a118d9b2013493ba40e4aed577cce1

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de10180be095
Stop using ImmutableJS in filters reducer. r=Honza

Comment 27

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/de10180be095