js-temporal / temporal-polyfill

Polyfill for Temporal (under construction)
ISC License
529 stars 28 forks source link

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

Closed digaus closed 1 year ago

digaus commented 1 year ago

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 commented 1 year ago

Please can you share your benchmarking code?

justingrant commented 1 year ago

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:

Thanks for trying out this polyfill!

digaus commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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