size-suggestion: Should custom calendars eagerly calculate all fields at construction time?

[justingrant]

Presently, Temporal getters for calendar fields like era, year, monthCode, day, etc. will call into a custom calendar each time the getter is executed. This is inefficient.

An alternative approach is to avoid calling into Calendar during the getter and instead only call the Calendar in the constructor, using a single Calendar.prototype.calculate() function. This function would return an ordinary object with calculated values for getters. In addition to enabling other optimizations and reducing user calls, this would remove the need for 14 functions:

• Temporal.Calendar.prototype.dayOfWeek ( temporalDateLike )
• Temporal.Calendar.prototype.dayOfYear ( temporalDateLike )
• Temporal.Calendar.prototype.daysInMonth ( temporalDateLike )
• Temporal.Calendar.prototype.daysInWeek ( temporalDateLike )
• Temporal.Calendar.prototype.daysInYear ( temporalDateLike )
• Temporal.Calendar.prototype.day ( temporalDateLike )
• Temporal.Calendar.prototype.era ( temporalDateLike )
• Temporal.Calendar.prototype.eraYear ( temporalDateLike )
• Temporal.Calendar.prototype.inLeapYear ( temporalDateLike )
• Temporal.Calendar.prototype.monthCode ( temporalDateLike )
• Temporal.Calendar.prototype.monthsInYear ( temporalDateLike )
• Temporal.Calendar.prototype.month ( temporalDateLike )
• Temporal.Calendar.prototype.weekOfYear ( temporalDateLike )
• Temporal.Calendar.prototype.yearOfWeek ( temporalDateLike )
• Temporal.Calendar.prototype.year ( temporalDateLike )

This issue is adapted from suggestions made by @FrankYFTang to help reduce the size of Temporal to address concerns from implementers (especially Android and Apple Watch) that Temporal's install size and/or in-memory size is too large.

From an initial quick chat with a few of the Temporal champions, we think this is an interesting suggestion!

I've got a few question about this solution. @FrankYFTang, feel free to chime in with answers if you have them:

• Where would the object returned by calculate be stored? In a new internal slot on Plain* types and ZDT?
• Would that slot be empty for built-in calendars? How much would it increase RAM requirements for the vast majority of objects with built-in calendars?
• Would this new solution support calendar-specific fields? (I'm not sure the current solution supports calendar-specific fields either, but I know that this case is something that @sffc has talked a lot about!)
• Would Temporal shallow-clone the object returned by calculate, so we're not holding onto an object that could be mutated later?
• Would there still be a need for getISOFields() methods with this approach?
• Would implementations really need to store a JS object with all the fields? Or could they optimize storage to leverage the fact that most fields don't require 8-byte numbers? For example, calendars could be constrained to U8 for day, month, monthCode, etc.

To set expectations, a change of this magnitude, especially if there is consensus that this is a better way to implement custom calendars than the current design, is probably beyond what we can accommodate in Temporal V1. It's just too large and would probably push out our implementation timeline by over a year. So the best course of action, if we feel that this is the right custom-calendar solution, may be to ship Temporal V1 with built-in calendars only (see #2826) so we're not locked into a sub-optimal design, and then carefully design this new custom-calendar solution for a follow-up proposal.

This is analogous to the plan for custom time zones after #2826, which is that they'd be better implemented declaratively (ideally based on an existing standard like JSCalendar) in a follow-up proposal after Temporal V1, rather than a sub-optimal, user-code-required design in V1.

[anba]

Additional questions:

1. It's unclear to me which arguments will be passed to Calendar.prototype.calculate()? Passing partially constructed objects to user-code seems bad, because it's unclear what should happen when a partially constructed object escapes. To avoid passing partially constructed objects, you'd need to pass the individual date-time fields, i.e. Calendar.prototype.calculate(isoYear, isoMonth, isoDay), right?
2. How/When should type validation happen? Are all calendar fields extracted from the object returned by Calendar.prototype.calculate() and will they get validated directly within the constructor?
3. Right now it's possible to avoid (eagerly) creating GC objects in some cases, for example in AddDaysToZonedDateTime it's not necessary to create a Temporal.PlainDateTime object for dateTimeResult when timeZoneRec calls the built-in Temporal.TimeZone.prototype.getPossibleInstantsFor function (i.e. when timeZoneRec is either a string time zone or an instance of Temporal.TimeZone with all built-in functions still intact). There are many additional places in the spec where GC allocations can be avoided or at least be deferred. It'd be good to know if this optimisation is still possible when the constructor may call arbitrary user-code.

[ptomato]

@anba Good questions,

1. Yes, ISO year/month/day seem like good arguments to pass. I'd probably suggest naming the method isoDateToCalendarDate rather than calculate to better indicate what it does.
2. I'd say yes, validation ASAP so that there are fewer failure points throughout the other operations.
3. This is a very good point and unfortunately has the potential to scuttle this idea. I'm not sure how I'd determine whether that's the case other than making a test implementation, though.

Currently, I'm neutral to mildly positive on this.

[FrankYFTang]

My origional proposal could be found in https://docs.google.com/document/d/16Rd43XB8z6iLC2c9AfF--unDFg_t476tI1PFe7PbbcM

[sffc]

I believe that these were originally proposed as separate functions because it was believed that certain calendars could have more efficient algorithms for computing some fields than others, but now that I've seen the implementation in ICU4X, I no longer believe that is the case, so I am fine with requiring calendars to have everything computed in one go.

[justingrant]

There are many additional places in the spec where GC allocations can be avoided or at least be deferred. It'd be good to know if this optimisation is still possible when the constructor may call arbitrary user-code.

@anba could you explain this concern a bit more? The construction-time call to user code would only happen in the case of custom time zones. Built-in (string ID) time zones would continue to never call user code in the constructor or at any other time. Does that address your concern?

FWIW, I'm not very concerned about optimization of the custom calendar case. If the user opts into a custom calendar, they're already in a sub-optimal case. Losing incremental optimization opportunities in this case seems like a small price to pay for a simpler overall custom-calendar solution that may allow other optimizations.

Finally, getters are much more commonly used than more complex operations like arithmetic. So any solution that made custom-calendar getters more efficient at the expense of arithmetic methods would seem to be a good tradeoff.

[sffc]

I think it would be compelling from a function count point of view if this proposal were coupled with adding this data as own properties of the Temporal objects. Creating a Temporal object would involve calling .calculate() and then just Object.assigning its fields to the Temporal object.

This is just an idea I had and I haven't discussed it with implementers to see if the function count reduction would outweigh the additional cost of storing owned properties.

[justingrant]

Given how many properties some Temporal objects have, using data properties for all properties would seem to blow up the runtime RAM requirements for objects. For example, today's ZonedDateTime type requires storing 75 bits of epoch-nanoseconds, a time zone (10 bits for built-in), and a calendar (4 bits for built-in). For objects with built-in time zones and calendars, that's potentially only 12 bytes. If instead we wanted to materialize all its data properties, we're looking at 200+ bytes assuming 8 bytes per property.

Or am I misunderstanding what you're proposing?

[anba]

@anba could you explain this concern a bit more? The construction-time call to user code would only happen in the case of custom time zones. Built-in (string ID) time zones would continue to never call user code in the constructor or at any other time. Does that address your concern?

AddDaysToZonedDateTime [1] can be implemented using a stack class [2, 3]. GetInstantFor [4] passes this stack class to GetPossibleInstantsFor [5], which only needs to create an actual Temporal.PlainDateTime GC-object in its slow path [6].

How much extra overhead for custom calendars will be necessary to when CreateTemporalDateTime can call into user-code? Do I just need to change Rooted<PlainDateTimeWithCalendar> to Rooted<mozilla::Variant<PlainDateTimeWithCalendar, PlainDateTimeObject*>> [7] and then handle the extra state everywhere? For example the possible states in GetPossibleInstantsFor change from:

1. String time zone.
2. Temporal.TimeZone using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor with Array iteration still intact.
3. Temporal.TimeZone using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor but Array iteration not intact.
4. Temporal.TimeZone with overridden getPossibleInstantsFor or some custom time zone object.

to double as much states:

1. String time zone and using PlainDateTimeWithCalendar.
2. String time zone and using PlainDateTimeObject*.
3. Temporal.TimeZone using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor with Array iteration still intact and using PlainDateTimeWithCalendar.
4. Temporal.TimeZone using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor with Array iteration still intact and using PlainDateTimeObject*.
5. Temporal.TimeZone using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor but Array iteration not intact and using PlainDateTimeWithCalendar.
6. Temporal.TimeZone using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor but Array iteration not intact and using PlainDateTimeObject*.
7. Temporal.TimeZone with overridden getPossibleInstantsFor or some custom time zone object and using PlainDateTimeWithCalendar.
8. Temporal.TimeZone with overridden getPossibleInstantsFor or some custom time zone object and using PlainDateTimeObject*.

And when tracing Rooted<mozilla::Variant<PlainDateTimeWithCalendar, PlainDateTimeObject*>> there's also some extra overhead to select the active part of the variant.

For comparison when neither custom calendar nor custom calendars are available, then GetPossibleInstantsFor can directly invoke the built-in functionality without any additional run-time checks. And Rooted<PlainDateTimeWithCalendar> can be replaced with a non-rooted type, because PlainDateTimeWithCalendar no longer holds any GC things.

Additionally I'd need to check if some reordered steps are still valid, because reordering steps is no longer valid when it's detectable through user-code.

[1] AddDaysToZonedDateTime: https://searchfox.org/mozilla-central/rev/c84d3db8d7d6503b1208a0378db640095e106355/js/src/builtin/temporal/ZonedDateTime.cpp#645-649
[2] PlainDateTimeWithCalendar: https://searchfox.org/mozilla-central/rev/c84d3db8d7d6503b1208a0378db640095e106355/js/src/builtin/temporal/PlainDateTime.h#183-211
[3] CreateTemporalDateTime: https://searchfox.org/mozilla-central/rev/c84d3db8d7d6503b1208a0378db640095e106355/js/src/builtin/temporal/PlainDateTime.cpp#405-427
[4] GetInstantFor: https://searchfox.org/mozilla-central/rev/c84d3db8d7d6503b1208a0378db640095e106355/js/src/builtin/temporal/TimeZone.cpp#2047-2083
[5] GetPossibleInstantsFor: https://searchfox.org/mozilla-central/rev/c84d3db8d7d6503b1208a0378db640095e106355/js/src/builtin/temporal/TimeZone.cpp#1731-1768
[6] GetPossibleInstantsFor, slow path: https://searchfox.org/mozilla-central/rev/c84d3db8d7d6503b1208a0378db640095e106355/js/src/builtin/temporal/TimeZone.cpp#1759-1767
[7] Rooted to trace GC-things, mozilla::Variant is like std::variant.

[justingrant]

Meeting 2024-05-23: In #2854, we're proposing removing the Temporal.Calendar class and protocol, so this issue doesn't apply to Temporal V1.

However, there is wide interest in creating a follow-up proposal to enable custom calendars, and the ideas proposed in this issue seem like much simpler and efficient solution than the current custom calendar design. Anyone working on a future custom calendar proposal should consider this kind of all-at-once calculation.

[ptomato]

With #2854 being approved, this issue no longer applies.