Closed Bug 1329165 Opened 6 years ago Closed 6 years ago

Optimize Immutable.js usage in Netmonitor

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Iteration:
53.5 - Jan 23
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: nobody → jsnajdr
Whiteboard: [netmonitor-triage]
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.
Status: NEW → ASSIGNED
Iteration: --- → 53.4 - Jan 9
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor-triage] → [netmonitor]
No longer blocks: 1328553
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
(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.
Flags: qe-verify? → qe-verify-
Iteration: 53.4 - Jan 9 → 53.5 - Jan 23
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 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
https://hg.mozilla.org/mozilla-central/rev/b6e73db48ca6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.