1434848 - StatusBar tries to updates on every state update and its render is slow

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

HAR export is currently very slow.
Here is a profile of it:
  https://perfht.ml/2npGY8N

It looks like everytime we fetch a lazy attribute (response content, cookies, headers, ...) the state is updated and a lot of react components try to update.
Some of which have quite slow render method.

StatusBar is a pure function. It would be helpful to be a Component and have a shouldComponentUpdate method.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1434848 - Prevent StatusBar updates by making it be a Component.

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

Comment 2

[Alexandre Poirot [:ochameau]]

Comment on attachment 8947450 [details]
Bug 1434848 - Prevent StatusBar updates by making it be a Component.

I did not meant to r? yet.
This patch doesn't report any win on DAMP,
I would like to first see its impact on a DAMP test involving HAR export.

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8947450 [details]
Bug 1434848 - Prevent StatusBar updates by making it be a Component.

https://reviewboard.mozilla.org/r/217150/#review222954


Static analysis found 4 defects in this patch.
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/src/components/StatusBar.js:48
(Diff revision 1)
> +const UPDATED_TIMINGS_PROPS = [
> +  "DOMContentLoaded",
> +  "load",
> +];
> +
> +class StatusBar extends Component {

Error: Block must not be padded by blank lines. [eslint: padded-blocks]

::: devtools/client/netmonitor/src/components/StatusBar.js:60
(Diff revision 1)
> +      timingMarkers: PropTypes.object.isRequired,
> +    };
> +  }
> +
> +  shouldComponentUpdate(nextProps) {
> +    return !propertiesEqual(UPDATED_SUMMARY_PROPS, this.props.summary, nextProps.summary) ||

Error: Line 60 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/netmonitor/src/components/StatusBar.js:61
(Diff revision 1)
> +    };
> +  }
> +
> +  shouldComponentUpdate(nextProps) {
> +    return !propertiesEqual(UPDATED_SUMMARY_PROPS, this.props.summary, nextProps.summary) ||
> +      !propertiesEqual(UPDATED_TIMINGS_PROPS, this.props.timingMarkers, nextProps.timingMarkers);

Error: Line 61 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/client/netmonitor/src/components/StatusBar.js:86
(Diff revision 1)
> -    div({ className: "devtools-toolbar devtools-toolbar-bottom" },
> +      div({ className: "devtools-toolbar devtools-toolbar-bottom" },
> -      button({
> +        button({
> -        className: "devtools-button requests-list-network-summary-button",
> +          className: "devtools-button requests-list-network-summary-button",
> -        title: TOOLTIP_PERF,
> +          title: TOOLTIP_PERF,
> -        onClick: openStatistics,
> +          onClick: () => {
> +            openStatistics(connector, true);

Error: 'connector' is not defined. [eslint: no-undef]

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Moving to p3 because no activity for at least 24 weeks.
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 5

[Hemakshi Sachdev [:hemakshis]]

Hi @honza,

If this issue is still unsolved, I would like to give this a try! Could it be assigned to me?

Hemakshi

Comment 6

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

@Alex, it looks like you are not working on this. Can I reassign it?

Honza

Comment 7

[Alexandre Poirot [:ochameau]]

Sure. I don't know if this bug is (still) relevant. DAMP was reporting mixed results, but perf-html may help confirm if such patch improves the perf or not.

Comment 8

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

(In reply to Hemakshi Sachdev [:hemakshis] from comment #5)

If this issue is still unsolved, I would like to give this a try! Could it be assigned to me?
Done

Honza

Comment 9

[Hemakshi Sachdev [:hemakshis]]

Attachment: Bug 1434848 - StatusBar tries to updates on every state update and its render is slow. r=Honza

Converted StatusBar to a Component and added a shouldComponentUpdate method to prevent unnecessary updates.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c9d39de3650
StatusBar tries to updates on every state update and its render is slow. r=Honza

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2c9d39de3650