Alexandre Poirot [:ochameau] Reporter
	Description • 7 years ago

L10N calls make simple tasks in network monitor be slow.
It looks like there is some unjustified overhead in devtools/shared/l10n.js

https://perfht.ml/2xmbFhn
See this profile.
* Calling toLocaleString is expensive and we call it two times from numberWithDecimals:
http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/devtools/shared/l10n.js#147-158
  Is this really needed?
* It looks like we create many regexp from sprintf, may be we could memoize some?
* getFormatStr seems to be slow, may be because of sprintf?

	Julian Descottes [:jdescottes] Assignee
	Comment 1 • 7 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #0)
> L10N calls make simple tasks in network monitor be slow.
> It looks like there is some unjustified overhead in devtools/shared/l10n.js
> 
> https://perfht.ml/2xmbFhn
> See this profile.

Indeed, ~10% of time spent in l10n.js is a lot. 

> * Calling toLocaleString is expensive and we call it two times from
> numberWithDecimals:
> http://searchfox.org/mozilla-central/rev/
> 8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/devtools/shared/l10n.js#147-158
>   Is this really needed?

Seems like the only case where we bail out after the first toLocaleString is for numbers such as 1.0001. They fail the integer check in the beginning of the method, but 1.0001.toLocaleString() === "1". If we call toLocaleString only once with the provided decimals parameters, we will get "1.00" (number of 0s depending on decimals). We can then check if the string ends with /\.0+$/ and in this case return just the part before the separator.  

> * It looks like we create many regexp from sprintf, may be we could memoize
> some?
I think all regexps from sprintf are only created once? See http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/devtools/shared/sprintfjs/sprintf.js#34-50

> * getFormatStr seems to be slow, may be because of sprintf?

Sprintf does a parse+format. Parsing is cached so we'll need to see what we can improve in format. I think we could try to reorder the code to make the most common paths for devtools faster.

	Julian Descottes [:jdescottes] Assignee
	Comment 2 • 7 years ago

