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?

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

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

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.

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.

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!

Comment on attachment 8916098 [details] Bug 1406311 - more tests for devtools l10n numbers method; https://reviewboard.mozilla.org/r/187334/#review192706

(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!

Comment on attachment 8916099 [details] Bug 1406311 - add unit test to cover sprintfjs; https://reviewboard.mozilla.org/r/187336/#review194524

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

Comment on attachment 8916101 [details] Bug 1406311 - sprintfjs: optimise string-format for %S patterns; https://reviewboard.mozilla.org/r/187340/#review194528

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.

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

(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!

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.

Comment on attachment 8916100 [details] Bug 1406311 - sprintfjs: remove getType; https://reviewboard.mozilla.org/r/187338/#review195550

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

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

(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

(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!