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
Investigate having read-only properties instead of accessor properties #2834
Comments
Accessors allow for more robust code patterns that extract the accessor and call-bind it, directly exposing the values of internal slots even if the prototype is later modified. Having a frozen property on a branded object that has some brand-checking mechanism would suffice, but it wouldn't be consistent with the rest of the language. |
I vaguely like the idea of continuing to use accessors, but I don't have an extremely strong argument. We've been moving in the accessor direction with ES6 (e.g. for RegExp flags), and this is aligned with how the web platform does things (WebIDL attributes are exposed as accessors). It feels somehow "safer" to me (more precisely, simpler to prove the safety) if the internal algorithms are only referencing internal slots and not frozen JS properties, even if in reality it is all the same amount safe (given that those algorithms would refuse to run if the brand isn't right). |
Can engines optimize this overhead away? I'm not suggesting we should make this change, but was curious. |
SpiderMonkey has a thing called "resolve hook", which allows to lazily compute data properties. But I don't think we're eager to be forced to have to use this escape hatch, because it leads to multiple other issues:
|
In the investigation in #2786 we confirmed that public builtin functions are quite expensive in V8. One idea that surfaced during today's champions meeting, to reduce the number of builtins, would be to replace the following accessor prototype properties with readonly own data properties:
calendarId
,era
,eraYear
,year
,month
,monthCode
,day
,dayOfWeek
,dayOfYear
,weekOfYear
,yearOfWeek
,daysInWeek
,daysInMonth
,daysInYear
,monthsInYear
,inLeapYear
hour
,minute
,second
,millisecond
,microsecond
,nanosecond
epochSeconds
,epochMilliseconds
,epochMicroseconds
,epochNanoseconds
timeZoneId
,hoursInDay
,offsetNanoseconds
,offset
years
,months
,weeks
,days
,hours
,minutes
,seconds
,milliseconds
,microseconds
,nanoseconds
,sign
,blank
calendarId
,era
,eraYear
,year
,month
,monthCode
,daysInYear
,daysInMonth
,monthsInYear
,inLeapYear
calendarId
,monthCode
,day
This would remove 103 builtins in total, about 1/3 of the builtins currently existing in Temporal.
Some of these were previously required to be accessor properties, because they would need to call calendar or time zone methods. With custom calendars and time zones being removed in #2826, it would now be possible for these to be own data properties. (If we wanted to add custom calendars and time zones in a future proposal, these would need to be converted back to accessor properties. I'm assuming that would be a web-compatible change. If not, then each constructor would need to make a bunch of calendar/time zone calls up front, which would not be very performant.)
To ensure that Temporal objects remained immutable, each type's constructor would need to define these own data properties, and they'd need to be non-writable and non-configurable.
Pros:
pdt.withPlainTime(pt)
you could doPlainDateTime.from({...pdt, ...pt})
. (Note: consider how to reconcile the calendars in withPlainDate)Cons:
new.target
. I could imagine that there'd be some way to make the property definitions fail by messing withObject.prototype
.NOTE: There is currently no decision to go ahead with this. It might be a terrible idea. This issue thread is for discussing and investigating whether this is feasible.
The text was updated successfully, but these errors were encountered: