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

Performance implications of ES.ToTemporalDateFields #1241

Closed
justingrant opened this issue Jan 4, 2021 · 10 comments
Closed

Performance implications of ES.ToTemporalDateFields #1241

justingrant opened this issue Jan 4, 2021 · 10 comments
Labels
documentation Additions to documentation
Milestone

Comments

@justingrant
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

ptomato commented Jan 5, 2021

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

  • engines will optimize out these calls for built-in calendars, because they won't be observable if the calendar is built-in and Temporal.Calendar.prototype hasn't been changed.
  • user calendars should cache expensive calculations using a WeakMap of Temporal objects.

@justingrant
Copy link
Collaborator Author

@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
Copy link
Collaborator

ptomato commented Jan 5, 2021

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
Copy link

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

@justingrant
Copy link
Collaborator Author

@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
Copy link
Collaborator

ptomato commented Jan 15, 2021

What is needed to resolve this issue?

@justingrant
Copy link
Collaborator Author

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 ptomato added documentation Additions to documentation and removed question labels Feb 18, 2021
@ptomato ptomato added this to the Next milestone Feb 18, 2021
@ptomato
Copy link
Collaborator

ptomato commented Dec 8, 2022

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
Copy link
Collaborator

ptomato commented Mar 26, 2024

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

@ptomato ptomato closed this as completed Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants