fix: Cache Intl.DateTimeFormat instances for better performance#4141
fix: Cache Intl.DateTimeFormat instances for better performance#4141TrevorBurnham wants to merge 2 commits into
Conversation
TrevorBurnham
force-pushed
the
format-date-localized-caching
branch
from
b4e5960 to
1bbc389
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4141 +/- ##
=======================================
Coverage 97.59% 97.59%
=======================================
Files 950 951 +1
Lines 30642 30658 +16
Branches 11229 11230 +1
=======================================
+ Hits 29906 29922 +16
- Misses 689 729 +40
+ Partials 47 7 -40 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /** | ||
| * Cache for Intl.DateTimeFormat instances. | ||
| * | ||
| * Creating Intl.DateTimeFormat objects is expensive because the browser must |
There was a problem hiding this comment.
How much performance gain are we expecting from this caching mechanism?
There was a problem hiding this comment.
It's tiny on a per-call basis: something on the order of 10-100 microseconds. But it's cumulative per localized date. On a page with a table full of formatted dates, the savings could add up to 10s of milliseconds.
amanabiy
force-pushed
the
format-date-localized-caching
branch
from
2d5c154 to
88ce863
Compare
| /** | ||
| * Cache for Intl.DateTimeFormat instances. | ||
| * | ||
| * Creating Intl.DateTimeFormat objects is expensive because the browser must | ||
| * parse locale data and resolve options. This cache stores formatter instances | ||
| * keyed by locale and options, allowing reuse across multiple format calls. | ||
| * | ||
| * The cache uses a simple Map with a string key derived from the locale and | ||
| * serialized options. Since the number of unique locale/option combinations | ||
| * in a typical application is small and bounded, we don't implement cache | ||
| * eviction. | ||
| */ |
There was a problem hiding this comment.
The comment here is verbose, The file and function names are descriptive. Let's try to simplify the comment, removing redundancy. we could keep why we don't implement cache eviction (and keep it close to the formatterCache.
There was a problem hiding this comment.
Good call. I've trimmed the docblocks down to just the why:
- Top comment reduced to a two-line rationale for the cache existing.
- Moved the no-eviction note directly onto the
formatterCachedeclaration, as you suggested. - Dropped the
getDateTimeFormatdocblock since it only restated the signature. - Kept the one non-obvious line on
createCacheKey(sorting keys for order-independent hits).
2 participants
Description
Currently, every call to
formatDateLocalized(used inDateInput) allocates newIntl.DateTimeFormatinstances, which have significant initialization overhead. This PR adds caching for those formatters so that they're only created the first timeformatDateLocalizedis called.How has this been tested?
I've added thorough unit test coverage.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.