(In reply to Julian Descottes [:jdescottes] from comment #1)
> (In reply to Alexandre Poirot [:ochameau] from comment #0)
> > L10N calls make simple tasks in network monitor be slow.
> > It looks like there is some unjustified overhead in devtools/shared/l10n.js
> > 
> > https://perfht.ml/2xmbFhn
> > See this profile.
> 
> Indeed, ~10% of time spent in l10n.js is a lot. 
> 
> > * Calling toLocaleString is expensive and we call it two times from
> > numberWithDecimals:
> > http://searchfox.org/mozilla-central/rev/
> > 8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/devtools/shared/l10n.js#147-158
> >   Is this really needed?
> 
> Seems like the only case where we bail out after the first toLocaleString is
> for numbers such as 1.0001. They fail the integer check in the beginning of
> the method, but 1.0001.toLocaleString() === "1". If we call toLocaleString
> only once with the provided decimals parameters, we will get "1.00" (number
> of 0s depending on decimals). We can then check if the string ends with
> /\.0+$/ and in this case return just the part before the separator.  
> 

I just quickly tried that and toLocaleString() itself is not expensive. It's the second call that takes most of the time, when we specify maximumFractionDecimals etc... So just calling this one once won't improve anything.

Talking with Flod on IRC, I was pointed at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat. It's just another way of calling the same formatting method, except we can memoize the formatter. With this I go from 2000ms to 250ms on 100 000 calls.

	Julian Descottes [:jdescottes] Assignee
	Comment 9 • 7 years ago

I didn't manage to find a good way to speed up getFormatStr, but I have a few improvements to propose. I measured perfs on 2 benchmarks, one calling a simple pattern with a single %S, another one with a more complex pattern with positioned arguments.


              simple       complex
baseline         410           570 (high variance for complex one)
no spread        380           540
get_type         310           410
switch           305           400
simple pattern   150           400

(in ms, for 1,000,000 calls, average on 5 cold runs for each)

I'll also try to strip sprintfjs from all the features we are not using to see if it's worth simplifying more. (looking at our coverage info, we only use %S and %d). If we go and optimize it I guess we should just rewrite it completely and make it part of our codebase.

	Julian Descottes [:jdescottes] Assignee
	Comment 17 • 7 years ago

I spent a bit of time improving l10n.js. After simplifying sprintfjs to the extreme, I'm down to 285ms on my benchmarks. In theory numberWithDecimals is way faster now and getFormatStr is ~two times faster.

Here are two profiles where I reloaded a page 10 times while being on the netmonitor.

- without my patches: https://perfht.ml/2xY3pbI
- with my patches: https://perfht.ml/2xY3tIu

Overall, time spent:

                          without patches        with patches
numberWithDecimals                  120ms                36ms  
getFormatStr                        124ms                52ms

It's hard to really compare profiles per se, but it looks like there is some improvement.
I probably should also look at getFormatStrWithNumbers which now shows up more significantly in the profile.

	Alexandre Poirot [:ochameau] Reporter
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8915964 [details]
Bug 1406311 - memoize number formatters in devtools l10n.js;

https://reviewboard.mozilla.org/r/187248/#review192700

With this patch I do not see StatusBar overall time stripping down, but getFormatStrWithNumbers is clearly faster as it goes from 25% on StatusBar overall exec time down to 9%.
And on the whole profile getFormatStrWithNumbers is also 2x faster.

Now, I'm wondering if DAMP would report any win?
I imagine not as getFormatStrWithNumbers takes less than 1% over the complete profile time.

Thanks for jumping on this!

	Alexandre Poirot [:ochameau] Reporter
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8916098 [details]
Bug 1406311 - more tests for devtools l10n numbers method;

https://reviewboard.mozilla.org/r/187334/#review192706

	Alexandre Poirot [:ochameau] Reporter
	Comment 20 • 7 years ago

(In reply to Julian Descottes [:jdescottes] from comment #17)
> It's hard to really compare profiles per se, but it looks like there is some
> improvement.
> I probably should also look at getFormatStrWithNumbers which now shows up
> more significantly in the profile.

If you filter by l10n.js you go from 352ms(~2%) down to 137ms (~1%) for l10n.js

On my side, when applying all patches, with only one page reload against my test page,
I go from 307ms(~3%) down to 87ms(~1%) for l10n.js.
This time, such win may be confirmed by DAMP!

	Brian Grinstead [:bgrins]
	Comment 29 • 7 years ago
mozreview-review

Comment on attachment 8916099 [details]
Bug 1406311 - add unit test to cover sprintfjs;

https://reviewboard.mozilla.org/r/187336/#review194524

	Brian Grinstead [:bgrins]
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8916100 [details]
Bug 1406311 - sprintfjs: remove getType;

https://reviewboard.mozilla.org/r/187338/#review194526

You mention that license information will be updated in another commit. I can't tell which commit that is done in but I'd prefer if you update the license information with this one

	Brian Grinstead [:bgrins]
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8916101 [details]
Bug 1406311 - sprintfjs: optimise string-format for %S patterns;

https://reviewboard.mozilla.org/r/187340/#review194528

	Brian Grinstead [:bgrins]
	Comment 32 • 7 years ago
mozreview-review

Comment on attachment 8916102 [details]
Bug 1406311 - string-format: string-format: use typeof instead of getType();

https://reviewboard.mozilla.org/r/187342/#review194530

::: devtools/shared/string-format.js:69
(Diff revision 4)
>    for (i = 0; i < treeLength; i++) {
> -    nodeType = getType(parseTree[i]);
> +    nodeType = typeof parseTree[i];
> +    // parseTree item is either a string or an array (return of match() call).
>      if (nodeType === "string") {
>        output[output.length] = parseTree[i];
> -    } else if (nodeType === "array") {
> +    } else if (nodeType === "object") {

Should we use `Array.isArray()` here instead to be sure that is what we are dealing with?  

Likewise, could we get the same performance win without this patch by instead using modifying `getType` to something like:

```
function getType(variable) {
   let type = typeof variable;
   if (type === "number" || type === "string" || type === "function") {
     return type;
   }
   if Array.isArray(variable) {
     return "array";
   }
   return Object.prototype.toString.call(variable).slice(8, -1).toLowerCase();
}
```

I haven't tested it locally, but it appears the function and array cases are the two that are expected in this function but falling into the Object.prototype.toString call.

	Julian Descottes [:jdescottes] Assignee
	Comment 33 • 7 years ago

(In reply to Brian Grinstead [:bgrins] from comment #32)
> Comment on attachment 8916102 [details]
> Bug 1406311 - string-format: string-format: use typeof instead of getType();
> 
> https://reviewboard.mozilla.org/r/187342/#review194530
> 
> ::: devtools/shared/string-format.js:69
> (Diff revision 4)
> >    for (i = 0; i < treeLength; i++) {
> > -    nodeType = getType(parseTree[i]);
> > +    nodeType = typeof parseTree[i];
> > +    // parseTree item is either a string or an array (return of match() call).
> >      if (nodeType === "string") {
> >        output[output.length] = parseTree[i];
> > -    } else if (nodeType === "array") {
> > +    } else if (nodeType === "object") {
> 
> Should we use `Array.isArray()` here instead to be sure that is what we are
> dealing with?  
> 

Comes with a 10% performance hit on the complex case in my benchmarks. 
Alternatively I can also remove the === "object" comparison which is misleading and add a more detailed comment.

> Likewise, could we get the same performance win without this patch by
> instead using modifying `getType` to something like:
> 
> ```
> function getType(variable) {
>    let type = typeof variable;
>    if (type === "number" || type === "string" || type === "function") {
>      return type;
>    }
>    if Array.isArray(variable) {
>      return "array";
>    }
>    return Object.prototype.toString.call(variable).slice(8,
> -1).toLowerCase();
> }
> ```
> 
> I haven't tested it locally, but it appears the function and array cases are
> the two that are expected in this function but falling into the
> Object.prototype.toString call.

I see a 20% slowdown when using a getType function (even when trying to tune it to be faster for our use case). I guess inlining `typeof` statements can lead to optimizations on the engine side.

On my machine I get the following benchmark results:

                    base      patch      patch+review
simple case        700ms      300ms             360ms
complex case      1500ms      430ms             550ms

Testing with benchmarks is not really ideal, and we shouldn't try to make the code unreadable just for a few ms that might not even make an impact in more realistic scenarios. For this patch, I didn't feel like getType was more readable than using typeof, that's why I inlined them. 

Given the test results above, let me know how you feel about this.

	Julian Descottes [:jdescottes] Assignee
	Comment 42 • 7 years ago

(In reply to Brian Grinstead [:bgrins] from comment #30)
> Comment on attachment 8916100 [details]
> Bug 1406311 - string-format: move sprintfjs to string-format;
> 
> https://reviewboard.mozilla.org/r/187338/#review194526
> 
> You mention that license information will be updated in another commit. I
> can't tell which commit that is done in but I'd prefer if you update the
> license information with this one

Sure, done in this commit.

gerv: I am not sure what we need to do in this case. We used the sprintfjs library. I moved and renamed it because I want to start modifying it more than for just a few quick nits.

The license mentions:

  Redistribution and use in source and binary forms, with or without
  modification, are permitted provided that the following conditions are met:
  * Redistributions of source code must retain the above copyright
    notice, this list of conditions and the following disclaimer.

Should I also include the MPL in the modified version? Should I make an explicit mention that points to the original library?
Thanks!

	Julian Descottes [:jdescottes] Assignee
	Comment 43 • 7 years ago

Comment on attachment 8916100 [details]
Bug 1406311 - sprintfjs: remove getType;

Cancelling review requests. As discussed with Brian on slack, will go for a more conservative approach for now and will investigate switching back to services.strings in the long run.

	Brian Grinstead [:bgrins]
	Comment 47 • 7 years ago
mozreview-review

Comment on attachment 8916100 [details]
Bug 1406311 - sprintfjs: remove getType;

https://reviewboard.mozilla.org/r/187338/#review195550

	Pulsebot
	Comment 49 • 7 years ago

Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70ab0eb9afe2
memoize number formatters in devtools l10n.js;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/23317a542b40
more tests for devtools l10n numbers method;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/d25069b1b75c
add unit test to cover sprintfjs;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/66d602064c8b
sprintfjs: remove getType;r=bgrins
https://hg.mozilla.org/integration/autoland/rev/41ab5d21e50a
sprintfjs: optimise string-format for %S patterns;r=bgrins

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 50 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/70ab0eb9afe2
https://hg.mozilla.org/mozilla-central/rev/23317a542b40
https://hg.mozilla.org/mozilla-central/rev/d25069b1b75c
https://hg.mozilla.org/mozilla-central/rev/66d602064c8b
https://hg.mozilla.org/mozilla-central/rev/41ab5d21e50a

	Gervase Markham [:gerv]
	Comment 51 • 7 years ago

(In reply to Julian Descottes [:jdescottes] from comment #42) 
> gerv: I am not sure what we need to do in this case. We used the sprintfjs
> library. I moved and renamed it because I want to start modifying it more
> than for just a few quick nits.
> 
> The license mentions:
> 
>   Redistribution and use in source and binary forms, with or without
>   modification, are permitted provided that the following conditions are met:
>   * Redistributions of source code must retain the above copyright
>     notice, this list of conditions and the following disclaimer.
> 
> Should I also include the MPL in the modified version? Should I make an
> explicit mention that points to the original library?

When we import code from elsewhere, it keeps the original license. We don't MPL everything we import - that makes contributing back upstream impossible. Make sure the directory has a LICENSE file giving the license. If we are shipping this code with Firefox, you'll need to add the text to about:license in order (tag me for a review).

Gerv

	Julian Descottes [:jdescottes] Assignee
	Comment 52 • 7 years ago

(In reply to Gervase Markham [:gerv] from comment #51)
> (In reply to Julian Descottes [:jdescottes] from comment #42) 
> > gerv: I am not sure what we need to do in this case. We used the sprintfjs
> > library. I moved and renamed it because I want to start modifying it more
> > than for just a few quick nits.
> > 
> > The license mentions:
> > 
> >   Redistribution and use in source and binary forms, with or without
> >   modification, are permitted provided that the following conditions are met:
> >   * Redistributions of source code must retain the above copyright
> >     notice, this list of conditions and the following disclaimer.
> > 
> > Should I also include the MPL in the modified version? Should I make an
> > explicit mention that points to the original library?
> 
> When we import code from elsewhere, it keeps the original license. We don't
> MPL everything we import - that makes contributing back upstream impossible.
> Make sure the directory has a LICENSE file giving the license. If we are
> shipping this code with Firefox, you'll need to add the text to
> about:license in order (tag me for a review).
> 
> Gerv

The sprintfjs file is already referenced properly in license.html (done in Bug 1296600). I wanted to port the file to something we would maintain (and consequently have no more interactions with the upstream version). So my question was more about this scenario. In any case we decided against moving the file and doing significant changes, so no action required here anymore I suppose. 
Thanks for having a look!