Immutable/Redux introduces slowness in netmonitor

RESOLVED FIXED in Firefox 59

Status

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: ochameau, Assigned: Honza)

Tracking

unspecified
Firefox 59
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fix-optional, firefox59 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

If you look at a profile with all the current perf patches related to netmonitor (bug 1404917, bug 1407561, bug 1406312, bug 1406311):
  https://perfht.ml/2gA92mG
And then filter by "immutable.js" you get 893ms, so about 20% of overall computation!
You can't filter by "redux" as many frames related to redux also call react and many other code, so it won't only focus on redux cost. Whereas immutable is at the end of the call tree and doesn't call other code.
But you can filter by "reducers" and get 300ms or around 7%, but it also includes many immutable frames.

When seeing this, I decided to do an experiment and remove immutable in requests reducer. DAMP report a 9.78% win, so close to 10%!
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=571377f1594c4cd783108f1419f1ecd1026b68a7&newProject=try&newRevision=d293674a1fd096cda5933a62774925db81bbd77f&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1
And that's only for one reducers, whereas we have many others.
Last but not least, because of redux, I still have to copy many objects. If we also prevent that from happening, I'm expecting even more time savings!

But before doing anything, I would like someone to confirm that my experiment is correct and that I didn't broke the netmonitor:
  https://hg.mozilla.org/try/rev/ee55ef6c1783299d2227068d12315003542b6a6a
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> complicated.netmonitor.close.DAMP opt e10s   138.79 ± 5.00% 	>   127.42 ±
> 2.52% -8.19%,   5.33 (high) 
> complicated.netmonitor.requestsFinished.DAMP opt e10s   9,330.46 ± 6.53% >  
> 7,637.04 ± 0.41% -18.15%,   11.42 (high)

I'll say a word to everyone about this next tuesday,
"compare with last x days" gives confusing results when significant improvements landed recently.
Here, in these results, you also include response content patch, because
the base run your compare with don't have the response content patch applied yet.

The best, when you know a significant patch landed, is to compare against a custom try push,
including the patch that improved performances. I did one in bug 1404917.
It is try f71c357ac4fe5b9d30780b61f9200b78a073609e.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f71c357ac4fe5b9d30780b61f9200b78a073609e&newProject=try&newRevision=1e4675599b783369dc7723e71d8db1f7f6872126&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1
We get back to what I originaly reported in comment 0:
  complicated.netmonitor.requestsFinished  -7.93% 
  simple.netmonitor.reload  -7.55%
  simple.netmonitor.requestsFinished  -9.05%

Nonetheless, it is still very important improvements!!
Comment on attachment 8928868 [details]
Bug 1408182 - Replace ImmutableJS by plain JS code;

https://reviewboard.mozilla.org/r/200160/#review205406

::: devtools/client/netmonitor/src/reducers/requests.js:203
(Diff revision 3)
> +  // Only custom requests can be removed
> +  if (!removedRequest || !removedRequest.isCustom) {
> +    return state;
> +  }
> +
> +  let nextState = Object.assign({}, state);

In every action we recreate a plain new object.
Do you think we could change reducers to accept the same object again and still assume there is a change?
In term of memory this redux practice is quite bad.

