StatusBar updates too frequently and/or should cache l10n values

RESOLVED FIXED in Firefox 58

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: ochameau, Assigned: pradeepgangwar, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 58
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fix-optional, firefox58 fixed)

Details

(Whiteboard: [good first bug][mentor-lang=zh][lang=js])

Attachments

(1 attachment)

Reporter

Description

2 years ago
https://perfht.ml/2yvmluY
  Search for "StatusBar"
  Expand the first item in the Call Tree until seeing StatusBar.

You will see in the profile that StatusBar is slow because of its calls to L10N APIs getFormat* and getStr.

http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/status-bar.js#26
When you look at its implementation you can see many call to L10N.getStr for example.
At very least all L10N values should be cached. We call StatusBar() about once per request. We at least update the number of requests. But most of its content doesn't change...

Updated

2 years ago
Mentor: gasolin
Keywords: good-first-bug
Priority: -- → P3
If anyone is interesting in picking this bug, you can refer the `mdn-link` component, define L10N strings as `const` like
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/mdn-link.js#17
I am new one here. Please let me know, do i need to build mozilla-central repo to work on this issue or any other build is required?
Flags: needinfo?(gasolin)
Pradeep, you have to clone the repo from mozilla-central,

then you can build the whole mozilla-central repo by following
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Or just follow the guide to run netmonitor on a browser tab.
https://github.com/mozilla/gecko-dev/tree/master/devtools/client/netmonitor#quick-setup


You can reply on bugzilla, use IRC #devtools channel or slack if you have any question
http://firefox-dev.tools/
Flags: needinfo?(gasolin)
Hey! I am looking to make my first contributions to the open-source community and this seems like a good bug to get started on. If Pradeep isn't working on this bug, can it be assigned to me?

Comment 5

2 years ago
There are some places where we use call L10 with parameters - for example
```L10N.getFormatStrWithNumbers("networkMenu.summary.finish",
    getFormattedTime(millis)```
- Can we cache these as well?
Flags: needinfo?(gasolin)
I have cloned and built mozilla-central repo. Since i am new one here with little understanding of mozilla's code. Please guide me a little to get started.
Hi Pradeep,

Please check the guide here to run the network monitor on browser tab
https://github.com/mozilla/gecko-dev/tree/master/devtools/client/netmonitor#quick-setup

If you have any question, please let me know
Flags: needinfo?(gasolin)
Once you can run on the browser tab, check `netmonitor/src/components/status-bar.js`

You'll see several `L10N.getStr` call inside. We should declare them as const out of function StatusBar to get some performance gain. (because StatusBar will refresh lots of times during loading)

We should keep `L10N.getFormatStrWithNumbers` as it is because they should calc dynamically.
mithilan91, since Pradeep show his interest on this issue first, you can find another good-first-bug to solve.
Welcome to ping me on irc or slack if you need help.
Flags: needinfo?(mithilan91)

Updated

2 years ago
Assignee: nobody → pradeepgangwar39
Thanks, Fred Lin for all the help and mentoring :). I have made necessary changes to the file. Please let me know how do i proceed further to get my code reviewed.
Also on using `hg diff` i see yarn.lock has been modified. Please let me know if it was supposed to change or there is something wrong.
Comment hidden (mozreview-request)
As it was my first patch so, after getting help from IRC I was able to make a patch through Mozilla Review Board.

Comment 14

2 years ago
mozreview-review
Comment on attachment 8915635 [details]
Bug 1404130- Cache L10N values in status bar;

https://reviewboard.mozilla.org/r/186828/#review192052

Nice work! 

For update submittion, make sure you squashed all changes into single commit. 
You can add `;r=gasolin` at the end of the commit message, therefore mozreview will set the right review flag for your patch.

Please address below issues and it will be good to go.

::: devtools/client/netmonitor/src/components/status-bar.js:27
(Diff revision 1)
>  const { L10N } = require("../utils/l10n");
>  
>  const { button, div } = DOM;
>  
> +const REQUESTS_COUNT_EMPTY = L10N.getStr("networkMenu.summary.requestsCountEmpty");
> +const PERF = L10N.getStr("networkMenu.summary.tooltip.perf");

PERF is not a good name since pref/perf are used several places, `TOOLTIP_PERF` will be more specific

::: devtools/client/netmonitor/src/components/status-bar.js:28
(Diff revision 1)
>  
>  const { button, div } = DOM;
>  
> +const REQUESTS_COUNT_EMPTY = L10N.getStr("networkMenu.summary.requestsCountEmpty");
> +const PERF = L10N.getStr("networkMenu.summary.tooltip.perf");
> +const REQUESTS_COUNT = L10N.getStr("networkMenu.summary.tooltip.requestsCount");

To enhance the readability, Please add `TOOLTIP_` prefix for the rest of declarations (if they are tooltip), such as TOOLTIP_REQUESTS_COUNT/TOOLTIP_TRANSFERRED

Comment 15

