1370268 - Stop using Immutable.Record for messages types

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

Although Immutable is great and offer some nice functionalities, it also adds some overhead that can slow down the console.
When the message state is normalized, we can replace Immutable in some places: 
- messagesTableDataById: Immutable.Map -> Object
- messagesUiById: Immutable.List -> Array (and we should rename the property to messagesUi)
- groupsById: Immutable.Map -> Object
- messagesById: Immutable.OrderedMap -> Object, but would need to have integer ids so sorting the keys are fast. ( This should be measured and compared to using the Immutable OrderedMap)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Here's a profile when not using immutable for the message slice of the store, with a Map for the messages : 
https://perf-html.io/public/17e8641c6a1781d26e3d293b5c25f85989a71f0d/calltree/?implementation=js&range=22.1051_31.9397&thread=0


And one without the patch:
https://perfht.ml/2rHWyzk

We can see that without Immutable, it's worse. The majority of time is spent on re-creating the map.
It's needed because we redux expect a new reference to pass the property to React.

Maybe what we can do is limit the number of time we need to return a new reference. Which is possible since we already batch message add. We should be able to only have to return a new reference for each batch.
Having a MESSAGES_ADD action could be the best way to do this.


The only advantage of removing Immutable  can be seen is on getRepeatId:
without immutable: https://perf-html.io/public/17e8641c6a1781d26e3d293b5c25f85989a71f0d/calltree/?implementation=js&range=41.7280_52.5399&search=getRepea&thread=0 ~70ms
with immutable : https://perf-html.io/public/1a65a172adfac2fe664d2636dd6c8c76bdf54273/calltree/?implementation=js&search=getRepeatId&thread=0 ~210ms

This is because in the patch we're not using Immutable.record anymore , so we don't have to do the toJS() part which is costly. We can get this number even lower if we find a way not to use Object.assign to get rid of the timestamp, which takes most of the time here.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Re-purposing this bug to stop using Immutable in message type ( See http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/devtools/client/webconsole/new-console-output/types.js#16-59) and just a return a frozen plain object.
Then we could be smarter on how we generate the repeatId (picking properties one by one instead of copying the whole object and overwriting timeStamp)

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

With the new patch:

3 INFO STREAMING: On average, it took 60.593999999999944 ms (median 59.29250000000002 ms) for each message
4 INFO took 6823.005000000001 ms to render bulk messages (iteration 0)
5 INFO took 6622.889999999999 ms to render bulk messages (iteration 1)
6 INFO took 7282.529999999999 ms to render bulk messages (iteration 2)
7 INFO took 7385.360000000001 ms to render bulk messages (iteration 3)
8 INFO took 8071.129999999997 ms to render bulk messages (iteration 4)
9 INFO BULK: On average, it took 7236.982999999998 ms (median 7282.529999999999 ms) to render 4000 messages
10 INFO Filter toggle time (off): 376.45000000000437
11 INFO Filter toggle time (on): 6016.9000000000015

Profile: https://perf-html.io/public/53a8cbf73cc87097a2f120924c71f7763ddb7c34/calltree/?callTreeFilters=prefixjs-5Ag5Ag5Er5AgJiJjy2&implementation=js&range=16.5514_23.6946&thread=0
prepareMessage (which calls repeatId) : 60-80ms for 4000 messages

Without the patch:

3 INFO STREAMING: On average, it took 63.54190000000001 ms (median 60.96499999999969 ms) for each message
4 INFO took 9335.91 ms to render bulk messages (iteration 0)
5 INFO took 9008.095000000001 ms to render bulk messages (iteration 1)
6 INFO took 7742.540000000001 ms to render bulk messages (iteration 2)
7 INFO took 8811.195 ms to render bulk messages (iteration 3)
8 INFO took 7026.185000000005 ms to render bulk messages (iteration 4)
9 INFO BULK: On average, it took 8384.785 ms (median 8811.195 ms) to render 4000 messages
10 INFO Filter toggle time (off): 405.7900000000009
11 INFO Filter toggle time (on): 6088.640000000007

Profile: https://perf-html.io/from-file/calltree/?callTreeFilters=prefixjs-5Bs5Bs5F95BsxqL1xkxlxm&implementation=js&search=prepareMessage&thread=0
prepareMessage:  400-700ms

There's a net gain on prepareMessage call, not only for repeatId but also the time spent creating the actual record.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1370268 - Stop using Immutable.Record for messages types.

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

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Ooops, profile without path should be this one https://perfht.ml/2rRlowE (Comment 3 show the one not uploaded)

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8876618 [details]
Bug 1370268 - Stop using Immutable.Record for messages types.

https://reviewboard.mozilla.org/r/147944/#review152458

Very simple change for such an improvement, thanks!  Feel free to land with a green try

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a5048f9789d
Stop using Immutable.Record for messages types. r=bgrins

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a5048f9789d