Optimize Immutable.js usage in Netmonitor

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: jsnajdr, Assigned: jsnajdr)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [netmonitor])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

11 months 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

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

Comment 2

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59ddaa713b70
(Assignee)

Comment 3

11 months 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

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

Updated

11 months ago
No longer blocks: 1328553
(Assignee)

Updated

11 months 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

11 months 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

11 months ago
Flags: qe-verify? → qe-verify-

Updated

11 months ago
Iteration: 53.4 - Jan 9 → 53.5 - Jan 23

Comment 6

11 months 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

11 months 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

11 months 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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6e73db48ca6
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.