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

Intl.format slow performance when using PlainDate instead of Date #262

Closed
digaus opened this issue Aug 24, 2023 · 9 comments
Closed

Intl.format slow performance when using PlainDate instead of Date #262

digaus opened this issue Aug 24, 2023 · 9 comments

Comments

@digaus
Copy link

digaus commented Aug 24, 2023

When using PlainDate with a cached Intl.DateTimeFormat the performance is 15x worse than using Date.

Is this expected because it is just a polyfill ?

Date:
image

PlainDate:
image

@12wrigja
Copy link
Contributor

Please can you share your benchmarking code?

@justingrant
Copy link
Contributor

justingrant commented Aug 24, 2023

Our main focus with the polyfill so far has been to ensure it's compliant to the Temporal spec. So we haven't spent a lot of time on perf optimizations, other than the most obvious ones like caching our usage of Intl.DateTimeFormat instances which can cost 10+ milliseconds to create.

But if you find an optimization opportunity in the polyfill, PRs are always welcome!

That said, seeing slower perf in this case isn't surprising. The polyfill's Intl.DateTimeFormat implementation is built on top of the native pre-Temporal Intl.DateTimeFormat implementation, which only accepts a Date parameter or milliseconds-since-eopch value which is interchangeable with a Date. So in order to call format, the polyfill needs to convert the Temporal.PlainDate to a milliseconds-since-eopch value.

This conversion is a relatively expensive operation that runs hundreds of lines of JS code and that makes at least one additional call to Intl.DateTimeFormat.prototype.format. (Maybe two, I forget if our DST-handling algorithm requires two of those calls.) You can trace through the call in a debugger to get a sense for what the polyfill has to do to make that conversion. It's a lot!

Note that the Temporal.PlainDate=>Date conversion is actually two steps: first convert Temporal.PlainDate to a Temporal.Instant in the current time zone (this is slow), and then convert the Temporal.Instant into an milliseconds-since-epoch value (this is very fast).

There's also some one-time costs associated with the first use of some Temporal APIs, while the polyfill sets up internal caches (notably a single cached Intl.DateTimeFormat instance that's used for time zone conversions as well as a list of time zone IDs). So the first call that deals with time zones will likely cost a few tens of milliseconds. I think there's other Intl.DateTimeFormat-specific caches too, so if you format a Temporal.PlainDate with a particular polyfilled Intl.DateTimeFormat instance and later format a Temporal.PlainMonthDay, there's a cache-setup cost for each Temporal type.

A native implementation will be a lot, lot faster: likely faster than Intl.DateTimeFormat.prototype.format with a Date argument, because with Temporal.PlainDate there's no need to use a time zone to convert the argument to Year/Month/Day values that will be formatted.

But given the Temporal polyfill's dependency on native pre-Temporal Intl.DateTimeFormat implementation, we're kinda limited in how fast it can be.

If you have a particular, perf-sensitive use case that you want to share more details about, then we can probably recommend a faster way to achieve that use case that avoids expensive Temporal.PlainDate=>Instant conversions.

Here's a few techniques that could work:

  • If you have a few Temporal.PlainDate values that you'll be formatting regularly with different formatting options, then set up a cached map of Temporal.PlainDate=>Temporal.Instant conversions.
  • If you have a few Temporal.PlainDate values that you'll be formatting regularly with different formatting options, then set up a cached map of Temporal.PlainDate=>formatted-string.
  • Format Temporal.Instant instances instead. I suspect if you run the same benchmark with a Temporal.Instant instance, which has the same data model (time since epoch) as Date, then I'd expect Intl.DateTimeFormat.prototype.format(instant) to be only a little bit more expensive (at most 2x) than Intl.DateTimeFormat.prototype.format(date).
  • There are also some use cases where Temporal.ZonedDateTime can be faster, because Intl.DateTimeFormat.prototype.format(zdt.toInstant()) will be very fast. You still may incur conversion costs while creating the ZDT instance, so this will likely only be faster in use cases where the ZDT was created directly from a Temporal.Instant (e.g. instant.toZonedDateTimeISO('UTC')) as opposed to being created from an ISO string.

Thanks for trying out this polyfill!

@digaus
Copy link
Author

digaus commented Aug 25, 2023

#Thank you for the detailed information.

Customers are ordering outside of germany in a different timezone but delivery is happening in germany. So I need to check if date in german timezone is sunday/saturday or if it is a holiday, so that I can disable these days.

Also customer can for example not order for monday if current day (in germany) is a sunday.

Thats why I need the timezone conversions.

Previously I used my own simple Intl cache and converted a Date to the specific timezone. This was a lot faster.

    public setTimezone(timezone: string): void {
        if (timezone !== this.timezone) {
            this.timezone = timezone;
            this.dayIntl = new Intl.DateTimeFormat('en-GB', { timeZone: this.timezone, weekday: 'short' } as Intl.DateTimeFormatOptions);
            this.dateIntl = new Intl.DateTimeFormat('en-GB', { timeZone: this.timezone, dateStyle: 'short' } as Intl.DateTimeFormatOptions);
            this.timeIntl = new Intl.DateTimeFormat('en-GB', { timeZone: this.timezone, timeStyle: 'medium' } as Intl.DateTimeFormatOptions);
        }
    }

    public getDate(date: Date, timeZone?: string): string {
        this.setTimezone(timeZone);
        return this.dateIntl.format(date).split('/').reverse().join('-');
    }

   public getDay(date: Date, timeZone?: string): 'Sun' | 'Mon' | 'Tue' | 'Wed' |  'Thu' | 'Fri'| 'Sat' {
        this.setTimezone(timeZone);
        return this.dayIntl.format(date) as 'Sun' | 'Mon' | 'Tue' | 'Wed' | 'Thu' | 'Fri'| 'Sat';
    }

To speed up the current usage with plainDate I changed:

dayIntl.format(plainDate) -> dayIntl.format(new Date(plainDate.toString())

Is there any drawback doing this?

Speeds (T = Temporal, D = Date):

image

@ptomato
Copy link
Contributor

ptomato commented Aug 25, 2023

For getting the day of the week in zoned time, I would suggest not going through formatting at all! Formatting other than with toString() is basically always going to be slow. Something like this:

function isWeekendInGermany(requestedDeliveryTime: Temporal.ZonedDateTime) {
  const {dayOfWeek} = requestedDeliveryTime.withTimeZone('Europe/Berlin');
  return dayOfWeek === 6 || dayOfWeek === 7;
}

@digaus
Copy link
Author

digaus commented Aug 25, 2023

For getting the day of the week in zoned time, I would suggest not going through formatting at all! Formatting other than with toString() is basically always going to be slow. Something like this:

function isWeekendInGermany(requestedDeliveryTime: Temporal.ZonedDateTime) {
  const {dayOfWeek} = requestedDeliveryTime.withTimeZone('Europe/Berlin');
  return dayOfWeek === 6 || dayOfWeek === 7;
}

Tried that already and it is not significantly faster since the performance loss is from converting to the specific timezone.

In my example PlainDate and Date both get converted to the timezone specific format, but PlainDate is a lot slower.

@ptomato
Copy link
Contributor

ptomato commented Aug 25, 2023

I see, so the bottleneck is the internal TimeZone.prototype.getPlainDateTimeFor call, not actually the formatting? (the formatting also has to convert exact time to plain time via the time zone, so that's what's slowing it down, I guess)

One possible workaround, if you only need the Europe/Berlin time zone functionality, is to create a custom time zone object that implements the DST transitions in Germany, and use that instead of the built-in time zone. (In a real implementation of Temporal, or a polyfill that brings its own time zone data instead of deriving it from Intl, a built-in time zone would always perform better than a custom one, but that doesn't seem to be the case here.)

An example of that is here, although that example has been languishing as an unmerged PR until I've finished some other higher-priority things so it might need some updating. That example is probably more comprehensive than you would need. You could probably cut out the parsing code, for example.

@justingrant
Copy link
Contributor

justingrant commented Aug 25, 2023

Are you originally starting with a date like 2023-08-25 or an exact point in time like 2023-08-25T12:34:56Z?

If the former, then the solution is very easy and fast: don't bother with time zones at all:

pd = Temporal.PlainDate.from('2023-08-25');
console.log(pd.dayOfWeek);
// 5

You can probably shave a few tens of microseconds by using a cached calendar:

isoCalendar = Temporal.Calendar.from('iso8601');
pd = Temporal.PlainDate.from('2023-08-25');
console.log(isoCalendar.dayOfWeek(pd));
// 5

But if you're starting with an exact instant (e.g. a Date or a Temporal.Instant) and need to figure out the date of that instant in a particular time zone, then any spec-compliant Temporal polyfill is pretty much guaranteed to be much slower than a single call to Intl.DateTimeFormat, because a lot of JS code needs to be run to do the conversion. You can minimize that code by using a cached time zone and a cached calendar:

const tzBerlin = Temporal.TimeZone.from('Europe/Berlin');
const isoCalendar = Temporal.Calendar.from('iso8601');
function isWeekendInGermany(instant) {
  const dt = tzBerlin.getPlainDateTimeFor(instant);
  const dayOfWeek = isoCalendar.dayOfWeek(dt);
  return dayOfWeek === 6 || dayOfWeek === 7;
}
isWeekendInGermany(new Date().toTemporalInstant());
// => false
isWeekendInGermany(new Date().toTemporalInstant().add({hours: 24}));
// => true

If that's still not fast enough for your use case, then the custom time zone that @ptomato recommends seems like your best bet. Although at that point, given that you already have a workable solution using Intl.DateTimeFormat, then I'd probably just stick with that one. Polyfills are, by definition, going to be slower than native solutions.

All that said, a difference of 0.1-0.2 milliseconds is fairly small! Relative to all the other things that a system is doing, does the perf of this call significantly impact your app's overall performance?

@digaus
Copy link
Author

digaus commented Aug 25, 2023

@justingrant

Oh wow I am so stupid ... obviously the 25.08.2023 is always a friday regardless of the timezone🙈

Time to take a break^^

It had some impact because I use it on a filter on a DateAdapter for Angular Material.

When opening the year view, the days get checked to disable years if needed.

But I also have some other additional performance issue here which I need to investigate.

@justingrant
Copy link
Contributor

Cool. I'm going to close this issue because it sounds like the Temporal part is resolved. This was an interesting discussion. Thanks!

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

No branches or pull requests

4 participants