2 years ago
mozreview-review
Comment on attachment 8915635 [details]
Bug 1404130- Cache L10N values in status bar;

https://reviewboard.mozilla.org/r/186828/#review192056

The commit message could be `Bug 1404130- Cache L10N values in status bar;r=gasolin` to reflect the actual work done in this patch
Reporter

Comment 16

2 years ago
With Praddeep's patch, we go from 2%/65ms down to 1%/43ms:
  https://perfht.ml/2xmarmo
But StatusBar is still taking too much time regarding what it does (which is really simple).

Fred, do you think we could reduce the calls to getFormattedTime/getFormattedSize/getFormatStrWithNumbers? Could you somehow cache this? I mean, in many case, the values won't change, so there is no need to recompute the translation. May be componentWillReceiveProps can help?

Otherwise, this issue highlights slowness in L10N library and we should also fix that as it impact various other components. I opened bug 1404130 to try to fix some of that.
Flags: needinfo?(gasolin)
Reporter

Updated

2 years ago
Depends on: 1406311
Reporter

Comment 17

2 years ago
> I opened bug 1404130 to try to fix some of that.

I meant bug 1406311.
It looks like I should wait till Bug 1406311 is resolved. Or I can implement the changes what Fred told in the review.
Reporter

Comment 19

2 years ago
No need to wait, it can be done in parallel.
When the values doesn't change, React won't trigger update since we already did some cache via reselect
http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/selectors/requests.js#88


For the case when update is triggered from other property and L10N methods are too expensive, we could store current value and formatted strings, then lookup if we hit the same result. ex:


```
// outside of the function
let summaryCache = {
  countText: { count: 3, str: 'formatedString' }
  transferText: { transferredSize: 2, str: 'formatedString'}
  finishText: { millis: 100, str: 'formatedString'}
}

...
// in function StatusBar

// check if value is equalled and return the catched string
let finishText = summaryCache.finishText.millis === millis ? summaryCache.finishText.str : L10N.getFormatStrWithNumbers("networkMenu.summary.finish", getFormattedTime(millis));
```

How do you think?
Flags: needinfo?(gasolin) → needinfo?(poirot.alex)
Praddeep, please keep working on update the current patch, the above discussion could be done in the separate patch.
Flags: needinfo?(pradeepgangwar39)
(In reply to Fred Lin [:gasolin] from comment #21)
> Praddeep, please keep working on update the current patch, the above
> discussion could be done in the separate patch.

Yeah I will do it in evening today. Thanks!
Flags: needinfo?(pradeepgangwar39)

Updated

2 years ago
Whiteboard: [good first bug][mentor-lang=zh][lang=js]
Comment hidden (mozreview-request)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8915635 [details]
Bug 1404130- Cache L10N values in status bar;

https://reviewboard.mozilla.org/r/186828/#review192328

Looks good!
I've trigger the try build on try server to make sure the change doesn't break any existing test cases. Once test passed the patch is ready to land.

At the mean time if you still interest in solving more issues, here are some suggestions

1. search L10N usage in netmonitor/ folder, do the similar change as this bug (you can file bugs like this one) , or
2. consider do some experiment based on Comment 20 and check if there's any performance gain
3. find next bug in http://bugs.firefox-dev.tools/?easy&tool=all

Thanks for your contribution :)
Attachment #8915635 - Flags: review?(gasolin) → review+
I see that some of the tests failed. Please help me with it and let me know what needs to be changed.
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=496ca123654e&selectedJob=135378280

ES fail means some lint issue happens

For this case the issue is this line is too long
```
const TOOLTIP_DOM_CONTENT_LOADED = L10N.getStr("networkMenu.summary.tooltip.domContentLoaded");
```

Please define as
```
const TOOLTIP_DOM_CONTENT_LOADED =(No trail here)
  L10N.getStr("networkMenu.summary.tooltip.domContentLoaded");
```

And the lint will be fine
Flags: needinfo?(mithilan91) → needinfo?(pradeepgangwar39)
Ok! I will do it very soon.
Flags: needinfo?(pradeepgangwar39)
Reporter

Comment 28

2 years ago
(In reply to Fred Lin [:gasolin] from comment #20)
> When the values doesn't change, React won't trigger update since we already
> did some cache via reselect
> http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/
> selectors/requests.js#88
> 
> 
> For the case when update is triggered from other property and L10N methods
> are too expensive, we could store current value and formatted strings, then
> lookup if we hit the same result. ex:

We may wait for bug 1406311 and revisit that after as it looks like l10n.js can be made much faster.
Flags: needinfo?(poirot.alex)
Comment hidden (mozreview-request)

Comment 30

2 years ago
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4753cd9ea2d9
Cache L10N values in status bar; r=gasolin

Updated

2 years ago
See Also: → 1407548

Updated

2 years ago
See Also: → 1407550

Updated

2 years ago
See Also: → 1407552
https://hg.mozilla.org/mozilla-central/rev/4753cd9ea2d9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.