Optimize rendering of the RequestListItem component

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Developer Tools: Netmonitor
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jsnajdr, Assigned: jsnajdr)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [netmonitor])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Assignee: nobody → jsnajdr
Blocks: 1307743, 1328553
Whiteboard: [netmonitor-triage]
Comment hidden (mozreview-request)
Status: NEW → ASSIGNED
Iteration: --- → 53.4 - Jan 9
Flags: qe-verify?
Priority: -- → P1
Whiteboard: [netmonitor-triage] → [netmonitor]
(Assignee)

Updated

a year ago
No longer blocks: 1328553
(Assignee)

Updated

a year ago
Blocks: 1321749

Comment 2

a year ago
mozreview-review
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.
Attachment #8824370 - Flags: review?(odvarko) → review+
(Assignee)

Comment 3

a year ago
(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

a year ago
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/980a5c4e312a
Optimize rendering of RequestListItem r=Honza
Flags: qe-verify? → qe-verify-

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/980a5c4e312a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.