Closed Bug 1419368 Opened 6 years ago Closed 6 years ago

NetMonitor: Stop using ImmutableJS in timing-markers reducer

Categories

(DevTools :: Netmonitor, enhancement, P2)

enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: Honza, Assigned: abhinav.koppula, Mentored)

References

Details

(Whiteboard: good-first-bug)

Attachments

(1 file)

ImmutableJS is slow and we should stop using it in NetMonitor's timing-markers reducer.

devtools/client/netmonitor/src/reducers/timing-markers.js 

It should be replaced by plain JS structures.

See bug 1408182 as an example.

Honza
Mentor: odvarko
Priority: -- → P2
Whiteboard: good-first-bug
Comment on attachment 8932961 [details]
Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer.

https://reviewboard.mozilla.org/r/203964/#review209436

::: devtools/client/netmonitor/src/reducers/timing-markers.js
(Diff revision 1)
>  }
>  
>  function clearTimingMarkers(state) {
> -  return state.withMutations(st => {
> +  return new TimingMarkers();
> -    st.remove("firstDocumentDOMContentLoadedTimestamp");
> -    st.remove("firstDocumentLoadTimestamp");

Shouldn't we delete these two props just like before?
Comment on attachment 8932961 [details]
Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer.

https://reviewboard.mozilla.org/r/203964/#review209436

> Shouldn't we delete these two props just like before?

Hi Honza,
So I tried this:
```
state = { ...state };
delete state.firstDocumentDOMContentLoadedTimestamp;
delete state.firstDocumentLoadTimestamp;
return state;
```
but adding the above snippet was resulting in breaking of the functionality of TimingMarkers and also 2 tests were failing.
However on just having `return new TimingMarkers();` fixes both of the above problems.

I am not sure how the above snippet should be modified to make the functionality work. Can you help me here?
Comment on attachment 8932961 [details]
Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer.

https://reviewboard.mozilla.org/r/203964/#review209756

(In reply to Abhinav Koppula from comment #3)
> I am not sure how the above snippet should be modified to make the
> functionality work. Can you help me here?

Does fixing my two inline comments help with this issue?

Honza

::: devtools/client/netmonitor/src/reducers/timing-markers.js:23
(Diff revision 1)
> +}
>  
>  function addTimingMarker(state, action) {
>    if (action.marker.name === "document::DOMContentLoaded" &&
>        state.firstDocumentDOMContentLoadedTimestamp === -1) {
> -    return state.set("firstDocumentDOMContentLoadedTimestamp",
> +    state.firstDocumentDOMContentLoadedTimestamp = action.marker.unixTime / 1000;

You are not cloning the state here

::: devtools/client/netmonitor/src/reducers/timing-markers.js:28
(Diff revision 1)
> -                     action.marker.unixTime / 1000);
>    }
>  
>    if (action.marker.name === "document::Load" &&
>        state.firstDocumentLoadTimestamp === -1) {
> -    return state.set("firstDocumentLoadTimestamp",
> +    state.firstDocumentLoadTimestamp = action.marker.unixTime / 1000;

The same as above, you need to clone the state object here.

Please prefer spread operator before Object.assign()
Attachment #8932961 - Flags: review?(odvarko) → review-
Comment on attachment 8932961 [details]
Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer.

https://reviewboard.mozilla.org/r/203964/#review209756

Hi Honza,
So after addressing the inline comments also, the functionality doesn't work if we use `delete` to remove the two keys(firstDocumentDOMContentLoadedTimestamp and firstDocumentLoadTimestamp).
So I reverted my patch and found out that this is how the functionality works with ImmutableJS. I just logged the values before and after the mutation

initialValue - firstDocumentDOMContentLoadedTimestamp = 1512137925747.967  timing-markers.js:36:3
initialValue - firstDocumentLoadTimestamp = 1512137930776.788  timing-markers.js:37:3
finalValue - firstDocumentDOMContentLoadedTimestamp = -1  timing-markers.js:42:3
finalValue - firstDocumentLoadTimestamp = -1  timing-markers.js:43:3```

I think this might explain why using `new TimingMarkers()` restores the original functionality.
Comment on attachment 8932961 [details]
Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer.

https://reviewboard.mozilla.org/r/203964/#review210188

Thanks, looks good, just one inline comment

Honza

::: devtools/client/netmonitor/src/reducers/timing-markers.js:23
(Diff revision 2)
> +}
>  
>  function addTimingMarker(state, action) {
>    if (action.marker.name === "document::DOMContentLoaded" &&
>        state.firstDocumentDOMContentLoadedTimestamp === -1) {
> -    return state.set("firstDocumentDOMContentLoadedTimestamp",
> +    state = { ...state };

Please, clone the state just once at the beginning of the method (not within the conditions).

::: devtools/client/netmonitor/src/reducers/timing-markers.js:30
(Diff revision 2)
> +    return state;
>    }
>  
>    if (action.marker.name === "document::Load" &&
>        state.firstDocumentLoadTimestamp === -1) {
> -    return state.set("firstDocumentLoadTimestamp",
> +    state = { ...state };

So, this should be removed.
Attachment #8932961 - Flags: review?(odvarko) → review-
Comment on attachment 8932961 [details]
Bug 1419368 - NetMonitor: Stop using ImmutableJS in timing-markers reducer.

https://reviewboard.mozilla.org/r/203964/#review210204

Looks good!

R+ assuming try is green

Thanks Abhinav!
Honza
Attachment #8932961 - Flags: review?(odvarko) → review+
Hi Honza,
There are 2 failures but looks like they are intermittent and not related to this.
Can you confirm once from your side?
Flags: needinfo?(odvarko)
Yes, seems unrelated, thanks!

Honza
Flags: needinfo?(odvarko)
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed1e9dcdee4a
NetMonitor: Stop using ImmutableJS in timing-markers reducer. r=Honza
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/ed1e9dcdee4a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: