1425577 - Do not create a new state for every message in a MESSAGES_ADD batch

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

At the moment, we clone the state for each addMessage call, which is call by the MESSAGES_ADD action.
This is pointless since we have batching, and we should only clone the state once, and add all the message of the batch to it.

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Okay, so I have a patch which I'll put up for review after this comment.
It adds a bit of complexity, and there was no improvement on DAMP.
However, I do have a case where we can see nice benefits.

So here is the test case : 

```js
let i = 0;
function streamLogs() {
  console.log(i++);
  if (i < 500) {
    requestAnimationFrame(streamLogs)
  }
}
streamLogs();
```

in short, streaming logs (i.e. adding lots of messages in a short timespan).

Here's the profile without the patch : 
- https://perfht.ml/2B82mUg
- 4% (366ms of 7756ms) of the total time is spent on the messages reducer 

Here's the profile with the patch: 
- https://perfht.ml/2B3VUNL
- 1% (125ms of 6966ms) of the total time is spent on the reducer.


So here, we are almost 3 times faster on reducer time. 
The thing that we need to discuss is: is the added complexity to the code worth the gain ?

The overall gain is not stunning, but we do know that we don't spend most of the time in reducer. Still, this shaves off hundred ms on a high end laptop (so probably more on low end devices).

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425577 - Remove MESSAGE_ADD action;.

This action is not used anywhere since we batch messages and thus use MESSAGES_ADD action.
The tests that were using it now use messagesAdd.

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

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425577 - Do not create a new state for each message of a batch;.

In the MESSAGES_ADD action, we used to create a new state for each new
message in the batch. This is a bit of an overkill since we can clone
the state only once.

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

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8942892 [details]
Bug 1425577 - Remove MESSAGE_ADD action;.

https://reviewboard.mozilla.org/r/213164/#review218976

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8942893 [details]
Bug 1425577 - Do not create a new state for each message of a batch;.

https://reviewboard.mozilla.org/r/213166/#review218980

Is there any way you could split this up so that the parts that aren't changing the behavior are in a separate changeset (i.e. test changes that pass without the rest of the patch, addMessages function, etc)? I think that would make it easier to review, but if that's going to be a pain or the parts can't effectively be split up then let me know and I'll go through it as-is.

