Optimize Immutable.js usage in Netmonitor

RESOLVED FIXED in Firefox 53

Status

P1
normal
RESOLVED FIXED
2 years ago
5 months ago

People

(Reporter: jsnajdr, Assigned: jsnajdr)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [netmonitor])

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Profiling the React/Redux implemenation of Netmonitor UI with Cleopatra tells me that a lot of time is spent inside Immutable.js:

- finding a record by id in Immutable.List. We do a O(n) sequential search. Switching to Immutable.Map will improve that to O(log n).

- property access in Immutable.Record is expensive - computing hashes, searching in a trie... We need to be more careful about that in code that is executed frequently - like comparators for sorting.
(Assignee)

Updated

2 years ago
Assignee: nobody → jsnajdr
Blocks: 1307743, 1328553
Whiteboard: [netmonitor-triage]
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
The patch does the following:

- use Immutable.Map for the request list. Makes the reducer simpler and faster. Note that the derived lists - results of getSortedRequests and getDisplayedRequests - are still Lists, because they are ordered and need indexed access. They could be plain Arrays in future, if that brings any performance wins - they are never mutated in any way.

- optimize the sorting predicates and the sorting functions to do fewer property accesses, to be more efficient in the "clone" case (very rarely happening), and to have less duplicated code, too.

Updated

2 years ago
Status: NEW → ASSIGNED
Iteration: --- → 53.4 - Jan 9
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor-triage] → [netmonitor]
(Assignee)

Updated

2 years ago
No longer blocks: 1328553
(Assignee)

Updated

2 years ago
Blocks: 1321749
There is an OrderedMap which is able to guarantee the order in which they were set(). So we can keep order and indexed access.

https://facebook.github.io/immutable-js/docs/#/OrderedMap
(Assignee)

Comment 5

2 years ago
(In reply to Ricky Chien [:rickychien] from comment #4)
> There is an OrderedMap which is able to guarantee the order in which they
> were set(). So we can keep order and indexed access.

How would you use OrderedMap in our case? The order in which set() was called has no meaning for us. We order the requests by sorting.

Updated

2 years ago
Flags: qe-verify? → qe-verify-

Updated

2 years ago
Iteration: 53.4 - Jan 9 → 53.5 - Jan 23

Comment 6

2 years ago
mozreview-review
Comment on attachment 8824381 [details]
Bug 1329165 - Use Immutable.Map for request list, optimize sorting

https://reviewboard.mozilla.org/r/102906/#review104146

LGTM

R+

Honza

::: devtools/client/netmonitor/reducers/requests.js:57
(Diff revision 1)
>    responseContent: undefined,
>    responseContentDataUri: undefined,
>  });
>  
>  const Requests = I.Record({
>    // The request list

Update the comment list -> map
Attachment #8824381 - Flags: review?(odvarko) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8824381 [details]
Bug 1329165 - Use Immutable.Map for request list, optimize sorting

https://reviewboard.mozilla.org/r/102906/#review104146

> Update the comment list -> map

Updated, rebased, going to land.

Comment 9

2 years ago
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b6e73db48ca6
Use Immutable.Map for request list, optimize sorting r=Honza

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6e73db48ca6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

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