Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i18n: reduce unnecessary message formats #14030

Merged
merged 3 commits into from
May 31, 2022
Merged

i18n: reduce unnecessary message formats #14030

merged 3 commits into from
May 31, 2022

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented May 19, 2022

alternate approach to memoization in #14026

Memoization needs to be on the icuMessageFn and the ICU replacement values, since messages with different replacements are going to end up different as different strings in the end. One way to avoid the extra arg bookkeeping work would be to memoize only strings with no replacements, which is most of the strings (especially during config generation, where it's mostly checking audit title, failureTitle, description, etc.

The root performance problem is that IntlMessageFormat is relatively slow, and we call it a lot. If you look at a profile, it's spending almost all its time parsing each message (which to be fair, is apparently much faster in more recent intl-messageformat versions and I'm the one who has resisted updating).

Since ICU placeholders have to occur inside {}, and most of our strings don't have any replacements, this PR instead adds a simple filter that just checks if the message has a { in it before attempting to parse it. This will catch all messages with placeholders (letting in false positives with escaped { in them, but I don't believe we have any right now?). The result is equivalent to the "memoize only strings with no replacements" mentioned above.

This is slightly less efficient than memoizing the whole thing, but almost all formatted strings have no placeholders, and of those that do, only a fraction will have repeated placeholder replacements that would get a cache hit.

75 out of 880 messages (8.5%) in en-US.json have ICU placeholders in them, but almost all of those are things like displayValues and the like, which are used rarely. In a real run using the command in #14026

node ./lighthouse-cli --quiet -A=./lighthouse-core/test/results/artifacts --config-path=./lighthouse-core/test/results/sample-config.js --output=json --output-path=./lighthouse-core/test/results/sample_v2.json

of the 16,608 messages that pass through getIcuMessageFn, 42 (0.25%) need a replacement, so it's probably not worth the extra effort for them.


results of

node ./lighthouse-cli --quiet -A=./lighthouse-core/test/results/artifacts --config-path=./lighthouse-core/test/results/sample-config.js --output=json --output-path=./lighthouse-core/test/results/sample_v2.json; 
node -e "t = require('./lighthouse-core/test/results/sample_v2.json'); console.log(t.timing.entries.filter(e => e.name === 'lh:config').at(-1).duration)"
# this branch
376.38
176.09
170.07
181.17
189.9

# master
541.81
522.81
497.86
538.26
512.19

which look roughly equivalent to the #14026 improvements to me

@brendankenny brendankenny requested a review from a team as a code owner May 19, 2022 20:06
@brendankenny brendankenny requested review from connorjclark and removed request for a team May 19, 2022 20:06
function formatMessage(message, values, locale) {
// Parsing and formatting can be slow. Don't attempt if the string can't
// contain ICU placeholders, in which case formatting is already complete.
if (!message.includes('{') && values === undefined) return message;
Copy link
Member Author

@brendankenny brendankenny May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also requires that values is undefined so it preserves the behavior of throwing on invalid values for messages with no placeholders

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simpler! i dig it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants