delegate review: why are "Temporal-like" objects supported at all? #1428
Comments
|
It's important to support construction of Temporal instances from options bags of named arguments, so this is why it is important to support case 3. For the question of when internal slots vs properties should be accessed, we had some pretty extensive discussion on this topic in #231 . I became convinced, in that thread, that being careful to use internal slots whenever possible adds much more complexity than is saved. One way to think about |
|
That makes sense, but I’d generally expect a fast unobservable path for actual instances.#231 seems to focus on |
|
@ljharb For me, the reason is that, if you think about all of the different Temporal types that have a sense of, say, what the "day" is, and how this is calculated (given the complexity of calendars), it ends up adding significant additional complexity to make a sort of "fastpath" for these sorts of cases. (The complexity is more clear if you think through how the spec would be written, and implementations would end up looking similar.) |
|
The builtin temporal types could call into the intrinsics, for example, instead of doing lookups from an instance. |
|
I think it's fine to do these fast paths. There are the following ones:
I might be wrong but I don't expect this will add too much implementation complexity. I expect this is a question of adding a few extra steps to operations like ToTemporalDate, something like:
I'm not sure why we decided not to do these before but it was probably for simplicity and may have been before getters actually triggered all these observable Calendar calls. |
Is the rule that if no calls to the calendar and/or time zone are needed, then we use a fast path, otherwise we do observable reads of properties? Or does the fast path apply even in cases where the calendar and/or time zone needs to be called in order to calculate the result? If the former, then what about these?
If the latter, then these too?
|
I had the former in mind, but (see below) I'm not sure there are any more fast paths possible if we were to say the latter.
I didn't include these because they both must go through the calendar (PlainYearMonth must normalize the ISO day, and PlainMonthDay must normalize the ISO year.)
I think these are already "fast-pathed" as much as possible. You get a PlainDateTime by going through the time zone. In the case of ZonedDateTime → PlainDateTime, that object is returned directly. In the case of PlainDate and PlainTime, the slots are read. In the case of PlainMonthDay and PlainYearMonth, you still have to go through the calendar as above. |
Does it matter that for ISO or built-in non-ISO calendars, the ISO day is always And does it matter that for ISO or built-in calendars except Hebrew and Chinese/Dangi, the ISO year is always a constant value? In other words, is there some possible fast path for built-in calendars only that's possible by avoiding observable calls?
Not sure I understand. There are two ways to get
Isn't (2) best from a performance perspective and for ease for implementations to optimize? Or are you saying that we're already doing (2) so that's why it's not on your list? |
Exactly, we're already doing it. (see, for example https://tc39.es/proposal-temporal/#sec-temporal.zoneddatetime.prototype.toplaindate)
Maybe for built-in calendars you could have a non-observable path for ISO y/m/d to calendar PlainYearMonth or PlainMonthDay, but I'm not sold on this idea because it would duplicate calendar logic (choosing a reference year or reference day) outside of the calendar. To me, this crosses the boundary mentioned in #1428 (comment). |
|
Following up on this after having written a bunch of test262 tests for it...
I was only partially correct about this. We are doing it this way in places like |
If passing a Temporal object with a [[Calendar]] internal slot where a calendar is expected, then read that slot instead of doing a Get on the "calendar" property. (The Get operation was also not consistently done throughout the existing spec text, so fix that as well. This means the CalendarFrom operation becomes redundant, so remove it.) See: #1428
If passing a Temporal.ZonedDateTime where a Temporal.Instant is expected, then create a Temporal.Instant directly from the ZonedDateTime's [[Nanoseconds]] internal slot. Previously, the ZonedDateTime's toString() method would have been called, and the result would have been parsed into an Instant. See: #1428
If passing a Temporal.PlainDateTime where a Temporal.PlainDate is expected, then create a Temporal.PlainDate directly from the PlainDateTime's [[ISOYear]], [[ISOMonth]], and [[ISODay]] internal slots. Previously, the PlainDateTime's calendar, year, month, monthCode, and day getters would have been called. See: #1428
If passing a Temporal.PlainDate where a Temporal.PlainDateTime is expected, then create a Temporal.PlainDateTime directly from the PlainDate's [[ISOYear]], [[ISOMonth]], and [[ISODay]] internal slots, and assume midnight as the time, just as if a property bag with year, month/monthCode, and day properties had been passed. Previously, the PlainDate's calendar, year, month, monthCode, and day getters would have been called, and the time properties would have been accessed as well. See: #1428
If passing a Temporal.PlainDateTime where a Temporal.PlainTime is expected, then create a Temporal.PlainTime directly from the PlainDateTime's [[ISOHour]], [[ISOMinute]], [[ISOSecond]], [[ISOMillisecond]], [[ISOMicrosecond]], and [[ISONanosecond]] internal slots. Previously, the PlainDateTime's hour, minute, second, millisecond, microsecond, nanosecond, and calendar getters would have been called. See: #1428
If passing a Temporal.ZonedDateTime (that is, a Temporal object with a [[TimeZone]] internal slot) where a Temporal.TimeZone is expected, then read that slot instead of doing a Get on the "timeZone" property. The Get operation was also not consistently done throughout the existing spec text, so fix that as well. This means the TimeZoneFrom operation becomes redundant, so remove it. Additionally, the check for the "timeZone" property on plain objects was not implemented according to the consensus in #925 (comment) and this change fixes that as well. See: #1428
The value of a relativeTo property in a property bag is already allowed to be either a Temporal.PlainDateTime or Temporal.ZonedDateTime. If passing a Temporal.PlainDate, then create a Temporal.PlainDateTime from its slots as we do in ToTemporalDateTime. See: #1428
There are a few places where the `calendar` property of a property bag is read. If the property bag is actually a Temporal object with a [[Calendar]] internal slot, then read this slot instead. See: #1428
…Time If passing a Temporal.ZonedDateTime where a Temporal.PlainDate, Temporal.PlainTime, or Temporal.PlainDateTime is expected, then first call the built-in TimeZone.getPlainDateTimeFor() method to obtain a Temporal.PlainDateTime object and proceed accordingly. Previously, the ZonedDateTime's getters would have been called, potentially calling the getPlainDateTimeFor() method multiple times; very inefficient. See: #1428
All of the with() methods check that the given object doesn't have a `calendar` or `timeZone` property. Short-circuit these checks by first checking if it's a Temporal object with a [[Calendar]] or [[TimeZone]] internal slot, so that no Gets are performed in that case. See: #1428
Make sure it is consistent whether a particular Calendar method can be called with a Temporal.PlainYearMonth or Temporal.PlainMonthDay without triggering a conversion. (They can all be called with a Temporal.PlainDate.) See: #1428
If passing a Temporal object with a [[Calendar]] internal slot where a calendar is expected, then read that slot instead of doing a Get on the "calendar" property. (The Get operation was also not consistently done throughout the existing spec text, so fix that as well. This means the CalendarFrom operation becomes redundant, so remove it.) See: #1428
If passing a Temporal.ZonedDateTime where a Temporal.Instant is expected, then create a Temporal.Instant directly from the ZonedDateTime's [[Nanoseconds]] internal slot. Previously, the ZonedDateTime's toString() method would have been called, and the result would have been parsed into an Instant. See: #1428
If passing a Temporal.PlainDateTime where a Temporal.PlainDate is expected, then create a Temporal.PlainDate directly from the PlainDateTime's [[ISOYear]], [[ISOMonth]], and [[ISODay]] internal slots. Previously, the PlainDateTime's calendar, year, month, monthCode, and day getters would have been called. See: #1428
All of the with() methods check that the given object doesn't have a `calendar` or `timeZone` property. Short-circuit these checks by first checking if it's a Temporal object with a [[Calendar]] or [[TimeZone]] internal slot, so that no Gets are performed in that case. See: #1428
Make sure it is consistent whether a particular Calendar method can be called with a Temporal.PlainYearMonth or Temporal.PlainMonthDay without triggering a conversion. (They can all be called with a Temporal.PlainDate.) See: #1428
Now that we have a fast-path conversion from PlainDateTime to PlainDate, it's no longer necessary to check for an [[ISOYear]] internal slot. Make sure it is consistent whether a particular Calendar method can be called with a Temporal.PlainYearMonth or Temporal.PlainMonthDay without triggering a conversion, if appropriate. The policy is that these Calendar methods can all be called with a PlainDate, and additionally some of them can be called with a PlainYearMonth and/or PlainMonthDay. Anything else gets passed through ToTemporalDate, which includes the fast path for PlainDateTime. See: #1428
Now that we have a fast-path conversion from PlainDateTime to PlainDate, it's no longer necessary to check for an [[ISOYear]] internal slot. Make sure it is consistent whether a particular Calendar method can be called with a Temporal.PlainYearMonth or Temporal.PlainMonthDay without triggering a conversion, if appropriate. The policy is that these Calendar methods can all be called with a PlainDate, and additionally some of them can be called with a PlainYearMonth and/or PlainMonthDay. Anything else gets passed through ToTemporalDate, which includes the fast path for PlainDateTime. See: #1428
If passing a Temporal.ZonedDateTime (that is, a Temporal object with a [[TimeZone]] internal slot) where a Temporal.TimeZone is expected, then read that slot instead of doing a Get on the "timeZone" property. The Get operation was also not consistently done throughout the existing spec text, so fix that as well. This means the TimeZoneFrom operation becomes redundant, so remove it. Additionally, the check for the "timeZone" property on plain objects was not implemented according to the consensus in #925 (comment) and this change fixes that as well. See: #1428
The value of a relativeTo property in a property bag is already allowed to be either a Temporal.PlainDateTime or Temporal.ZonedDateTime. If passing a Temporal.PlainDate, then create a Temporal.PlainDateTime from its slots as we do in ToTemporalDateTime. See: #1428
There are a few places where the `calendar` property of a property bag is read. If the property bag is actually a Temporal object with a [[Calendar]] internal slot, then read this slot instead. See: #1428
…Time If passing a Temporal.ZonedDateTime where a Temporal.PlainDate, Temporal.PlainTime, or Temporal.PlainDateTime is expected, then first call the built-in TimeZone.getPlainDateTimeFor() method to obtain a Temporal.PlainDateTime object and proceed accordingly. Previously, the ZonedDateTime's getters would have been called, potentially calling the getPlainDateTimeFor() method multiple times; very inefficient. See: #1428
All of the with() methods check that the given object doesn't have a `calendar` or `timeZone` property. Short-circuit these checks by first checking if it's a Temporal object with a [[Calendar]] or [[TimeZone]] internal slot, so that no Gets are performed in that case. See: #1428
Now that we have a fast-path conversion from PlainDateTime to PlainDate, it's no longer necessary to check for an [[ISOYear]] internal slot. Make sure it is consistent whether a particular Calendar method can be called with a Temporal.PlainYearMonth or Temporal.PlainMonthDay without triggering a conversion, if appropriate. The policy is that these Calendar methods can all be called with a PlainDate, and additionally some of them can be called with a PlainYearMonth and/or PlainMonthDay. Anything else gets passed through ToTemporalDate, which includes the fast path for PlainDateTime. See: #1428
The constructors of PlainDate, PlainDateTime, PlainMonthDay, PlainYearMonth, and ZonedDateTime call ToTemporalCalendar on the calendar argument, and the constructor of ZonedDateTime additionally calls ToTemporalTimeZone on its timeZone argument. Now that these operations include an observable HasProperty operation, we do not want to call them more than once on the same calendar or time zone object. Therefore we add a private Symbol ES.PrivateCreateMethod, which hides internal static methods on these types, which create an instance without any observable validation of the arguments. The spec text already works like this, with abstract operations such as CreateTemporalZonedDateTime and friends that do not call ToTemporalCalendar or ToTemporalTimeZone. So this is not a normative change; it's a compliance bug in the polyfill. It is required for the test262 tests in 19cd26f to pass, which observe HasProperty(`timeZone`) operations. See: #1428
All of the with() methods check that the given object doesn't have a `calendar` or `timeZone` property. Short-circuit these checks by first checking if it's a Temporal object with a [[Calendar]] or [[TimeZone]] internal slot, so that no Gets are performed in that case. See: #1428
Now that we have a fast-path conversion from PlainDateTime to PlainDate, it's no longer necessary to check for an [[ISOYear]] internal slot. Make sure it is consistent whether a particular Calendar method can be called with a Temporal.PlainYearMonth or Temporal.PlainMonthDay without triggering a conversion, if appropriate. The policy is that these Calendar methods can all be called with a PlainDate, and additionally some of them can be called with a PlainYearMonth and/or PlainMonthDay. Anything else gets passed through ToTemporalDate, which includes the fast path for PlainDateTime. See: #1428
The constructors of PlainDate, PlainDateTime, PlainMonthDay, PlainYearMonth, and ZonedDateTime call ToTemporalCalendar on the calendar argument, and the constructor of ZonedDateTime additionally calls ToTemporalTimeZone on its timeZone argument. Now that these operations include an observable HasProperty operation, we do not want to call them more than once on the same calendar or time zone object. Therefore we add CreateTemporalDate etc. which create an instance without any observable validation of the arguments. The spec text already works like this, with abstract operations such as CreateTemporalZonedDateTime and friends that do not call ToTemporalCalendar or ToTemporalTimeZone. So this is not a normative change; it's a compliance bug in the polyfill. It is required for the test262 tests in 19cd26f to pass, which observe HasProperty(`timeZone`) operations. See: #1428
If passing a Temporal object with a [[Calendar]] internal slot where a calendar is expected, then read that slot instead of doing a Get on the "calendar" property. (The Get operation was also not consistently done throughout the existing spec text, so fix that as well. This means the CalendarFrom operation becomes redundant, so remove it.) See: #1428
If passing a Temporal.ZonedDateTime where a Temporal.Instant is expected, then create a Temporal.Instant directly from the ZonedDateTime's [[Nanoseconds]] internal slot. Previously, the ZonedDateTime's toString() method would have been called, and the result would have been parsed into an Instant. See: #1428
If passing a Temporal.PlainDateTime where a Temporal.PlainDate is expected, then create a Temporal.PlainDate directly from the PlainDateTime's [[ISOYear]], [[ISOMonth]], and [[ISODay]] internal slots. Previously, the PlainDateTime's calendar, year, month, monthCode, and day getters would have been called. See: #1428
If passing a Temporal.PlainDate where a Temporal.PlainDateTime is expected, then create a Temporal.PlainDateTime directly from the PlainDate's [[ISOYear]], [[ISOMonth]], and [[ISODay]] internal slots, and assume midnight as the time, just as if a property bag with year, month/monthCode, and day properties had been passed. Previously, the PlainDate's calendar, year, month, monthCode, and day getters would have been called, and the time properties would have been accessed as well. See: #1428
If passing a Temporal.PlainDateTime where a Temporal.PlainTime is expected, then create a Temporal.PlainTime directly from the PlainDateTime's [[ISOHour]], [[ISOMinute]], [[ISOSecond]], [[ISOMillisecond]], [[ISOMicrosecond]], and [[ISONanosecond]] internal slots. Previously, the PlainDateTime's hour, minute, second, millisecond, microsecond, nanosecond, and calendar getters would have been called. See: #1428
If passing a Temporal.ZonedDateTime (that is, a Temporal object with a [[TimeZone]] internal slot) where a Temporal.TimeZone is expected, then read that slot instead of doing a Get on the "timeZone" property. The Get operation was also not consistently done throughout the existing spec text, so fix that as well. This means the TimeZoneFrom operation becomes redundant, so remove it. Additionally, the check for the "timeZone" property on plain objects was not implemented according to the consensus in #925 (comment) and this change fixes that as well. See: #1428
The value of a relativeTo property in a property bag is already allowed to be either a Temporal.PlainDateTime or Temporal.ZonedDateTime. If passing a Temporal.PlainDate, then create a Temporal.PlainDateTime from its slots as we do in ToTemporalDateTime. See: #1428
There are a few places where the `calendar` property of a property bag is read. If the property bag is actually a Temporal object with a [[Calendar]] internal slot, then read this slot instead. See: #1428
…Time If passing a Temporal.ZonedDateTime where a Temporal.PlainDate, Temporal.PlainTime, or Temporal.PlainDateTime is expected, then first call the built-in TimeZone.getPlainDateTimeFor() method to obtain a Temporal.PlainDateTime object and proceed accordingly. Previously, the ZonedDateTime's getters would have been called, potentially calling the getPlainDateTimeFor() method multiple times; very inefficient. See: #1428
All of the with() methods check that the given object doesn't have a `calendar` or `timeZone` property. Short-circuit these checks by first checking if it's a Temporal object with a [[Calendar]] or [[TimeZone]] internal slot, so that no Gets are performed in that case. See: #1428
Now that we have a fast-path conversion from PlainDateTime to PlainDate, it's no longer necessary to check for an [[ISOYear]] internal slot. Make sure it is consistent whether a particular Calendar method can be called with a Temporal.PlainYearMonth or Temporal.PlainMonthDay without triggering a conversion, if appropriate. The policy is that these Calendar methods can all be called with a PlainDate, and additionally some of them can be called with a PlainYearMonth and/or PlainMonthDay. Anything else gets passed through ToTemporalDate, which includes the fast path for PlainDateTime. See: #1428
The constructors of PlainDate, PlainDateTime, PlainMonthDay, PlainYearMonth, and ZonedDateTime call ToTemporalCalendar on the calendar argument, and the constructor of ZonedDateTime additionally calls ToTemporalTimeZone on its timeZone argument. Now that these operations include an observable HasProperty operation, we do not want to call them more than once on the same calendar or time zone object. Therefore we add CreateTemporalDate etc. which create an instance without any observable validation of the arguments. The spec text already works like this, with abstract operations such as CreateTemporalZonedDateTime and friends that do not call ToTemporalCalendar or ToTemporalTimeZone. So this is not a normative change; it's a compliance bug in the polyfill. It is required for the test262 tests in 19cd26f to pass, which observe HasProperty(`timeZone`) operations. See: #1428
(To start, this issue is not about TimeZone and Calendar, but the other Temporal types)
#1419 (comment) implies that
.frommethods are meant to support the following inputs:The current spec seems to treat the first case as a subset of the third case - ie, it looks up object properties, and assumes that a Temporal-like object and a Temporal object have the same property names.
For actual Temporal instances, why would we not prefer looking things up from internal slots, so as to minimize observable lookups and calls into user code?
The text was updated successfully, but these errors were encountered: