Closed justingrant closed 4 months ago
Additional questions:
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?Calendar.prototype.calculate()
and will they get validated directly within the constructor? 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. @anba Good questions,
isoDateToCalendarDate
rather than calculate
to better indicate what it does.Currently, I'm neutral to mildly positive on this.
My origional proposal could be found in https://docs.google.com/document/d/16Rd43XB8z6iLC2c9AfF--unDFg_t476tI1PFe7PbbcM
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.
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.
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.assign
ing 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.
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 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:
Temporal.TimeZone
using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor
with Array iteration still intact.Temporal.TimeZone
using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor
but Array iteration not intact.Temporal.TimeZone
with overridden getPossibleInstantsFor
or some custom time zone object.to double as much states:
PlainDateTimeWithCalendar
.PlainDateTimeObject*
.Temporal.TimeZone
using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor
with Array iteration still intact and using PlainDateTimeWithCalendar
.Temporal.TimeZone
using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor
with Array iteration still intact and using PlainDateTimeObject*
.Temporal.TimeZone
using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor
but Array iteration not intact and using PlainDateTimeWithCalendar
.Temporal.TimeZone
using built-in Temporal.TimeZone.prototype.getPossibleInstantsFor
but Array iteration not intact and using PlainDateTimeObject*
.Temporal.TimeZone
with overridden getPossibleInstantsFor
or some custom time zone object and using PlainDateTimeWithCalendar
.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
.
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.
With #2854 being approved, this issue no longer applies.
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: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:
calculate
be stored? In a new internal slot on Plain* types and ZDT?calculate
, so we're not holding onto an object that could be mutated later?getISOFields()
methods with this approach?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.