1422841 - Remove Immutable in messages reducer

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1422841 - Remove Immutable usage from the messages reducer;

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

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1422841 - Adapt tests to the new state; .

This introduces a few helpers to work with the state now
that we don't use immutable anymore.

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

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

damp push https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=785572419acc&newProject=try&newRevision=f8b33dfd73cc7fc8cab40b4b4b30a9a40ab21aa0&framework=1

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> damp push
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&originalRevision=785572419acc&newProject=try&newRevision=f8b33dfd73cc
> 7fc8cab40b4b4b30a9a40ab21aa0&framework=1

FYI I find it's easier to do a separate base push on try by removing your local changes and then using the same syntax you used for the 'new' revision. The issue this compare runs into is that the base (m-c) only has one run - you can fix this by clicking through the treeherder UI to retrigger relevant tests 5 more times, but I think it's easier to just do a second try push.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

yeah, i just did that, it should be easier https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0fdc65286e085584e0bb2910ff73d3d21a50780e&newProject=try&newRevision=f8b33dfd73cc7fc8cab40b4b4b30a9a40ab21aa0&framework=1

Comment 6

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

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #3)
> > damp push
> > https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> > central&originalRevision=785572419acc&newProject=try&newRevision=f8b33dfd73cc
> > 7fc8cab40b4b4b30a9a40ab21aa0&framework=1
> 
> FYI I find it's easier to do a separate base push on try by removing your
> local changes and then using the same syntax you used for the 'new'
> revision. The issue this compare runs into is that the base (m-c) only has
> one run - you can fix this by clicking through the treeherder UI to
> retrigger relevant tests 5 more times, but I think it's easier to just do a
> second try push.

Pushing a custom base run is less important now that PerfHerder supports comparing against branch and last X days.
It is only useful when some patch recently landed and changed the performances you are looking at.

With this workflow you get this result:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=f8b33dfd73cc7fc8cab40b4b4b30a9a40ab21aa0&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTimeRange=172800
  -3.49% on complicated.open
  -4.8% on bulklog
  -4% on openwithcache

It is unclear how you got this link, but if you open PerfHerder link from the mail try sends you:
  https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=f8b33dfd73cc7fc8cab40b4b4b30a9a40ab21aa0
you only need to click on compare and get this view, with plenty of base runs.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #6)
> With this workflow you get this result:
> https://treeherder.mozilla.org/perf.html#/
> comparesubtest?originalProject=mozilla-
> central&newProject=try&newRevision=f8b33dfd73cc7fc8cab40b4b4b30a9a40ab21aa0&o
> riginalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec6
> 6500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1&selectedTi
> meRange=172800
>   -3.49% on complicated.open
>   -4.8% on bulklog
>   -4% on openwithcache

Looks like some really nice DAMP wins from this patch, and on the exact tests I'd expect 
 
> It is unclear how you got this link, but if you open PerfHerder link from
> the mail try sends you:
>  
> https://treeherder.mozilla.org/perf.html#/
> comparechooser?newProject=try&newRevision=f8b33dfd73cc7fc8cab40b4b4b30a9a40ab
> 21aa0
> you only need to click on compare and get this view, with plenty of base
> runs.

Thanks for the tip, that'll save some time

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934184 [details]
Bug 1422841 - Remove Immutable usage from the messages reducer;

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

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934185 [details]
Bug 1422841 - Adapt tests to the new state; .

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

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Numbers are a bit less good when comparing to the parent changeset https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=0fdc65286e085584e0bb2910ff73d3d21a50780e&newProject=try&newRevision=f8b33dfd73cc7fc8cab40b4b4b30a9a40ab21aa0&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1 

Maybe in Alex's one other improvements are showing up.

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8934184 [details]
Bug 1422841 - Remove Immutable usage from the messages reducer;

https://reviewboard.mozilla.org/r/205126/#review210736

This generally looks good to me, with the caveat that I think it'll now be easier now to accidentally change the old state properties (i.e. the object spread is just doing a shallow clone and we still need to remember to clone properties on the new state we need to remember to do: `newState.messagesUiById = [...messagesUiById, newMessage.id];`). That said, I think there are actually quite a few ergonomic improvements here, and the perf improvement alone makes this worth doing even if it was a net neutral change.  Clearing review so I can have another chance to look at it before landing.

::: devtools/client/webconsole/new-console-output/components/ConsoleOutput.js:98
(Diff revision 2)
>      // - the number of messages displayed changed
>      //   and we are already scrolled to the bottom
>      // - the number of messages in the store changed
>      //   and the new message is an evaluation result.
> +    const isNewMessageEvaluationResult = messagesDelta > 0 &&
> +      nextProps.messages.get([...nextProps.messages.keys()][nextProps.messages.size - 1])

Nit: `[...nextProps.messages.values()][nextProps.messages.size - 1]` would do the same. Haven't checked to see if there's a perf impact with it, as the message values will be larger than the keys, but it'd be easier to read.

::: devtools/client/webconsole/new-console-output/reducers/messages.js:84
(Diff revision 2)
> +      currentGroup: getNewCurrentGroup(currentGroup, groupsById)
> +    };
>    }
>  
>    if (newMessage.allowRepeating && messagesById.size > 0) {
> -    let lastMessage = messagesById.last();
> +    const keys = [...messagesById.keys()];

Ditto about using `values` instead of keys here, unless if that's a performance problem in which case you can leave it as-is

::: devtools/client/webconsole/new-console-output/reducers/messages.js:170
(Diff revision 2)
>          let message = action.messages[i];
> -        if (!message.groupId && !isGroupType(message.type) &&
> -            message.type !== MESSAGE_TYPE.END_GROUP) {
> +        if (
> +          !message.groupId && !isGroupType(message.type) &&
> +          message.type !== MESSAGE_TYPE.END_GROUP
> +        ) {
> +          if (message.repeatId !== lastMessageRepeatId) {

Why do we need the new condition here?

::: devtools/client/webconsole/new-console-output/reducers/messages.js:200
(Diff revision 2)
>  
>      case constants.MESSAGES_CLEAR:
> -      return new MessageState({
> +      return MessageState({
>          // Store all actors from removed messages. This array is used by
>          // `releaseActorsEnhancer` to release all of those backend actors.
> -        "removedActors": [...state.messagesById].reduce((res, [id, msg]) => {
> +        removedActors: [...state.messagesById].reduce((res, [id, msg]) => {

In this case using `[...state.messagesById.values())` seems strictly better, since we never use the key in the reduce function

::: devtools/client/webconsole/new-console-output/reducers/messages.js:257
(Diff revision 2)
>  
>      case constants.MESSAGE_CLOSE:
> -      return state.withMutations(function (record) {
> +      const closeState = {...state};
> -        let messageId = action.id;
> +      let messageId = action.id;
> -        let index = record.messagesUiById.indexOf(messageId);
> -        record.deleteIn(["messagesUiById", index]);
> +      let index = closeState.messagesUiById.indexOf(messageId);
> +      closeState.messagesUiById = Array.from(closeState.messagesUiById).splice(index, 1);

Why Array.from in this case instead of `...`? We should be consistent unless if there's a reason to differ

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8934185 [details]
Bug 1422841 - Adapt tests to the new state; .

https://reviewboard.mozilla.org/r/205128/#review210748

::: devtools/client/webconsole/new-console-output/test/helpers.js:42
(Diff revision 2)
>  }
>  
>  /**
>   * Prepare the store for use in testing.
>   */
> -function setupStore(input = [], hud, options) {
> +function setupStore(input = [], hud, options, wrappedActions) {

I don't see the caller to setupStore that passes wrappedACtions in as a parameter.  In what cases does this get called with and without the parameter?

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934184 [details]
Bug 1422841 - Remove Immutable usage from the messages reducer;

https://reviewboard.mozilla.org/r/205126/#review211396

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934184 [details]
Bug 1422841 - Remove Immutable usage from the messages reducer;

https://reviewboard.mozilla.org/r/205126/#review210736

> Why do we need the new condition here?

because there was a bug :) we were counting repeated messages in the prune count, which we don't want

it was revealed by tests using messagesAdd: in a test we were adding the following messages [1,1,2,2] with a logLimit of 2, in order to see the repeat values. and previously the store was pruning the first two "1" because of the loglimit, which was false.

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934185 [details]
Bug 1422841 - Adapt tests to the new state; .

https://reviewboard.mozilla.org/r/205128/#review210748

> I don't see the caller to setupStore that passes wrappedACtions in as a parameter.  In what cases does this get called with and without the parameter?

it is called with actions in "cleans the repeatsById object when messages are pruned" test. Thisallow to use the same id generator between setupStore and later lone dispatch of the action

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934184 [details]
Bug 1422841 - Remove Immutable usage from the messages reducer;

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

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8934185 [details]
Bug 1422841 - Adapt tests to the new state; .

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

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8934184 [details]
Bug 1422841 - Remove Immutable usage from the messages reducer;

https://reviewboard.mozilla.org/r/205126/#review211584

Thanks, looks good to me

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dfdac1f98c9
Remove Immutable usage from the messages reducer; r=bgrins
https://hg.mozilla.org/integration/autoland/rev/a90670bce931
Adapt tests to the new state; r=bgrins.

Comment 20

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/2dfdac1f98c9
https://hg.mozilla.org/mozilla-central/rev/a90670bce931