Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve IANAZone.offset performance #1503

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mohd-akram
Copy link
Contributor

@mohd-akram mohd-akram commented Sep 8, 2023

This is more than 2.5x faster in my testing. Example benchmark script:

import { setTimeout } from 'timers/promises';

import { DateTime } from './src/luxon.js';

// Warm up
for (let i = 0; i < 10; i++) {
  DateTime.fromJSDate(new Date(), { zone: 'America/New_York' });
}

await setTimeout(200);

console.time('run');
for (let i = 0; i < 10000; i++) {
  DateTime.fromJSDate(new Date(), { zone: 'America/New_York' });
}
console.timeEnd('run');

@mohd-akram mohd-akram force-pushed the improve-offset-perf branch 3 times, most recently from d8f8f69 to 203a219 Compare September 9, 2023 00:04
@diesieben07
Copy link
Collaborator

The problem with this is that the output format of longOffset is not described in the specification, so there is no guarantee that it will work like this in all engines or even that it will keep working in the same way.

@mohd-akram
Copy link
Contributor Author

@diesieben07 This problem exists for hackyOffset and partsOffset as well. Luxon generally makes certain assumptions about the engine, such as it having Intl.DateTimeFormat, an en-US locale, and a format that looks a certain way. It is recommended for engines to use the CLDR data, and in the case of long offset formatting it is fairly rigid.

@diesieben07
Copy link
Collaborator

hackyOffset is only used if formatToParts is not supported, which is pretty much only in ancient runtimes. I don't see how partsOffset is susceptible to the same issue, as it looks at each part individually and only parses numbers.
At the very least we should check that the expected format is returned by longOffset and only set hasLongOffset if that is so.

@mohd-akram
Copy link
Contributor Author

hackyOffset is only used if formatToParts is not supported, which is pretty much only in ancient runtimes. I don't see how partsOffset is susceptible to the same issue, as it looks at each part individually and only parses numbers.

partsOffset is certainly less susceptible, perhaps the only part where it might fail is the BC check.

At the very least we should check that the expected format is returned by longOffset and only set hasLongOffset if that is so.

I've added this. I also extracted the offset calculations out of the method, and made each function get its own DTF since they rely on different formats. The check is expensive, that's why I had to do it inside the offset method as otherwise importing the library is slower (the Intl.DateTimeFormat constructor is painfully slow).

@icambron
Copy link
Member

In general, I like this kind of optimization, as long as the feature detection works well. For partsOffset and even Intl support in general, we went through years of feature detection and updates to the support matrix as support stabilized and we bumped the minimum browser version up to support them. And then we left hackyOffset in there as a fallback to formatToParts anyway. My point isn't that we shouldn't do this -- 2.x is nothing to sneeze at -- but that we need to do it with care.

I'm also somewhat hoping there's a better way to detect support for longOffset, but I don't have any bright ideas there.

@schleyfox
Copy link
Contributor

I found this issue while investigating a performance issue in offset that impacted a use case of parsing a few hundred thousand dates at a time.

I noticed that hackyOffset was substantially faster than partsOffset (about 2.5x on node 18). I wrote a benchmark to compare and got even more dramatic results in browser

Test case name Result
hackyOffset hackyOffset x 718,239 ops/sec ±0.67% (68 runs sampled)
partsOffset partsOffset x 208,390 ops/sec ±0.21% (69 runs sampled)
dtf.formatToParts dtf.formatToParts x 298,653 ops/sec ±0.52% (68 runs sampled)
dtf.format dtf.format x 822,318 ops/sec ±0.18% (60 runs sampled)

It seems like there might be good reasons not to enable this in the browser, but I am very tempted to use this approach in node where I control the environment...

@mohd-akram
Copy link
Contributor Author

Any update on this? I've been using it in production since and it seems to be working well. Note that this is even faster than hackyOffset (1.4x as fast in my testing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants