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

RESOLVED FIXED in Firefox 68

Status

enhancement
P3
normal
RESOLVED FIXED
Last year
3 months ago

People

(Reporter: ochameau, Assigned: hemakshis)

Tracking

unspecified
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(2 attachments)

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.
Reporter

Updated

Last year
Blocks: 1434855
Reporter

Updated

Last year
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request)
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.
Attachment #8947450 - Flags: review?(odvarko)

Comment 3

Last year
mozreview-review
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]
Status: NEW → ASSIGNED
Priority: -- → P1

Updated

Last year
Product: Firefox → DevTools
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
Priority: P1 → P3
Assignee

Comment 5

3 months ago

Hi @honza,

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

Hemakshi

Flags: needinfo?(odvarko)

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

Honza

Flags: needinfo?(odvarko) → needinfo?(poirot.alex)
Reporter

Comment 7

3 months ago

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.

Assignee: poirot.alex → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(poirot.alex)

(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

Assignee: nobody → sachdev.hemakshi
Status: NEW → ASSIGNED
Assignee

Comment 9

3 months ago

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

Comment 10

3 months ago
Pushed by jodvarko@mozilla.com:
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

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.