1419367 - NetMonitor: Stop using ImmutableJS in sort 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 sort reducer.
devtools/client/netmonitor/src/reducers/sort.js 

It should be replaced by plain JS structures.

See bug 1408182 as an example.

Honza

Comment 1

[Abhinav Koppula]

Attachment: Bug 1419367 - NetMonitor: Stop using ImmutableJS in sort reducer.

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

Comment 2

[Abhinav Koppula]

Hi Honza,
I've taken a stab at this issue. Is this what you had in mind?

Comment 3

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

Comment on attachment 8932713 [details]
Bug 1419367 - NetMonitor: Stop using ImmutableJS in sort reducer.

https://reviewboard.mozilla.org/r/203766/#review209370

Thanks for working on this!

Please see my inline comments

Honza

::: devtools/client/netmonitor/src/reducers/sort.js:13
(Diff revision 1)
>  
> -const Sort = I.Record({
> +const Sort = {
>    // null means: sort by "waterfall", but don't highlight the table header
>    type: null,
>    ascending: true,
> -});
> +};

Implement this as a function returning an object, so it's consistent with the other reducers.

function Sort() {
  return {
    type: null,
    ascenging: true,
  }
}

::: devtools/client/netmonitor/src/reducers/sort.js:15
(Diff revision 1)
>    // null means: sort by "waterfall", but don't highlight the table header
>    type: null,
>    ascending: true,
> -});
> +};
>  
> -function sortReducer(state = new Sort(), action) {
> +function sortReducer(state = Object.assign({}, Sort), action) {

... and so you don't need the Object.assign() here.

::: devtools/client/netmonitor/src/reducers/sort.js:18
(Diff revision 1)
> -});
> +};
>  
> -function sortReducer(state = new Sort(), action) {
> +function sortReducer(state = Object.assign({}, Sort), action) {
>    switch (action.type) {
>      case SORT_BY: {
> -      return state.withMutations(st => {
> +      state = Object.assign({}, state);

Please use spread operator instead of Object.assign()

::: devtools/client/netmonitor/src/utils/create-store.js:34
(Diff revision 1)
>    const initialState = {
>      filters: new Filters({
>        requestFilterTypes: getFilterState()
>      }),
>      requests: new Requests(),
> -    sort: new Sort(),
> +    sort: Object.assign({}, Sort),

And you can do the following now:
sort: Sort(),

Comment 4

[Abhinav Koppula]

Comment on attachment 8932713 [details]
Bug 1419367 - NetMonitor: Stop using ImmutableJS in sort reducer.

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

Comment 5

[Abhinav Koppula]

Comment on attachment 8932713 [details]
Bug 1419367 - NetMonitor: Stop using ImmutableJS in sort reducer.

https://reviewboard.mozilla.org/r/203766/#review209370

Hi Honza,
Thanks for the review. I have addressed the comments.

Comment 6

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

Comment on attachment 8932713 [details]
Bug 1419367 - NetMonitor: Stop using ImmutableJS in sort reducer.

https://reviewboard.mozilla.org/r/203766/#review210100

Looks good, thanks!
R+
Honza

Comment 7

[Pulsebot]

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

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2cf30748e1c1