1408182 - Immutable/Redux introduces slowness in netmonitor

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

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

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1408182 - remove immutable from requests store

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

Comment 2

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

Attachment: Bug 1408182 - Replace ImmutableJS by plain JS code;

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

Comment 3

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

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=1e4675599b783369dc7723e71d8db1f7f6872126&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800

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)

Try is green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e4675599b783369dc7723e71d8db1f7f6872126

Almost ready for review

Honza

Comment 4

[Alexandre Poirot [:ochameau]]

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

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

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

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

Comment 6

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

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

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

Comment 7

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

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a82aeb026295642ce4b4c72c83525aa05107d4a8

Ready for review.

Honza

Comment 8

[Alexandre Poirot [:ochameau]]

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"?

Comment 9

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

(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

Comment 10

[Alexandre Poirot [:ochameau]]

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

[Ricky Chien [:rickychien] (inactive)]

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 }

Comment 12

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

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

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

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

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

Comment 14

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

Try is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fee31eb11ce7030ebbc7833227ddaeb17a84ee3

Honza

Comment 15

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 16

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

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

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

Comment 17

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

(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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/edf2b70d2f01
Replace ImmutableJS by plain JS code; r=rickychien

Comment 19

[Pulsebot]

Backout by [e-mail]:
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

Comment 20

[Tiberius Oros[:tiberius_oros]]

Backed out changeset edf2b70d2f01 (bug 1408182)for failing in devtools/client/netmonitor/test/browser_net_copy_params.js
https://hg.mozilla.org/integration/autoland/rev/3da20c5290dfdef39b1a6c2f08e6f71d8bda450b

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=edf2b70d2f01157bdf1e6b21342a171ddf88d35a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success

https://treeherder.mozilla.org/logviewer.html#?job_id=146329615&repo=autoland&lineNumber=5463

Comment 21

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

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

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

Comment 22

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

Ah, sorry, test fixed, landing again.
Honza

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c7a4c69bb8c
Replace ImmutableJS by plain JS code; r=rickychien

Comment 24

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3c7a4c69bb8c