Allow caching of ZonedDateTime's ISO datetime values

[arshaw]

Hello! While implementing temporal-polyfill, I noticed from this test that I have fewer calls to a TimeZoneProtocol's getOffsetNanosecondsFor for a given ZonedDateTime. In that test, the ZonedDateTime (instance) queries the TimeZone for its offset during each call to withPlainDate. It needs this in order to compute its hour/minute/second/etc values for merging with the given year/month/day. It could potentially cache its offset (and potentially the resulting ISO-y/m/d/h/m/s/etc values) between calls but it does not.

I would like to advocate for ZonedDateTime's ability to cache its ISO-y/m/d/h/m/s/etc values. This would result in fewer calls to getOffsetNanosecondsFor and better performance for computing its calendar-based y/m/d/h/m/s/etc values.

Normally, I would advocate for the TimeZone (and Calendar) objects doing their own caching (with a WeakMap lookup) as an implementation detail. That's what I'm doing in other situations. However, that does not help in this case:

• zdt calls TimeZone::getOffsetNanosecondsFor(zdt[internalInstant])
  • the TimeZone can recognize this is a repeat call for the zdt (using WeakMap) and give a fast result
  • the result is a offset number
• zdt needs to compute its ISO-y/m/d/h/m/s/etc values
  • it add zdt.epochNanoseconds to the offset number which it can then use to compute ISO values (aka PlainDateTime)
  • it cannot cache the result because the input is a number, which does not work for WeakMaps
  • we would not want to use a Map instead for fear of it growing out of control
• zdt needs to compute its calendar-based y/m/d/h/m/s/etc values
  • it calls Calendar::year()/month()/day()/etc with the ISO values (aka PlainDateTime) it previously computed
  • it cannot cache the result because the input is a fresh PlainDateTime each time, and results in a WeakMap cache miss

If we allowed ZonedDateTime to cache a stable reference to its ISO-y/m/d/h/m/s/etc values, it would enable cache hits for Calendar::year()/month()/day()/etc calls.

[ptomato]

I seem to remember we did consider storing the PlainDateTime in an internal slot but decided against it for reasons I can't remember and can't find at the moment. Possibly a similar weirdness to what I described in #1294 (comment) although that is now less of a concern since we have string-IDs for builtin time zones and calendars.

Some things that would be good to think through: Would the GetOffsetNanosecondsFor call happen at ZonedDateTime construction time? Or at the first moment it's needed? The latter might be difficult to justify to TC39 because it makes ZonedDateTime stateful, which I think will probably raise some eyebrows. On the other hand, if the call happens at construction time, what happens if it fails? Does it throw in the constructor, or give you an incomplete ZonedDateTime object that you can only do exact-time operations on, or try the call again when you do an operation that requires the offset (statefulness again...)?

[arshaw]

Thanks for the thoughtful response as always @ptomato.

If making ZonedDateTime stateful is a no-go, the constructor seems like the only place. Thus, TimeZone::getOffsetNanosecondsFor would need to be called from the constructor, with potential error.

This would be inconsistent with an object like PlainDate, which does NOT consult its Calendar until needed, and thus won't error upon construction.

But I'd argue that ZonedDateTime is much MORE dependent on its TimeZone than PlainDateTime is on its Calendar, and thus it doesn't make as much sense to try to allow an unviable ZonedDateTime to be created. For example, whereas PlainDate can call .toString() and .getISOFields() for serialization without querying a potentially faulty Calendar, ZonedDateTime MUST consult its TimeZone for these two operations.

I'd opt for erroring in ZonedDateTime's constructor.

I also considered forcing ZonedDateTime to use TimeZone::getPlainDateTimeFor instead of getOffsetNanosecondsFor if available, and have that function return a stable-ish object reference (not literally same object each time, but some hidden slot that could be tracked from a memory management perspective) but it seems too complicated from an implementation perspective.

Aside from when this internal PlainDateTime would be created, do you acknowledge the problem and need for a way to make calls like ZonedDateTime::day cacheable?

[ptomato]

Aside from when this internal PlainDateTime would be created, do you acknowledge the problem and need for a way to make calls like ZonedDateTime::day cacheable?

Sure, I see the problem, but I don't think we agree on the severity of the need. Custom time zones and calendars are a niche use case, so fundamentally it's OK with me if they're slow. This problem is nonexistent if the ZonedDateTime uses a builtin calendar and time zone, because you can compute and cache the PlainDateTime at construction time, without any danger of any of the calls throwing, and it's all unobservable.

C++ implementations would also be able to write their own private data structures for caching if they chose to optimize the custom path. For example, Firefox has an internal data structure js::ValueWeakMap, which is a weak map which allows any JS value, including primitives, as the key and value types.

I believe optimizing the custom path in JS is also possible, if a bit more complicated. I was trying to think how I'd do it. You could have GetOffsetNanosecondsFor return a Number wrapper object internally, which you can use as the key in a WeakMap, and pass that to GetISOPartsFromEpoch. Then GetISOPartsFromEpoch could have a cache mapping offset nanoseconds object to ISO fields object (e.g. { isoYear: 2024, isoMonth: 3, ...etc... } ), and stash the ISO fields object in a private slot of PlainDateTime (like the hidden slot you suggested). The calendar methods could then get the ISO fields object from that private slot, and use that as a key in their own caches.

[arshaw]

@ptomato, it's really good to know browser implementors can implement caching without modifications to the spec (a la js::ValueWeakMap). That takes away 90% of my concern.

Thanks for your suggestions about how to make this work in JS. Luckily I don't much care about the speed for custom calendars either, as long as the native ones are fast and the code is DRY. I'll likely just pre-compute the ISO-fields in ZonedDateTime's constructor if the calendar is native. When the ISO fields are needed I'll return the precomputed ones if present or compute fresh each time for custom calendars. Won't change the code size all that much. Thanks for the inspiration!