Closed
Bug 1329165
Opened 6 years ago
Closed 6 years ago
Optimize Immutable.js usage in Netmonitor
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox53 fixed)
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: jsnajdr, Assigned: jsnajdr)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor])
Attachments
(1 file)
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•6 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59ddaa713b70
Assignee | ||
Comment 3•6 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•6 years ago
|
Status: NEW → ASSIGNED
Iteration: --- → 53.4 - Jan 9
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor-triage] → [netmonitor]
Comment 4•6 years ago
|
||
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•6 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•6 years ago
|
Flags: qe-verify? → qe-verify-
Updated•6 years ago
|
Iteration: 53.4 - Jan 9 → 53.5 - Jan 23
Comment 6•6 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•6 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.
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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b6e73db48ca6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•