::: devtools/client/webconsole/new-console-output/reducers/messages.js:60
(Diff revision 1)
> -  // Map of the form {messageId : networkInformation}
> +    // Map of the form {messageId : networkInformation}
> -  // `networkInformation` holds request, response, totalTime, ...
> +    // `networkInformation` holds request, response, totalTime, ...
> -  networkMessagesUpdateById: {},
> +    networkMessagesUpdateById: {},
> -}, overrides));
> +  }, overrides);
>  
> -function addMessage(state, filtersState, prefsState, newMessage) {
> +  const wrap = freeze === true ? Object.freeze : (x) => x;

Nit: the `wrap` function makes it slightly more difficult for me to follow and adds an extra function call for the non freeze state. Something like `return (freeze === true) ? Object.freeze(state) : state;` (or the same but out of a ternary if you prefer) would help.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425577 - Fix and enhance console mocha store tests; .

Pass the wrapped actions object to setupStore so we do use the same
generator when adding messages later.
Add more group messages in the group test to make sure group are being
closed when a groupEnd message is passed in a messagesAdd batch.

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

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425577 - Do not clone the state for each message of a batch; .

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

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425577 - Change getMessageVisibility signature; .

The function now takes an object so it is more readable, and this
object only needs part of the message state instead of the whole state.

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

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425577 - Fix typo;.

currentGoup -> currentGroup.
The typo was in the function parameter so we did not had bug
because of that.

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

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425577 - Freeze returned object from limitTopLevelMessageCount; .

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

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1425577 - Move MESSAGES_ADD reducer block to a dedicated function;.

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

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8942892 [details]
Bug 1425577 - Remove MESSAGE_ADD action;.

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

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8942893 [details]
Bug 1425577 - Do not create a new state for each message of a batch;.

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

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8943234 [details]
Bug 1425577 - Fix typo;.

https://reviewboard.mozilla.org/r/213492/#review219356

::: commit-message-7024c:4
(Diff revision 1)
> +Bug 1425577 - Fix typo;r=bgrins.
> +
> +currentGoup -> currentGroup.
> +The typo was in the function parameter so we did not had bug

Nit: 'not have a bug because of it'

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8943231 [details]
Bug 1425577 - Fix and enhance console mocha store tests; .

https://reviewboard.mozilla.org/r/213486/#review219374

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943232 [details]
Bug 1425577 - Do not clone the state for each message of a batch; .

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

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943233 [details]
Bug 1425577 - Change getMessageVisibility signature; .

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

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943235 [details]
Bug 1425577 - Freeze returned object from limitTopLevelMessageCount; .

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

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8942893 [details]
Bug 1425577 - Do not create a new state for each message of a batch;.

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

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943236 [details]
Bug 1425577 - Move MESSAGES_ADD reducer block to a dedicated function;.

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

Comment 21

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943232 [details]
Bug 1425577 - Do not clone the state for each message of a batch; .

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

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943233 [details]
Bug 1425577 - Change getMessageVisibility signature; .

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

Comment 23

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943235 [details]
Bug 1425577 - Freeze returned object from limitTopLevelMessageCount; .

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

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8942893 [details]
Bug 1425577 - Do not create a new state for each message of a batch;.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213166/diff/3-4/

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943236 [details]
Bug 1425577 - Move MESSAGES_ADD reducer block to a dedicated function;.

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

Comment 26

[Alexandre Poirot [:ochameau] [PTO back August 21]]

FYI, DAMP reports some wins on panelsInBackground. In this test, we call console.log while the webconsole is in background. React updates are paused, but not redux actions. So it quite well highlights your win here and ensure we will not regress it.
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=9f498fc6a1eaa0b77c61a947edd5d88eee2d5bc2&newProject=try&newRevision=e7cf09086077c91d40fb6ae3948184bde879da6d&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyConfident=1&framework=1
  complicated.reload -2.8%
  panelsInBackground.reload -10%
  + various other smaller win with medium confidence.

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943232 [details]
Bug 1425577 - Do not clone the state for each message of a batch; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213488/diff/3-4/

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Brian, I changed things a bit in the last patch, and I think this looks much better.
What we do now is that we clone the state before adding the new messages, and then mutate the cloned state in the addMessage function.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8943232 [details]
Bug 1425577 - Do not clone the state for each message of a batch; .

https://reviewboard.mozilla.org/r/213488/#review220904

Nice!

::: devtools/client/webconsole/new-console-output/reducers/messages.js:135
(Diff revision 4)
> -  } = getMessageVisibility(addedMessage, newState, filtersState);
> +  } = getMessageVisibility(addedMessage, state, filtersState);
>  
>    if (visible) {
> -    newState.visibleMessages = [...visibleMessages, newMessage.id];
> +    state.visibleMessages.push(newMessage.id);
>    } else if (DEFAULT_FILTERS.includes(cause)) {
> -    newState.filteredMessagesCount = {
> +    state.filteredMessagesCount = {

Nit: I believe this could be done without cloning filteredMessagesCount again - i.e. you could do something like:

```
state.filteredMessagesCount.global++; state.filteredMessages[cause]++;
```

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8943232 [details]
Bug 1425577 - Do not clone the state for each message of a batch; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213488/diff/4-5/

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a6d7da2f9b1
Do not clone the state for each message of a batch; r=bgrins.

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/9a6d7da2f9b1

Comment 33

[Alexandre Poirot [:ochameau] [PTO back August 21]]

This patch improved by 28% the impact of the console when it is in background!
http://firefox-dev.tools/performance-dashboard/perfherder/?test=panelsInBackground.reload&days=7&filterstddev=true