Perf: Summary of all perf changes. DO NOT MERGE. #1584
Closed
Conversation
This file contains 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
We validate the zone in the constructor anyway, but the tests missed a branch where we attempt to use an invalid zone to compute an offset, which will result in an exception. Zone#isValid is calculated at creation time (and checked in DateTime constructor already) so this will not degrade performance in the happy case.
All calls to the datetime constructor that pass an offset (o), have just computed that offset (quickDT, fromObject) except for clone(). Clone passes an "old" option to determine whether previous offset can be reused. If we have config.o, but config.old is not set, then we can use o without computing it. This saves an expensive call to zone.offset that duplicates work that was done immediately prior.
Previously we guessed by calling zone.offset(tsNow) on every construction of a date from date parts. This caches that result instead, saving a call to offset
Profiles reveal that a substantial portion of fromFormat time is spent in digitsRegex. It is only called from unitsForToken and it is always called with 11 suffixes to match different numbers of digits. There are 21 numbering systems, so a fully expanded cache would hold 11*21 = 231 regexes. Benchmarks show about a 1.5x improvement on DateTime.fromFormat
This allows constructing a parser for a locale/format and reusing it when parsing dates. Without this, DateTime.fromFormat constructs a new parser on every call. When parsing large amounts of date strings, this gets rather slow. In benchmarks, this speeds up parsing by 4.4x
The time zone offset of a date can be computed on platforms that support it (anything that's not super ancient) by using Intl.DateTimeFormat.formatToParts with en-US to output an array of the date components. For legacy reasons, you can also generate a date string using Intl.DateTimeFormat.format which can be parsed into an array using regexes. The string/regex approach (hackyOffset) is way faster (2-4x), but much more susceptible to weird client configurations. This detects whether hackyOffset is able to parse a known date correctly, and uses it if it does.
Husky adds git hooks into the repo. We want the dev experience of
$ git clone remote/luxon
$ npm install # installs husky hooks
$ ...develop develop develop...
$ git commit
_lint runs, everything works_
We also want to build luxon when we call npm pack or npm publish.
We also want to build luxon when it is installed somewhere else as a git
dependency
(https://docs.npmjs.com/cli/v10/configuring-npm/package-json#git-urls-as-dependencies)
Git dependencies work by running `npm install` from GitFetcher and then
depend on DirFetcher running `npm prepare` (but only prepare, not
prepack or postpack)
This change moves `husky install` to postinstall (which will,
unfortunately, still pointlessly run when we install luxon as a git
dependency). It moves the buildAll to prepare from prepack so that the
build happens when installed as a git dependency. This should preserve
existing behavior while enabling installation from git.
When installing from git and/or in CI environments, the husky command is
sometimes not available in postinstall or is available but bails out
when not run in the context of a git repo.
…jamin--integ-all-changes
…benjamin--integ-all-changes
…integ-all-changes
|
/easycla |
|
@schleyfox Hey Ben -- these are great, and I really like the effort and depth here. I've merged most of them. The remaining items are:
|
|
@icambron thanks! for #1579 I'll get that fixed shortly for #1580 I'll benchmark that and respond, should be straightforward. for #1583 totally! It's been on my backlog to docker up an example to see how the npm hooks behave under different conditions (I'm quite confused), but I haven't had a chance yet. |
|
Sorry for taking so long to release these. But now that they're out, I think we can close this uber PR |
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.
We have worked to improve luxon performance for our use case of parsing/formatting hundreds of thousands of dates at a time. These are represented as a series of independent PRs
perf
bug fix/quality of life
While each perf-related PR improves perf on its own, we see the most benefit when they are all combined (especially for time zone offset handling).
Benchmark Comparison (
name | before | after | after/before):