Closed moh-incom closed 7 months ago
The committers listed above are authorized under a signed CLA.
@moh-incom This seems reasonable, but would be nice if the benchmark was included in the PR (see the benchmarks dir)
I was unaware of the benchmarks
directory. Are there any instructions on running the benchmarks? The straight-forward guess npm run benchmark
doesn't work for me (gives me a cryptic ESM/CommonJS incompatibility error)
From the looks of it, the existing benchmarks DateTime.fromFormat
and DateTime.fromFormat with zone
should already cover what my benchmark does.
I pushed up a fix for npm run benchmark
I found out a test from the test suite was failing because I was caching based on the Locale.locale
field which does not contain the numbering system. I now cache on Locale.intl
instead. This should cover all the problematic cases.
I got the benchmarks running after you fixed them. In addition, I added some more to more easily test the impact of my optimizations.
Even though it might be a bit outside the scope of this PR, I also added similar caching functionality to the Locale.isEnglish
function. This speeds up DateTime.fromFormat
incredibly when run with a non-English locale, which is also covered in one of the new benchmarks. If you prefer, I can move it to a different PR.
Any progress on approving this or #1581? I do not have a strong preference as to which of the two is used, but I could really use the speedup soon
Closing this in favor of #1581. Thanks for your work on this!
The body of unitForToken creates a lot of objects, including a bunch of
RegExp
s. When parsing a lot of strings usingDateTime.fromFormat
in the same locale, these objects are created from scratch every time even though they are identical each time. This simple PR caches the resultingunitate
function for each locale on first use so these objects are only created once per locale.I have carried out a simple benchmark using the below code:
My measurements indicate roughly a 2x speed-up on Node 18.18.0. This might result in a low to negligible increase in memory usage due to the few cached functions.
Edit
Benchmark results before and after:
Before (Luxon 3.4.4)
After