-
Notifications
You must be signed in to change notification settings - Fork 155
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
Most of methods of Temporal.PlainDate requires a fresh new Calendar instance for each Temporal.PlainDate #1808
Comments
My thinking on this topic so far has been that it will be mostly unnecessary to create calendar objects. If the following three conditions hold, then you won't need to create the object:
It's annoying that the implementation would have to keep checking that condition 1 holds every time you would have to call a calendar method. But otherwise I expect that in the vast majority of applications, these 3 conditions will hold, and implementations can elide the calendar objects while still maintaining the same semantics as if they were created. Do you have any ideas already in mind for how to remove the necessity of creating calendar objects? Related: #1432; #1588; thread starting at #1294 (comment) for an exploration of a different way that it could work and the weirdnesses that that brings in. |
I don't think implementation optimizations are the right perspective from which to approach this issue. The question is instead whether or not the following kinds of action-at-a-distance (and possibly covert communication) should be supported. const instance = Temporal.PlainDate.from("2021-11-10");
Temporal.Calendar.from("iso8601").year = () => 0;
instance.year; // => 0 |
Here's my summary of what we discussed in the meeting of 2021-11-18.
|
I have done some initial investigation around this and will continue by figuring out how to express these changes in the spec text. (1) Built-in calendar objects have own-property methods: https://github.com/tc39/proposal-temporal/compare/frozen-builtin-calendar-1?expand=1 In both branches you can see some tests that illustrate what this change would mean for Calendar objects. You can view the branches commit-by-commit to see what would change between the two approaches. A few things to note:
Please let me know if you have any opinions about this. |
I haven't had any comments in the meantime, so I intend to proceed with approach (1) after the September plenary. |
It's probably worth mentioning that this will make it more easier to optimise Calendar/TimeZone methods in the interpreter, but will probably require special handing in the JITs. For example if
I'm still not convinced that we need this optimisation. Especially copying every built-in method seems questionable to me. Checking if a built-in Calendar uses the built-in Calendar methods shouldn't be too hard to implement in engines and fairly fast. (Should be fast because there are only memory loads and comparisons.) Engines typically know how to implement this sequence of operations:
|
Thanks for the comments!
I intend to take this proposal to the SES meeting and discuss it there before discussing it in TC39. My understanding is that the way that adding new unreachable intrinsincs would "break existing code" is that the code expects to be able to reach them by walking the properties of the global object in order to freeze them. So, it seems like if they were already frozen that wouldn't apply. I considered whether we could make them reachable by walking the properties, which seems doable for calendars (and you could test for support of a particular calendar with
I'm glad you raised this. How big of a problem is it? I don't have a sense of whether it'd be a minor annoyance for JIT implementors or something that would break all sorts of assumptions.
That sounds like a good reason to go for approach 2 rather than 1, if there are no larger disadvantages. Although in approach 2
This is helpful too. This is a more detailed version of my hand-wavy assumption in #1808 (comment) that engines could optimize this. All other things being equal, I'd be happy not to make a change here. Maybe @Constellation and @FrankYFTang could weigh in about these checks? |
It kind of depends on the exact specification. For example it's more complicated if every built-in Calendar/TimeZone method is a bound function. It should be less complicated if the methods are the same function object for all built-in Calendar/TimeZone objects. That means if
That's also the case for approach 1, isn't it? With approach 1: var iso8601 = Temporal.Calendar.from("iso8601");
var gregorian = Temporal.Calendar.from("gregory");
assert.notSameValue(iso8601.year, gregorian.year);
assert.notSameValue(iso8601.year, Temporal.Calendar.prototype.year);
assert.notSameValue(gregorian.year, Temporal.Calendar.prototype.year); With approach 2: var iso8601 = Temporal.Calendar.from("iso8601");
var gregorian = Temporal.Calendar.from("gregory");
assert.sameValue(iso8601.year, gregorian.year);
assert.notSameValue(iso8601.year, Temporal.Calendar.prototype.year);
assert.notSameValue(gregorian.year, Temporal.Calendar.prototype.year); |
https://bugs.webkit.org/show_bug.cgi?id=245282 Reviewed by Yusuke Suzuki. This patch implements the following Temporal API methods: - PlainDate.prototype.with - PlainDateTime.prototype.{with, withPlainDate, withPlainTime, round} Note: `with` will likely need significant refactoring in the future. At present, the spec demands bafflingly inefficient behavior, observably calling out to Calendar multiple times and creating multiple intermediate JSObjects, even in the default case where the user is not making conscious use of Calendar at all. This is a showstopper, but one which Yusuke already reported in tc39/proposal-temporal#1808. * JSTests/stress/temporal-plaindate.js: * JSTests/stress/temporal-plaindatetime.js: * JSTests/test262/config.yaml: * JSTests/test262/expectations.yaml: * Source/JavaScriptCore/runtime/TemporalCalendar.cpp: (JSC::TemporalCalendar::from): (JSC::TemporalCalendar::isoDateFromFields): Split into two. * Source/JavaScriptCore/runtime/TemporalCalendar.h: * Source/JavaScriptCore/runtime/TemporalObject.cpp: (JSC::rejectObjectWithCalendarOrTimeZone): Added. * Source/JavaScriptCore/runtime/TemporalObject.h: * Source/JavaScriptCore/runtime/TemporalPlainDate.cpp: (JSC::TemporalPlainDate::toPartialDate): Added. (JSC::TemporalPlainDate::with): Added. * Source/JavaScriptCore/runtime/TemporalPlainDate.h: * Source/JavaScriptCore/runtime/TemporalPlainDatePrototype.cpp: * Source/JavaScriptCore/runtime/TemporalPlainDateTime.cpp: (JSC::TemporalPlainDateTime::with): Added. (JSC::TemporalPlainDateTime::round): Added. * Source/JavaScriptCore/runtime/TemporalPlainDateTime.h: * Source/JavaScriptCore/runtime/TemporalPlainDateTimePrototype.cpp: * Source/JavaScriptCore/runtime/TemporalPlainTime.cpp: (JSC::TemporalPlainTime::roundTime): (JSC::TemporalPlainTime::toTemporalTimeRecord): (JSC::TemporalPlainTime::toPartialTime): (JSC::TemporalPlainTime::with const): (JSC::toPartialTime): Made public static. * Source/JavaScriptCore/runtime/TemporalPlainTime.h: Canonical link: https://commits.webkit.org/254565@main
I think this issue is an important one to address, since the optimization cited earlier is very fragile: It needs to be turned off both whenever there are any changes to the Timezone/Calendar prototypes, and whenever the At the same time, I'd be cautious about using frozen classes to enable singleton instances. The most natural way to freeze classes would be to make To avoid these problems, I'd suggest a different design. The following is based on significant discussion with @gibson042 @ptomato and @pipobscure who helped me refine it. The data model of calendars and timezones, and occur as internal slots in dates and times and such, changes from "always an object with methods for operations" to "either an object or a string". If a string is provided as the argument to a date/time constructor, or if the Now/iso APIs are used, or the default iso calendar from certain date APIs appears, then rather than creating a Temporal.Calendar or Temporal.TimeZone instance, the built-in calendar/timezone is represented directly as the string that was going to be passed into that constructor. For example, the When an operation is done on a calendar or timezone, the logic changes from "call the appropriate method and use the result" to "if this is a string, directly apply the correct built-in algorithm; otherwise, call the right method on the calendar/timezone object". In the end, the observable behavior is extremely similar to if we used frozen classes for singleton objects for built-in timezones and calendars, but without all the complexity around the object model of those instances, and substituting that for the complexity of the logic to switch between the two cases at method call sites in internal algorithms. Frozen classes and the string-based system are similar in that there is no risk of a "performance cliff" from optimizations that may turn off if you get them confused. What do you think? |
I like the direction of using a primitive to represent built-in calendars and time zones. At what point do we check that we support the calendar or time zone ID? Do we verify eagerly, or do we wait until we actually need it to perform an operation? Is the string calendar/time zone ID the correct primitive, or should we consider e.g. symbols as the primitive? |
The proposed solution from @littledan is intriguing. I see its advantages, especially for implementers. Is this "string or object" pattern used in other built-in objects, either for method return values or object properties? (Other than types like I admittedly don't understand the downsides of the frozen object alternative. @littledan, I heard from @ptomato that you have some insight about challenges (technical as well as more opinionated concerns from committee members) about freezing. Could you share those here? I also see some significant challenges from the perspective of app developers and userland libraries... challenges which would go away if a frozen-object solution were adopted. I'll follow up with another post soon to explain the problems I see for app developers, but in parallel I'd also like to understand the problems with frozen objects, which might outweigh the problems I'm seeing for "string or object" properties. |
Here's a few concerns around the "string or object" solution: TypeScriptOne problem is library code: // library.ts
export function nowInMyTimeZone(timeZone?: string) {
return Temporal.Now.zonedDateTimeISO(timeZone);
}
// app.ts
function doSomething(zdt: Temporal.ZonedDateTime) {
const now = nowInMyTimeZone(zdt.timeZone); // ERROR!
...
} Library authors (including people in your own company) will probably assume that TZ is always a string, which puts callers of that code in a bind. They can lie to TS and claim it's a string, knowing that it will probably be OK. Or they can try to get the library author to change their signature, which can often be hard or impossible depending on the maintainer. This could also happen with the current API. Nothing stops library authors from being more restrictive. But I suspect that having the properties be strings almost all the time will make it more likely. There's also an easy-to-fix but annoying ergonomic issue, where type casts will be needed in a lot of places. let tz = 'America/Los_Angeles';
if (someCondition) tz = zdt.timeZone; // ERROR! The core problem is that even if a developer's code has no custom timezones/calendars, TypeScript doesn't know this so type casts are required. And if the developer adds type casts, then the code becomes brittle if, later, custom timezones/calendars are added. Challenges with
|
Given that there’s TS syntax to extract the type of the first argument from zonedDateTimeISO, and the DT types would probably export this as a type for convenience, if a dev only accepts a string they just have a bug. I don’t think we need to be concerned with this case in TS when designing JS. |
Apologies for my delayed response here. It will take me more time to consider how significant the downsides of object-or-string are, but here's my reasoning for avoiding frozen objects: If we make a frozen class, we have two choices, both of which violate constraints that we've discussed in this thread:
I'm also motivated by a possibly-weaker rationale for the object-or-string version: I see it as having a smaller "surface area". Now, this is a very debatable concept--exposing strings is its own surface area--but making this whole parallel class, which then implementations are supposed to realize they can elide, seems to me like a lot of "stuff" when there's a lighter-weight option sitting right there. I think JS is based on pervasive "union types", so it wouldn't be out of character to use one here. I do see your point that APIs that expect a timezone object should now also accept a string, if we adopt this change. |
My understanding is that unions (aka permissive typing) are used extensively in parameters to methods and constructors, as well as in coercion. In other words, as input. But my understanding is that the language generally canonicalizes input into well-known types on output generated by the language. Is this assumption correct? Are there any other cases in the language today where the return type of a method or property getter is a union type? (Other than the obvious exceptions of Array, Set, etc. who's entire purpose is to expose user data which could be a union.) |
Dumb question: why do classes need to be frozen? Why not just freeze the instances but not the class? |
The prototype would still need to be frozen, otherwise the instance methods wouldn't be safe to use. |
If the methods are frozen, then the built-in implementation can be used rather than dispatching out to JS all the time. The string version meets this goal. Freezing the instances doesn't achieve the elimination of state, since the big issue is that we shouldn't have any "hidden" identities embedded in methods like |
@ljharb is SES-compatible that what you meant by "safe to use"? Or is there a different problem you were referring to? If the former, @gibson042 noted in today's meeting that because all the methods on the TZ/Cal prototypes would be reachable from the global object, they could be locked down by SES even if shared singleton TZ/Cal instances were used. Or did we miss something?
Would it be possible to explain this case a bit more? What's the specific attack you have in mind? Also, @gibson042 how does Dan's point here relate to your conclusion in today's meeting that shared singleton frozen objects seemed to be SES-compatible?
Does frozen instances achieve this goal too? |
The toJSON method should return the value in the internal slot, instead of making an observable call to toString. See: #1808
Refactor so that ZonedDateTime's [[TimeZone]] internal slot can now store either a string (builtin time zone) or an object (custom time zone). The .timeZone getter is replaced with a .timeZoneId getter that always returns a string, and a .getTimeZone() method that always returns a new object if the slot stored a string. The operations ToTemporalTimeZoneIdentifier and ToTemporalTimeZoneObject convert a [[TimeZone]] slot value to one or the other. See: #1808
When creating Temporal objects with a builtin time zone, we want them to be created with a string in the internal slot, not a TimeZone instance. (For example, when parsing an IANA time zone name out of a string or using UTC as a default.) See: #1808
Previously algorithms such as TimeZoneEquals used ToString to get the identifier of a time zone object. Instead, use ToTemporalTimeZoneIdentifier which observably gets the .id property of a custom time zone instead of getting and calling the .toString property. See: #1808
The toJSON method should return the value in the internal slot, instead of making an observable call to toString. See: #1808
Temporal.Now.timeZone() is replaced by Temporal.Now.timeZoneId() which always returns a time zone identifier string, rather than a TimeZone instance. See: #1808
PlainTime now has no calendar. Removes the getter and the internal slot. Calendar annotations are now ignored in ParseTemporalTimeString. RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is renamed to RejectTemporalLikeObject. ToTemporalCalendar rejects PlainTime explicitly instead of looking for its `calendar` property or treating it as a plain object calendar. See: #1808 Closes: #1588
Refactor so that Temporal objects' [[Calendar]] internal slot can now store either a string (builtin calendar) or an object (custom calendar). The .calendar getter is replaced with a .calendarId getter that always returns a string, and a .getCalendar() method that always returns a new object if the slot stored a string. The operations ToTemporalCalendarIdentifier and ToTemporalCalendarObject convert a [[Calendar]] slot value to one or the other. See: #1808
When creating Temporal objects with a builtin ISO 8601 calendar, we want them to be created with a string in the internal slot, not a Calendar instance. (For example, when parsing a calendar name out of a string or using ISO 8601 as a default.) Therefore we don't need the GetISO8601Calendar and GetBuiltinCalendar AOs anymore. See: #1808
Previously algorithms such as CalendarEquals used ToString to get the identifier of a calendar object. Instead, use ToTemporalCalendarIdentifier which observably gets the .id property of a custom calendar instead of getting and calling the .toString property. See: #1808
The toJSON method should return the value in the internal slot, instead of making an observable call to toString. See: #1808
Refactor so that ZonedDateTime's [[TimeZone]] internal slot can now store either a string (builtin time zone) or an object (custom time zone). The .timeZone getter is replaced with a .timeZoneId getter that always returns a string, and a .getTimeZone() method that always returns a new object if the slot stored a string. The operations ToTemporalTimeZoneIdentifier and ToTemporalTimeZoneObject convert a [[TimeZone]] slot value to one or the other. See: #1808
When creating Temporal objects with a builtin time zone, we want them to be created with a string in the internal slot, not a TimeZone instance. (For example, when parsing an IANA time zone name out of a string or using UTC as a default.) See: #1808
Previously algorithms such as TimeZoneEquals used ToString to get the identifier of a time zone object. Instead, use ToTemporalTimeZoneIdentifier which observably gets the .id property of a custom time zone instead of getting and calling the .toString property. See: #1808
The toJSON method should return the value in the internal slot, instead of making an observable call to toString. See: #1808
Closed by #2482. |
Temporal.Now.timeZone() is replaced by Temporal.Now.timeZoneId() which always returns a time zone identifier string, rather than a TimeZone instance. See: #1808 UPSTREAM_COMMIT=d67db4a7731d13f9e6e2b4be4f6d10732756c06e
PlainTime now has no calendar. Removes the getter and the internal slot. Calendar annotations are now ignored in ParseTemporalTimeString. RejectObjectWithCalendarOrTimeZone still rejects PlainTime, so it is renamed to RejectTemporalLikeObject. ToTemporalCalendar rejects PlainTime explicitly instead of looking for its `calendar` property or treating it as a plain object calendar. See: #1808 Closes: #1588 UPSTREAM_COMMIT=61fbb401df7463e435b8e6f932de123fd5bc0c2d
Refactor so that Temporal objects' [[Calendar]] internal slot can now store either a string (builtin calendar) or an object (custom calendar). The .calendar getter is replaced with a .calendarId getter that always returns a string, and a .getCalendar() method that always returns a new object if the slot stored a string. The operations ToTemporalCalendarIdentifier and ToTemporalCalendarObject convert a [[Calendar]] slot value to one or the other. See: #1808 UPSTREAM_COMMIT=7cf55b9bea1f19ee5a911934e17a37a9bbf0a4c7
When creating Temporal objects with a builtin ISO 8601 calendar, we want them to be created with a string in the internal slot, not a Calendar instance. (For example, when parsing a calendar name out of a string or using ISO 8601 as a default.) Therefore we don't need the GetISO8601Calendar and GetBuiltinCalendar AOs anymore. See: #1808 UPSTREAM_COMMIT=52f3159ba4d03343e1cfdbcee1720b03e28753b8
Previously algorithms such as CalendarEquals used ToString to get the identifier of a calendar object. Instead, use ToTemporalCalendarIdentifier which observably gets the .id property of a custom calendar instead of getting and calling the .toString property. See: #1808 UPSTREAM_COMMIT=5e0e87061bfb563a2b8a0c740da5995d8ed850be
The toJSON method should return the value in the internal slot, instead of making an observable call to toString. See: #1808 UPSTREAM_COMMIT=9199c05fdd58ddf45a03211396762e57da4fe6bc
Refactor so that ZonedDateTime's [[TimeZone]] internal slot can now store either a string (builtin time zone) or an object (custom time zone). The .timeZone getter is replaced with a .timeZoneId getter that always returns a string, and a .getTimeZone() method that always returns a new object if the slot stored a string. The operations ToTemporalTimeZoneIdentifier and ToTemporalTimeZoneObject convert a [[TimeZone]] slot value to one or the other. See: #1808 UPSTREAM_COMMIT=c56a90c814b4632b87da80be26f4775b0fae1751
When creating Temporal objects with a builtin time zone, we want them to be created with a string in the internal slot, not a TimeZone instance. (For example, when parsing an IANA time zone name out of a string or using UTC as a default.) See: #1808 UPSTREAM_COMMIT=973981239d033fe29bbcaa753bccfcd688e654c3
Previously algorithms such as TimeZoneEquals used ToString to get the identifier of a time zone object. Instead, use ToTemporalTimeZoneIdentifier which observably gets the .id property of a custom time zone instead of getting and calling the .toString property. See: #1808 UPSTREAM_COMMIT=08ae76a9030e7592825317e4da4444f7686a2dd2
The toJSON method should return the value in the internal slot, instead of making an observable call to toString. See: #1808 UPSTREAM_COMMIT=51bb26b9392b90be6a7341a8af2332559639107e
Really closed by #2482. :-) |
The current spec creates a fresh instance for each
Temporal.PlainDate
. Since most of fields ofTemporal.PlainDate
touches calendars,Temporal.PlainDate
almost always requires two objects per instance.This fresh object is meaningful only when the user adds adhoc method to these created calendar instances after creating
Temporal.PlainDate
. But the use of custom calendar can be cleanly achieved by passing an optional calendar parameter to theTemporal.PlainDate
constructor.So, how about removing necessity of creating a calendar object for known calendars for performance and memory saving?
The text was updated successfully, but these errors were encountered: