tc39 / proposal-temporal

Provides standard objects and functions for working with dates and times.
https://tc39.es/proposal-temporal/docs/
Other
3.33k stars 150 forks source link

Performance implications of ES.ToTemporalDateFields #1241

Closed justingrant closed 7 months ago

justingrant commented 3 years ago

One of the learnings from building non-ISO calendars is that sometimes calendrical calculations can be expensive because they may rely on external libraries which may not have ideal performance characteristics. I saw this firsthand because my prototype non-ISO implementation makes repeated calls to DateTimeFormat.formatToParts, and for some calendars these calls can take more than 0.2 milliseconds. 200 microseconds isn't slow, but if you stack up multiple of those calls then you're starting to talk about significant delays, especially in use cases that access calendar data in a loop, e.g. when displaying a year of dates in a GUI.

One cause of non-ISO calendar in efficiency is ES.ToTemporalDateFields (and similar methods) where the calendar is called once for a cheap call to fields() but then again for 3+ maybe-expensive getter calls: .year(), .month(), .day(), and then any additional fields added by the calendar. For trivial calendars like Japanese that mostly match ISO this isn't a big deal, but for calendars that need to calculate the full calendar date for every ISO date, all 3+ of those getter functions will likely have the same implementation: 1) convert the ISO date to a calendar date (may be costly) 2) return one property

In cases where end-users are calling a Temporal object's field getters one at a time, this 3x+ cost is unavoidable. But for calls to getFields(), or for internal usage of ES.ToTemporalDateFields (e.g. in ES.ToTemporalDate which is used all over the place inside the polyfill) this cost seems unnecessary.

One way to address this would be to extend the Calendar protocol with a fieldValues(date, fieldNameArray) method that would allow the calendar to return multiple calendar field values in one call.

A partial solution which wouldn't require an API change could be to revise ES.ToTemporalDate (and similar methods like ES.ToTemporalDateTime) to be smarter about handling inputs that are Temporal objects. Currently these methods optimize the input if there's an exact match (e.g. ES.ToTemporalDate will return its input as-is if the input is a Temporal.Date) but not if there's an imperfect match (e.g. a Temporal.DateTime). Imperfect matches could be detected and transformed (using ISO fields and/or non-observable internal code) into perfect matches which would prevent observable, potentially-expensive field access. This is only a partial solution; it'd make some calls like with() cheaper but it wouldn't make getFields() any cheaper.

Louis-Aime commented 3 years ago

One way to address this would be to extend the Calendar protocol with a fieldValues(date, fieldNameArray) method that would allow the calendar to return multiple calendar field values in one call.

As a calendar's author, I find this is wise. In fact, I developed something very close for week fields data.

ptomato commented 3 years ago

I've been expecting that two things will mitigate this cost:

justingrant commented 3 years ago

@ptomato - These are good suggestions. re: the WeakMap option, I've been confused about how to apply WeakMap caching in a case where the expensive data is shared between multiple Temporal objects.

For example, some calendars (e.g. Chinese, Islamic) require dynamically (and, in my prototype, expensively) calculating the lengths of every month in a particular calendar year in order to perform some operations. But the result of those calculations can be shared for all Temporal instances that share a particular calendar year. Given that many applications may construct thousands or more Temporal objects in the same calendar year (often the current year!), what's a good way to share that data among all those objects? I know the pre-WeakMap answer is just to build your own simple cache or use an existing cache library, but is there a better way to do it with WeakMap?

BTW, one optimization I think we could add to the polyfill would be something like the one below. I'm seeing non-trivial performance gains from doing this in my non-ISO proof-of-concept PR. Are there any downsides to doing this?

  ToTemporalDate: (item, constructor, overflow = 'constrain') => {
    if (ES.Type(item) === 'Object') {
      if (ES.IsTemporalDate(item)) return item;
      // added: optimize for PDT and ZDT too
      if (ES.IsTemporalDateTime(item)) return ES.TemporalDateTimeToDate(item);
      if (ES.IsTemporalZonedDateTime(item)) {
        const dt = ES.GetTemporalDateTimeFor(GetSlot(item, TIME_ZONE), GetSlot(item, INSTANT), GetSlot(item, CALENDAR));
        return ES.TemporalDateTimeToDate(dt);
      }
ptomato commented 3 years ago

For example, some calendars (e.g. Chinese, Islamic) require dynamically (and, in my prototype, expensively) calculating the lengths of every month in a particular calendar year in order to perform some operations. But the result of those calculations can be shared for all Temporal instances that share a particular calendar year. Given that many applications may construct thousands or more Temporal objects in the same calendar year (often the current year!), what's a good way to share that data among all those objects? I know the pre-WeakMap answer is just to build your own simple cache or use an existing cache library, but is there a better way to do it with WeakMap?

WeakMap is only useful if the keys are garbage-collectable values (like Temporal objects). If the keys are calendar years, then I'd recommend using a regular Map, or some other existing thing if fancy expiration behaviour is needed.

BTW, one optimization I think we could add to the polyfill would be something like the one below. I'm seeing non-trivial performance gains from doing this in my non-ISO proof-of-concept PR. Are there any downsides to doing this?

As long as the change isn't observable from userland, then that seems fine to me. I can't tell off the top of my head whether that's the case or not.

Louis-Aime commented 3 years ago

Don't forget to let calendars' authors know whether or not they should care for code optimisation, and how they should do.

justingrant commented 3 years ago

@Louis-Aime - How much to optimize depends on your performance goals and dependencies. For #1245 optimization was important because the implementation I'm using relies on many, relatively-slow calls to Intl.DateTimeFormat. Some methods like dateUntil and dateAdd were taking a few hundred milliseconds when doing arithmetic for 12-month durations, which IMHO is too slow, even for a proof-of-concept, non-production implementation, because you can't even prototype an API that slows your UI to a crawl.

If your calendars are doing simple arithmetic calculations, it's unlikely that much optimization would be needed, if any. So I'd suggest you do some basic performance benchmarking on your calendars, and if methods take longer than 10-20ms, then consider trying to speed them up.

One easy optimization approach is to use a WeakMap to cache extra data about Temporal objects, like calendar fields or the results of expensive calculations. For an example, take at the OneObjectCache class in calendar.mjs of #1245. You can also look at intl.mjs in the /polyfill/test folder of that PR, which has some commented-out performance measurements for common use cases.

ptomato commented 3 years ago

What is needed to resolve this issue?

justingrant commented 3 years ago

I'm not sure there's anything actionable for us. It might be worth adding a paragraph or two in the custom-timezone docs explaining how to optimize using WeakMap. Also, #1245 includes the optimizations noted above that I think were helpful to speed things up somewhat.

ptomato commented 1 year ago

With the proposed solution for #1808 we've made sure this won't be a performance problem for built-in calendars, so this is only a question for userland calendars. Should this be documented somewhere or can we close the issue?

ptomato commented 7 months ago

Going to close this. I'd consider it not our responsibility to prescribe what custom calendar implementations should optimize.