1329147 - Optimize rendering of the RequestListItem component

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Jarda Snajdr [:jsnajdr]]

Profiling the React/Redux implemenation of Netmonitor UI with Cleopatra tells me that a lot of time is spent when rendering the RequestListItem component.

Subcomponents like CauseColumn or MethodColumn will never change after the first render, yet they are rerendered on every update.

My patch will do the following optimizations:
1. Convert all the *Column components into classes (they are stateless functional components now) and implement a suitable shouldComponentUpdate on them. This will prevent redundant calls to render().

2. Optimize how the properties are compared in shouldComponentUpdate. Property access in Immutable.Record is rather expensive. So, I'm reordering the property names in RELEVANT_ITEM_PROPS (renamed to UPDATED_REQ_ITEM_PROPS) so that the ones most likely to be different are the first: mimeType, eventTimings, securityState, responseContentDataUri.

Comment 1

[Jarda Snajdr [:jsnajdr]]

Attachment: Bug 1329147 - Optimize rendering of RequestListItem

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

Comment 2

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

Comment on attachment 8824370 [details]
Bug 1329147 - Optimize rendering of RequestListItem

https://reviewboard.mozilla.org/r/102892/#review103670

Thanks for the patch Jarda, looks good to me.

Just a few minor inline comments.

R+ assuming try is green.

Also, it isn't related to this patch but, I have noticed that timing labels are not visible sometimes and reported bug 1329481

Honza

::: devtools/client/netmonitor/components/request-list-item.js:18
(Diff revision 1)
>  /**
> + * Compare two objects on a subset of their properties
> + */
> +function propertiesEqual(props, item1, item2) {
> +  return item1 === item2 || props.every(p => item1[p] === item2[p]);
> +}

I think that it would be better to put helper functions at the bottom of the file.

::: devtools/client/netmonitor/components/request-list-item.js:67
(Diff revision 1)
>      onContextMenu: PropTypes.func.isRequired,
>      onMouseDown: PropTypes.func.isRequired,
>      onSecurityIconClick: PropTypes.func.isRequired,
>    },
>  
>    componentDidMount() {

We are probably not respecting the rule much, but there should be space between function name and parenthases.

componentDidMount() -> componentDidMount ()

This should be done in separate bug as a bulk change though.

::: devtools/client/netmonitor/components/request-list-item.js:202
(Diff revision 1)
> -function FileColumn(item) {
> -  const { urlDetails, responseContentDataUri } = item;
> +const UPDATED_FILE_PROPS = [
> +  "urlDetails",
> +  "responseContentDataUri",
> +];
> +
> +const FileColumn = createFactory(createClass({

It would be great if every component has general comment explaining the purpose.

Comment 3

[Jarda Snajdr [:jsnajdr]]

(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> I think that it would be better to put helper functions at the bottom of the
> file.

I'll move them before landing.

> We are probably not respecting the rule much, but there should be space
> between function name and parenthases.
> 
> componentDidMount() -> componentDidMount ()

No, componentDidMount() is correct according to our eslint rules. Space is required only after the function keyword:

function () {
}

And that rule for "function" is there only because of compatibility with generator syntax. Here is an issue I filed for eslint back in May, but never got (and probably never will) get around to actually fix it:

https://github.com/eslint/eslint/issues/6195

> > +const FileColumn = createFactory(createClass({
> 
> It would be great if every component has general comment explaining the
> purpose.

In this case, the comment would say only "This is a component that renders the File column". Which is already perfectly obvious from the component name and code. I don't know what else should be explained there.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/980a5c4e312a
Optimize rendering of RequestListItem r=Honza

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/980a5c4e312a