::: devtools/client/netmonitor/src/reducers/requests.js:218
(Diff revision 3)
> + * Clone an existing map.
> + */
> +function mapNew(map) {
> +  let newMap = new Map(map);
> +  newMap.isEmpty = () => newMap.size == 0;
> +  newMap.valueSeq = () => [...newMap.values()];

That would be cool to get rid of these Immutable specifics as well wherever we use that.

::: devtools/client/netmonitor/src/reducers/requests.js:227
(Diff revision 3)
> +/**
> + * Append new item into existing map and return new map.
> + */
> +function mapSet(map, key, value) {
> +  let newMap = mapNew(map);
> +  return newMap.set(key, value);

Why do you create a new map every time?
Can't you do that?
  requests.set(newRequest.id, newRequest);
  nextState.requests = requests;
If that causes issues somewhere else, could you followup to fix that "somewhere else"?
(In reply to Alexandre Poirot [:ochameau] from comment #8)
> Comment on attachment 8928868 [details]
> Bug 1408182 - Replace ImmutableJS by plain JS code;
> 
> https://reviewboard.mozilla.org/r/200160/#review205406
> 
> ::: devtools/client/netmonitor/src/reducers/requests.js:203
> (Diff revision 3)
> > +  // Only custom requests can be removed
> > +  if (!removedRequest || !removedRequest.isCustom) {
> > +    return state;
> > +  }
> > +
> > +  let nextState = Object.assign({}, state);
> 
> In every action we recreate a plain new object.
> Do you think we could change reducers to accept the same object again and
> still assume there is a change?
Changing the state object (i.e. not mutating the state) is one of the main premises of React-Redux based app, and tricking the logic by mutating data in the store can produce weird (hard to find) errors in the future (e.g. some components in the UI are not updated sometimes since they think the state didin't change).

But, I think we could perhaps implement (or search for an existing) redux middle-ware that optimizes this in case of handling more actions at once. In such case we can create just one state, handle all actions in a queue and return in (instead of creating new state for every action and return the last one).

Since we are batching actions, this could be nice perf opt.
Please file a bug and I'll investigate this.

> In term of memory this redux practice is quite bad.
> 
> ::: devtools/client/netmonitor/src/reducers/requests.js:218
> (Diff revision 3)
> > + * Clone an existing map.
> > + */
> > +function mapNew(map) {
> > +  let newMap = new Map(map);
> > +  newMap.isEmpty = () => newMap.size == 0;
> > +  newMap.valueSeq = () => [...newMap.values()];
> 
> That would be cool to get rid of these Immutable specifics as well wherever
> we use that.
I did remove most of the helpers, but these two were actually useful and made the map usage a bit better, so I kept it there. If you have strong opinion I can remove it too (please file a follow up, so we can land this).

> ::: devtools/client/netmonitor/src/reducers/requests.js:227
> (Diff revision 3)
> > +/**
> > + * Append new item into existing map and return new map.
> > + */
> > +function mapSet(map, key, value) {
> > +  let newMap = mapNew(map);
> > +  return newMap.set(key, value);
> 
> Why do you create a new map every time?
> Can't you do that?
>   requests.set(newRequest.id, newRequest);
>   nextState.requests = requests;
> If that causes issues somewhere else, could you followup to fix that
> "somewhere else"?
This is related to what I said above (mutating state is bad thing) and I think we should rather avoid this kind of hacks (it can end up by mutating various pieces of the store in various reducers, again issues hard to find...)

But, I understand, in case of significant perf improvement...

I was actually experimenting with this myself and created to Talos builds.

cloning the map:
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=1e4675599b783369dc7723e71d8db1f7f6872126

not cloning the map:
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=c79b33cfc7e8ed855bdb9269d664fb694ddb9940

I am not seeing significant perf improvements. Or do you see otherwise?

Thanks for the review Alex!
Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #9)
> Changing the state object (i.e. not mutating the state) is one of the main
> premises of React-Redux based app, and tricking the logic by mutating data
> in the store can produce weird (hard to find) errors in the future (e.g.
> some components in the UI are not updated sometimes since they think the
> state didin't change).

Julien told me this pattern where you always return new objects isn't mandatory in redux.
And that even if it not common to do so, it isn't an issue to do so.
But it requires to modify the reducers, I'll try to sync up with him.


> > That would be cool to get rid of these Immutable specifics as well wherever
> > we use that.
> I did remove most of the helpers, but these two were actually useful and
> made the map usage a bit better, so I kept it there. If you have strong
> opinion I can remove it too (please file a follow up, so we can land this).

This is weird having Immutable API on a Map.
Yes, I feel I should insist about that, and yes we can followup.

> I am not seeing significant perf improvements. Or do you see otherwise?

That's interesting. Yes DAMP reports no difference between the two.
It looks like cloning a Map is very cheap!
Comment on attachment 8928868 [details]
Bug 1408182 - Replace ImmutableJS by plain JS code;

https://reviewboard.mozilla.org/r/200160/#review206310

Thanks for working on this Honza! There are some issues, please see below.

I'd prefer to see if we can remove immutable helpers and even get rid of all immutable APIs where are using in other reducers. (I'm okay if you'd like to do it in follow-up. After removing entire immutable library, I expect that at least we can reduce the module load time of immutable.

::: devtools/client/netmonitor/src/reducers/requests.js:28
(Diff revision 3)
> -const Request = I.Record({
> -  id: null,
> -  // Set to true in case of a request that's being edited as part of "edit and resend"
> -  isCustom: false,
> -  // Request properties - at the beginning, they are unknown and are gradually filled in
> -  startedMillis: undefined,
> +/**
> + * This structure stores list of all HTTP requests received
> + * from the backend. It's using plain JS structures to store
> + * data instead of ImmutableJS, which is performance expensive.
> + */
> +function Requests(wasRecording = true) {

I'd suggest to remove `wasRecording = true` and keep the interface clean.

If people would like to customize their own Requests instance, they can modify it by merging object:

```
{ ...Requests(), { wasRecording: true } }

```

::: devtools/client/netmonitor/src/reducers/requests.js:42
(Diff revision 3)
> -
> -  let removedRequest = requests.get(selectedId);
>  
> -  // Only custom requests can be removed
> -  if (!removedRequest || !removedRequest.isCustom) {
> -    return state;
> +  return state;

nit: we can simply return this object instead of creating an extra one-time used state variable.

::: devtools/client/netmonitor/src/reducers/requests.js:53
(Diff revision 3)
> -
> -function requestsReducer(state = new Requests(), action) {
>    switch (action.type) {
> +    // Appending new request into the list/map.
>      case ADD_REQUEST: {
> -      return state.withMutations(st => {
> +      let nextState = Object.assign({}, state);

Can we try to use object spread syntax [1] and remove Object.assign() if possible?

`let nextState = Object.assign({}, state);`

to

`let nextState = { ...state };`

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_operator#Spread_in_object_literals


Enable ObjectRestSpread config in eslint by adding this to netmonitor/.eslintrc

```
  "parserOptions": {
    "ecmaFeatures": {
      experimentalObjectRestSpread: true,
    },
  },
```

::: devtools/client/netmonitor/src/reducers/requests.js:210
(Diff revision 3)
> +// Immutability helpers
> +
> +/**
> + * Clone an existing map.
> + */
> +function mapNew(map) {
> +  let newMap = new Map(map);
> +  newMap.isEmpty = () => newMap.size == 0;
> +  newMap.valueSeq = () => [...newMap.values()];
> +  return newMap;
> +}
> +
> +/**
> + * Append new item into existing map and return new map.
> + */
> +function mapSet(map, key, value) {
> +  let newMap = mapNew(map);
> +  return newMap.set(key, value);
> +}
> +
> +/**
> + * Remove an item from existing map and return new map.
> + */
> +function mapDelete(map, key) {
> +  let newMap = mapNew(map);
> +  newMap.requests.delete(key);
> +  return newMap;
> +}

Let's get rid of all of those immutable helpers rather than adding workaround APIs.

::: devtools/client/netmonitor/src/reducers/requests.js:239
(Diff revision 3)
> +// Exports
> +

nit: remove this comment since it seems not that useful.

::: devtools/client/netmonitor/src/selectors/requests.js:61
(Diff revision 3)
>  );
>  
>  const getSortFn = createSelector(
> -  state => state.requests.requests,
> +  state => state.requests,
>    state => state.sort,
> -  (requests, sort) => {
> +  ({requests}, sort) => {

nit: Please suround whitespaces in-between. { requests }

::: devtools/client/netmonitor/src/selectors/requests.js:71
(Diff revision 3)
>  );
>  
>  const getSortedRequests = createSelector(
> -  state => state.requests.requests,
> +  state => state.requests,
>    getSortFn,
> -  (requests, sortFn) => requests.valueSeq().sort(sortFn).toList()
> +  ({requests}, sortFn) => {

nit: Please suround whitespaces in-between. { requests }

::: devtools/client/netmonitor/src/selectors/requests.js:84
(Diff revision 3)
>  
>  const getDisplayedRequests = createSelector(
> -  state => state.requests.requests,
> +  state => state.requests,
>    getFilterFn,
>    getSortFn,
> -  (requests, filterFn, sortFn) => requests.valueSeq()
> +  ({requests}, filterFn, sortFn) => {

nit: Please suround whitespaces in-between. { requests }

::: devtools/client/netmonitor/src/selectors/requests.js:96
(Diff revision 3)
>  );
>  
>  const getTypeFilteredRequests = createSelector(
> -  state => state.requests.requests,
> +  state => state.requests,
>    getTypeFilterFn,
> -  (requests, filterFn) => requests.valueSeq().filter(filterFn).toList()
> +  ({requests}, filterFn) => requests.valueSeq().filter(filterFn)

nit: Please suround whitespaces in-between. { requests }
Attachment #8928868 - Flags: review?(rchien) → review-
(In reply to Ricky Chien [:rickychien] from comment #11)
> Comment on attachment 8928868 [details]
> Bug 1408182 - Replace ImmutableJS by plain JS code;
> 
> https://reviewboard.mozilla.org/r/200160/#review206310
> 
> Thanks for working on this Honza! There are some issues, please see below.
> 
> I'd prefer to see if we can remove immutable helpers and even get rid of all
> immutable APIs where are using in other reducers. (I'm okay if you'd like to
> do it in follow-up. After removing entire immutable library, I expect that
> at least we can reduce the module load time of immutable.
I'll file a meta for removing ImmutableJS from other reducers.

I created meta for removing ImmutableJS entirely this 1418970

> ::: devtools/client/netmonitor/src/reducers/requests.js:28
> (Diff revision 3)
> > -const Request = I.Record({
> > -  id: null,
> > -  // Set to true in case of a request that's being edited as part of "edit and resend"
> > -  isCustom: false,
> > -  // Request properties - at the beginning, they are unknown and are gradually filled in
> > -  startedMillis: undefined,
> > +/**
> > + * This structure stores list of all HTTP requests received
> > + * from the backend. It's using plain JS structures to store
> > + * data instead of ImmutableJS, which is performance expensive.
> > + */
> > +function Requests(wasRecording = true) {
> 
> I'd suggest to remove `wasRecording = true` and keep the interface clean.
Done

> 
> If people would like to customize their own Requests instance, they can
> modify it by merging object:
> 
> ```
> { ...Requests(), { wasRecording: true } }
Done

> 
> ```
> 
> ::: devtools/client/netmonitor/src/reducers/requests.js:42
> (Diff revision 3)
> > -
> > -  let removedRequest = requests.get(selectedId);
> >  
> > -  // Only custom requests can be removed
> > -  if (!removedRequest || !removedRequest.isCustom) {
> > -    return state;
> > +  return state;
> 
> nit: we can simply return this object instead of creating an extra one-time
> used state variable.
Done

> 
> ::: devtools/client/netmonitor/src/reducers/requests.js:53
> (Diff revision 3)
> > -
> > -function requestsReducer(state = new Requests(), action) {
> >    switch (action.type) {
> > +    // Appending new request into the list/map.
> >      case ADD_REQUEST: {
> > -      return state.withMutations(st => {
> > +      let nextState = Object.assign({}, state);
> 
> Can we try to use object spread syntax [1] and remove Object.assign() if
> possible?
Done (at all places in the file)
> 
> `let nextState = Object.assign({}, state);`
> 
> to
> 
> `let nextState = { ...state };`
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/
> Spread_operator#Spread_in_object_literals
> 
> 
> Enable ObjectRestSpread config in eslint by adding this to
> netmonitor/.eslintrc
> 
> ```
>   "parserOptions": {
>     "ecmaFeatures": {
>       experimentalObjectRestSpread: true,
>     },
>   },
> ```
Done


> 
> ::: devtools/client/netmonitor/src/reducers/requests.js:210
> (Diff revision 3)
> > +// Immutability helpers
> > +
> > +/**
> > + * Clone an existing map.
> > + */
> > +function mapNew(map) {
> > +  let newMap = new Map(map);
> > +  newMap.isEmpty = () => newMap.size == 0;
> > +  newMap.valueSeq = () => [...newMap.values()];
> > +  return newMap;
> > +}
> > +
> > +/**
> > + * Append new item into existing map and return new map.
> > + */
> > +function mapSet(map, key, value) {
> > +  let newMap = mapNew(map);
> > +  return newMap.set(key, value);
> > +}
> > +
> > +/**
> > + * Remove an item from existing map and return new map.
> > + */
> > +function mapDelete(map, key) {
> > +  let newMap = mapNew(map);
> > +  newMap.requests.delete(key);
> > +  return newMap;
> > +}
> 
> Let's get rid of all of those immutable helpers rather than adding
> workaround APIs.
Reported bug 1418969

> 
> ::: devtools/client/netmonitor/src/reducers/requests.js:239
> (Diff revision 3)
> > +// Exports
> > +
> 
> nit: remove this comment since it seems not that useful.
Done

> 
> ::: devtools/client/netmonitor/src/selectors/requests.js:61
> (Diff revision 3)
> >  );
> >  
> >  const getSortFn = createSelector(
> > -  state => state.requests.requests,
> > +  state => state.requests,
> >    state => state.sort,
> > -  (requests, sort) => {
> > +  ({requests}, sort) => {
> 
> nit: Please suround whitespaces in-between. { requests }
Done

> 
> ::: devtools/client/netmonitor/src/selectors/requests.js:71
> (Diff revision 3)
> >  );
> >  
> >  const getSortedRequests = createSelector(
> > -  state => state.requests.requests,
> > +  state => state.requests,
> >    getSortFn,
> > -  (requests, sortFn) => requests.valueSeq().sort(sortFn).toList()
> > +  ({requests}, sortFn) => {
> 
> nit: Please suround whitespaces in-between. { requests }
Done

> 
> ::: devtools/client/netmonitor/src/selectors/requests.js:84
> (Diff revision 3)
> >  
> >  const getDisplayedRequests = createSelector(
> > -  state => state.requests.requests,
> > +  state => state.requests,
> >    getFilterFn,
> >    getSortFn,
> > -  (requests, filterFn, sortFn) => requests.valueSeq()
> > +  ({requests}, filterFn, sortFn) => {
> 
> nit: Please suround whitespaces in-between. { requests }
Done

> 
> ::: devtools/client/netmonitor/src/selectors/requests.js:96
> (Diff revision 3)
> >  );
> >  
> >  const getTypeFilteredRequests = createSelector(
> > -  state => state.requests.requests,
> > +  state => state.requests,
> >    getTypeFilterFn,
> > -  (requests, filterFn) => requests.valueSeq().filter(filterFn).toList()
> > +  ({requests}, filterFn) => requests.valueSeq().filter(filterFn)
> 
> nit: Please suround whitespaces in-between. { requests }
Done

Thanks Ricky!

Honza
Comment on attachment 8928868 [details]
Bug 1408182 - Replace ImmutableJS by plain JS code;

https://reviewboard.mozilla.org/r/200160/#review206620

::: devtools/client/netmonitor/src/reducers/requests.js:90
(Diff revision 4)
> +        return state;
> -        }
> +      }
> -      });
> +
> +      request = {
> +        ...request,
> +        ...processNetworkUpdates(action.data)

nit: missing tailing comma.

::: devtools/client/netmonitor/src/reducers/requests.js:215
(Diff revision 4)
> +    requests: mapDelete(state.requests, selectedId),
> +    selectedId: null,
> +  };
> +}
> +
> +// Immutability helpers

nit: adding FIXME: bug 1418969.
Attachment #8928868 - Flags: review?(rchien) → review+
(In reply to Ricky Chien [:rickychien] from comment #15)
> Comment on attachment 8928868 [details]
> Bug 1408182 - Replace ImmutableJS by plain JS code;
> 
> https://reviewboard.mozilla.org/r/200160/#review206620
> 
> ::: devtools/client/netmonitor/src/reducers/requests.js:90
> (Diff revision 4)
> > +        return state;
> > -        }
> > +      }
> > -      });
> > +
> > +      request = {
> > +        ...request,
> > +        ...processNetworkUpdates(action.data)
> 
> nit: missing tailing comma.
Fixed

> 
> ::: devtools/client/netmonitor/src/reducers/requests.js:215
> (Diff revision 4)
> > +    requests: mapDelete(state.requests, selectedId),
> > +    selectedId: null,
> > +  };
> > +}
> > +
> > +// Immutability helpers
> 
> nit: adding FIXME: bug 1418969.
Done

Honza
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/edf2b70d2f01
Replace ImmutableJS by plain JS code; r=rickychien
Backout by toros@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3da20c5290df
Backed out changeset edf2b70d2f01 for failing in  devtools/client/netmonitor/test/browser_net_copy_params.js r=backout on a CLOSED TREE
Ah, sorry, test fixed, landing again.
Honza
Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3c7a4c69bb8c
Replace ImmutableJS by plain JS code; r=rickychien
https://hg.mozilla.org/mozilla-central/rev/3c7a4c69bb8c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1420711
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.