1329165 - Optimize Immutable.js usage in Netmonitor

Status: RESOLVED FIXED

Description

[Jarda Snajdr [:jsnajdr]]

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.

Comment 1

[Jarda Snajdr [:jsnajdr]]

Created attachment 8824381 [details]
Bug 1329165 - Use Immutable.Map for request list, optimize sorting

Review commit: https://reviewboard.mozilla.org/r/102906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102906/

Comment 2

[Jarda Snajdr [:jsnajdr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59ddaa713b70

Comment 3

[Jarda Snajdr [:jsnajdr]]

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.

Comment 4

[Ricky Chien [:rickychien] (inactive)]

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

Comment 5

[Jarda Snajdr [:jsnajdr]]

(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.

Comment 6

[Jan Honza Odvarko [:Honza] (need-info? me)]

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

Comment 7

[Jarda Snajdr [:jsnajdr]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102906/diff/1-2/

Comment 8

[Jarda Snajdr [:jsnajdr]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6e73db48ca6
Use Immutable.Map for request list, optimize sorting r=Honza

Comment 10

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/b6e73db48ca6