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
|
v8 has only ever cached the simplest formatters (e.g. the default |
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
| @@ -53,7 +53,12 @@ export class DetailsRenderer { | |||
| return this._renderList(details); | |||
| case 'table': | |||
| 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
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!!!!!!!