NetMonitor: Stop using ImmutableJS in sort reducer

RESOLVED FIXED in Firefox 59

Status

enhancement
P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Honza, Assigned: abhinav.koppula, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: good-first-bug)

Attachments

(1 attachment)

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
Mentor: odvarko
Priority: -- → P2
Whiteboard: good-first-bug
Comment hidden (mozreview-request)
(Assignee)

Comment 2

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

Comment 3

a year ago
mozreview-review
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(),
Attachment #8932713 - Flags: review?(odvarko) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a year ago
mozreview-review-reply
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.
(Reporter)

Comment 6

a year ago
mozreview-review
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
Attachment #8932713 - Flags: review?(odvarko) → review+
(Assignee)

Updated

a year ago
Assignee: nobody → abhinav.koppula
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 7

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/2cf30748e1c1
NetMonitor: Stop using ImmutableJS in sort reducer. r=Honza
Keywords: checkin-needed

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2cf30748e1c1
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.