Closed Bug 1406312 Opened 2 years ago Closed 2 years ago

Waterfall's timingBoxes is slow because of its l10n calls

Categories

(DevTools :: Netmonitor, enhancement, P3)

enhancement

Tracking

(firefox57 fix-optional, firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: ochameau, Assigned: abhinav.koppula)

References

Details

Attachments

(2 files)

https://perfht.ml/2xmarmo
  * Select "Filer: JS Only"
  * Search for timingBoxes
  * Expand the call tree until ending up on "timingBoxes"
  * See getFormatStr taking 41ms and timingBoxes 74ms

This bug is similar to bug 1404130. Calling L10N function is slow.
bug 1406311 is about making them faster, but may be we could make tooltip computation lazy? May be it can be computed only when we hover the element with the tooltip?
Thanks for the profiling, from the screenshot it looks L10N getFormatString cause most of time on render the waterfall.
Hi Fred,
Can I try this one?
Hi Fred, Alexandre,
I have given a shot at this issue. Would this help in performance improvement?
(In reply to Abhinav Koppula from comment #4)
> Hi Fred, Alexandre,
> I have given a shot at this issue. Would this help in performance
> improvement?

Yes! I see "timingBoxes" go from 117ms down to 53ms.
getFormatStr still takes 25ms, so is still significant, but hopefully julian is going to improve it via bug 1406311.
Assignee: nobody → abhinav.koppula
Comment on attachment 8916525 [details]
Bug 1406312 - Lazy loading of tooltip text on hover in Waterfall Timing boxes.

https://reviewboard.mozilla.org/r/187658/#review192956

Looks good! Just need to address some code consitency issue.

And L10N might the reason let resize feel slow, since we need do all string formatting again and again during resize...

For the left L10N.getFormatStr. Chrome put the time label in a separate time column, we could have that too.

::: devtools/client/netmonitor/src/components/request-list-column-waterfall.js:42
(Diff revision 1)
>        this.props.firstRequestStartedMillis !== nextProps.firstRequestStartedMillis;
>    },
>  
>    render() {
>      let { firstRequestStartedMillis, item, onWaterfallMouseDown } = this.props;
> -    const { boxes, tooltip } = timingBoxes(item);
> +    const { boxes } = timingBoxes(item);

Since we have one return value now, can do `const boxes = timingBoxes(item);`

::: devtools/client/netmonitor/src/components/request-list-column-waterfall.js:50
(Diff revision 1)
> -        div({
> +      div({
> +        className: "requests-list-column requests-list-waterfall",
> +        onMouseOver: function ({target}) {
> +          if (!target.title) {
> +            let tooltipTitle = timingTooltip(item);
> +            target.title = tooltipTitle;

can do `target.title = timingTooltip(item);` without define `tooltipTitle` variable

::: devtools/client/netmonitor/src/components/request-list-column-waterfall.js:98
(Diff revision 1)
>    let { eventTimings, fromCache, fromServiceWorker, totalTime } = item;
>    let boxes = [];
> -  let tooltip = [];
>  
>    if (fromCache || fromServiceWorker) {
> -    return { boxes, tooltip };
> +    return { boxes };

`return boxes;`

::: devtools/client/netmonitor/src/components/request-list-column-waterfall.js:131
(Diff revision 1)
>        }, title)
>      );
> -    tooltip.push(L10N.getFormatStr("netmonitor.waterfall.tooltip.total", totalTime));
>    }
>  
>    return {

return boxes;
Attachment #8916525 - Flags: review?(gasolin) → review+
Blocks: 1407561
Hi Fred,
Thanks for the review. Have made the suggested changes.
Can you check once?
Thanks for update, test green, I'll try to land it.
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e92ca171760d
Lazy loading of tooltip text on hover in Waterfall Timing boxes. r=gasolin
https://hg.mozilla.org/mozilla-central/rev/e92ca171760d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.