Speed up non-ISO calendar tests about 10x by caching DateTimeFormat instances #1624
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1624 +/- ##
=======================================
Coverage 91.04% 91.04%
=======================================
Files 17 17
Lines 10705 10714 +9
Branches 1600 1601 +1
=======================================
+ Hits 9746 9755 +9
Misses 943 943
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
justingrant
changed the title
See tc39/proposal-temporal#1624 for details.
Add proper caching of global builtins like other Temporal code uses.
I was wrong about what was making non-ISO calendars so slow. I thought the problem was `formatToParts()`, but it turns out that the `DateTimeFormat` constructor is really slow and also allocates ridiculous amounts of RAM. See more details here: https://bugs.chromium.org/p/v8/issues/detail?id=6528 @littledan in https://bugs.chromium.org/p/v8/issues/detail?id=6528#c4 recommended to cache DateTimeFormat instances, so that's what this commit does. The result is a 6x speedup in non-ISO calendar tests. Before: 6398.83ms After: 1062.26ms A similar speedup is likely for `ES.GetCanonicalTimeZoneIdentifier`. Caching time zone canonicalization (in a separate PR) should have a big positive impact on ZonedDateTIme and TimeZone perf. Many thanks to @fer22f for uncovering this optimization in js-temporal/temporal-polyfill#7.
|
I've split it up into two commits. |
See tc39/proposal-temporal#1624 for details.
See tc39/proposal-temporal#1624 for details.
|
I guess this meets the bar for what we should port over from the new polyfill, if it speeds up the tests so significantly. It's not important for this part of the code to keep matching the spec (because there is no part of the spec for it to match - the non-ISO calendars are implementation-defined.) |
I was wrong about what was making non-ISO calendars so slow. I thought the problem was
formatToParts(), but it turns out that theDateTimeFormatconstructor is really slow and also allocates ridiculous amounts of RAM. See more details here: https://bugs.chromium.org/p/v8/issues/detail?id=6528@littledan in https://bugs.chromium.org/p/v8/issues/detail?id=6528#c4 recommended to cache DateTimeFormat instances, so that's what this commit does.
The result is about a
6x10x speedup in non-ISO calendar tests.1062.26ms626.64ms (UPDATE: final code is even faster)I also added proper caching of global builtins like other Temporal code uses.
Many thanks to @fer22f for uncovering this optimization (and its root cause) in js-temporal/temporal-polyfill#7. As he notes in that issue, a similar speedup is likely for
ES.GetCanonicalTimeZoneIdentifier. Caching time zone canonicalization (in a separate PR) should have a big positive impact on ZonedDateTIme and TimeZone perf.