Performance improvements in reference code from profiling date arithmetic #3160
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3160 +/- ##
==========================================
- Coverage 96.96% 96.91% -0.06%
==========================================
Files 22 22
Lines 10157 10209 +52
Branches 1840 1839 -1
==========================================
+ Hits 9849 9894 +45
- Misses 259 266 +7
Partials 49 49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I did some measurements on a representative slow case (taking the difference between -002000-09-19 and 5000-01-01 in the various non-12-month calendars). The changes in this PR cut the time almost exactly in half in each case.
|
ptomato
force-pushed
the
3153-profile-date-arithmetic
branch
from
c691d80 to
f2dd2ab
Compare
gibson042
reviewed
Sep 26, 2025
ptomato
force-pushed
the
3153-profile-date-arithmetic
branch
from
f2dd2ab to
6543c99
Compare
|
The workflow is very simple! I use |
gibson042
approved these changes
Sep 27, 2025
justingrant
approved these changes
Oct 1, 2025
The RegExp was showing up on profiles. This is not the root cause of the slowness in #3153, but it noticeably helps. A RegExp is kind of overkill anyway, since the MonthCode grammar is very simple and the different elements occur at fixed indices in the string. Replacing the month code RegExp with very simple string-indexing code makes it go a lot faster. Adds a unit test suite for this file. See: #3153 Co-Authored-By: Richard Gibson <[email protected]>
I'm going to experiment if keys can be generated in a way that makes cache retrieval faster, so I will factor these out so that all the keys are generated in the same place. See: #3153
The spec has CONSTRAIN explicitly here. The overflow argument shouldn't be treated as optional for CalendarYearMonthFromFields and CalendarMonthDayFromFields. This doesn't observably affect any results, but regarding #3153, it avoids generating calendar cache keys with `undefined` as the overflow part of the key (should be `constrain` so that the same cache entry is fetched for the same overflow behaviour regardless of whether `constrain` was specified explicitly or not.)
Almost always when we populate an object with a copy of another object's cache, it is for calculations in the same calendar. So we can say that any particular object's cache is for only one calendar, and then we don't need to include the calendar ID in the lookup key. The only time when we copied the cache and it _wasn't_ for the same calendar, was after doing a withCalendar(). In this case it was useless to copy the cache anyway, because the entries from the old calendar would never be looked up anymore. So we change PlainDate and PlainDateTime's withCalendar methods to clone a fresh ISO Date Record object instead of reusing it. This provides an additional small speedup because it makes the keys shorter. Also fixes outdated comments about OneObjectCache; it associates caches with ISO Date Record objects, not Temporal objects. See: #3153
I'm not entirely sure how V8's Map.prototype.get algorithm works internally but using int32 keys seems to make the maps work much, much faster. Luckily, we can pack all the information for any cache key into 31 bits, now that we don't include the calendar ID in the key. See: #3153
Un-nesting this function makes the code easier to read, while also avoiding some iterations through calendar date properties. See: #3153
Object.entries is showing up as very hot on profiles of completeEraYear. It seems we are calling Object.entries on this table repeatedly, so may as well just store it in the entries format in the first place. See: #3153
As in the previous commit, Object.entries is showing up as very hot on profiles when performing date arithmetic in the Hebrew calendar. Turns out, the month information is actually used for two separate lookups: one by long name returned from Intl, and one by month code. We can separate the table into two tables, one for each lookup. We also make the min and max month length a 2-element array, so that we can avoid the property lookup in favour of an array element lookup. See: #3153
Sometimes when we reach this function, the month code is already present on the calendarDate parameter. It's a small speedup to just use it if it's there. (Not a large speedup, because calculating it doesn't involve a DateTimeFormat operation.) See: #3153
I found out that outside a certain date range, DateTimeFormat's methods throw a TypeError when using the Chinese calendar. We don't want to swallow this error, it is confusing to replace it with "Invalid ISO date" when the ISO date is clearly valid.
We computed and cached the number of days in each month in the Chinese calendar, but never used that information. Overriding daysInMonth and daysInPreviousMonth in the Chinese calendar helper to look up the cached month lengths cuts the time of a slow date difference almost in _half_. However, snapshot testing revealed that in some cases we cached incorrect daysInMonth information, which went unnoticed because we didn't use it until now. Fixing that requires removing the post-loop setting of `monthList[oldMonthString]` (that's wrong, because it's month 1 from the following calendar year.) See: #3153
Previously we cached the Chinese calendar's month list as an object indexed by Intl.DateTimeFormat month strings (e.g. "12", "4bis"), and providing the corresponding ordinal month and days in the month. Looking at what info we actually use: We use it to calculate the number of months in the year, which is already computed in getMonthList, so just add a monthsInYear property to the month list object. We lookup months by ordinal month, month code, and DTF month string. For month code lookups, we had to convert to DTF month string to index the month list object, and ordinal month lookups were expensive because we had to search the month list object's entries. So instead, cache the mappings both ways. We can look up month code by ordinal month and ordinal month by month code. Looking up by DTF month string is less common (the fromLegacyDate case) and it's trivial to convert a DTF month string to a month code. The daysInMonth are only looked up by ordinal month, so only put those in the ordinal month entries. This provides a modest speedup of arithmetic in the Chinese calendar. See: #3153
See issue #3158. This doesn't fix the issue, but adds an assertion that fails early when we hit that case, instead of later on with a more confusing error message.
ptomato
force-pushed
the
3153-profile-date-arithmetic
branch
from
6543c99 to
f9907ae
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #3153 - an exceptionally slow case when doing month arithmetic on non-ISO calendars, because we compute information on each month in a loop using Intl.DateTimeFormat, which is slow.
In #3154 and #3155 @gibson042 drastically improved the slowness for 12-month calendars, and suggested in #3153 (comment) an approach for doing the same for other calendars.
I plan to implement that approach for most of the remaining non-12-month calendars. However,
chineseanddangihave approximately 7 leap years per 19 years, but as far as I can tell that is an approximation and may not always be true. Since we might have to keep the current slow approach for those two calendars, I decided to try to speed up the calendar code by profiling first.I spent some time profiling several slow cases in various calendars and trying to speed up code that showed up as hot in the profiles. The results of that are this branch, which is best reviewed commit by commit.
I verified this code against snapshot tests.
I think more profiling after this would be diminishing returns. At this point, profiles are dominated by
Intl.DateTimeFormat.prototype.formatToPartsandMap.prototype.get/setand there's nothing else obviously taking a large fraction of the time.If you want to know what the biggest wins were, without going through the whole PR:
chineseanddangicalendars specifically, we cached days-in-month information but never used it.