report: reuse numberFormatters for ~50% performance gains #14493
Conversation
numberFormatters
report/renderer/details-renderer.js
Outdated
| return this._renderTable(details); | ||
| { | ||
| // Defer rendering of hidden tables, for a faster FCP | ||
| const tableElem = this._dom.createElement('table', 'lh-table'); |
There was a problem hiding this comment.
no way this didnt break tests
report/renderer/i18n.js
Outdated
| return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2); | ||
| let formatter; | ||
| // eslint-disable-next-line max-len | ||
| const cacheKey = `${opts.minimumFractionDigits}${opts.maximumFractionDigits}${opts.style}${opts.unit}${opts.unitDisplay}`; |
There was a problem hiding this comment.
maybe an array + join('')?
There was a problem hiding this comment.
Feels like this should really be a thing in js by now.
Could also do JSON.stringify(Object.entries(opts).sort(([keyA], [keyB]) => keyA.localeCompare(keyB))) (and throw in locale at the end or whatever)
report/renderer/i18n.js
Outdated
| return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2); | ||
| let formatter; | ||
| // eslint-disable-next-line max-len | ||
| const cacheKey = `${opts.minimumFractionDigits}${opts.maximumFractionDigits}${opts.style}${opts.unit}${opts.unitDisplay}`; |
There was a problem hiding this comment.
seems safe to not include locale in the key, but maybe we might as well? to protect against a future refactor
connorjclark
left a comment
connorjclark
left a comment
There was a problem hiding this comment.
re: caching the formatter, nice! IIRC the hope was "surely v8 would do this for us". Should we file a crbug for them?
|
v8 has only ever cached the simplest formatters (e.g. the default |
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
50ms still seems slow for table rendering, honestly :/
report/renderer/i18n.js
Outdated
| return new Intl.NumberFormat(this._locale, opts).format(number).replace(' ', NBSP2); | ||
| let formatter; | ||
| // eslint-disable-next-line max-len | ||
| const cacheKey = `${opts.minimumFractionDigits}${opts.maximumFractionDigits}${opts.style}${opts.unit}${opts.unitDisplay}`; |
There was a problem hiding this comment.
Feels like this should really be a thing in js by now.
Could also do JSON.stringify(Object.entries(opts).sort(([keyA], [keyB]) => keyA.localeCompare(keyB))) (and throw in locale at the end or whatever)
report/renderer/details-renderer.js
Outdated
| case 'opportunity': | ||
| return this._renderTable(details); | ||
| { | ||
| // Defer rendering of hidden tables, for a faster FCP |
There was a problem hiding this comment.
This is a really good idea in general, but feels like there could be a more principled way of doing it. What about deferring all audit detail rendering to a second pass and triggering it up in renderReport()?
There was a problem hiding this comment.
Good idea. We probably need a special case for filmstrip (and maybe *-budgets) but.. otherwise all of detailsrenderer deferred seems cool.
This comment was marked as spam.
This comment was marked as spam.
paulirish
changed the title
connorjclark
changed the title
5 participants
deferring table rendering stuff that we are _deferring_ to another PR
baseline perf of renderReport.. ~140ms JS plus rendering:
after deferring
renderTable's impl.. just 70ms of JS before FCP:and then
reusing formatters brings 120ms of table rendering down to 50ms:
that is renderReport being 50% faster!!!!!!!