Skip to content
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

Open
ptomato opened this issue May 2, 2024 · 4 comments
Open

Comments

@ptomato
Copy link
Collaborator

ptomato commented May 2, 2024

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:

  • PlainDate, PlainDateTime, ZonedDateTime: calendarId, era, eraYear, year, month, monthCode, day, dayOfWeek, dayOfYear, weekOfYear, yearOfWeek, daysInWeek, daysInMonth, daysInYear, monthsInYear, inLeapYear
  • PlainTime, PlainDateTime, ZonedDateTime: hour, minute, second, millisecond, microsecond, nanosecond
  • Instant, ZonedDateTime: epochSeconds, epochMilliseconds, epochMicroseconds, epochNanoseconds
  • ZonedDateTime: timeZoneId, hoursInDay, offsetNanoseconds, offset
  • Duration: years, months, weeks, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds, sign, blank
  • PlainYearMonth: calendarId, era, eraYear, year, month, monthCode, daysInYear, daysInMonth, monthsInYear, inLeapYear
  • PlainMonthDay: 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:

  • Hugely reducing the number of builtins which would be beneficial for (at least) V8.
  • If the own properties were also enumerable, Temporal objects would become spreadable into plain objects. This is something that we really wanted in the past, but dropped because it didn't seem feasible under the constraints that we had. (It is the reason that the with(), withPlainDate(), withPlainTime(), etc. methods exist.) This would change those constraints. It might be possible to remove those methods: instead of pdt.withPlainTime(pt) you could do PlainDateTime.from({...pdt, ...pt}). (Note: consider how to reconcile the calendars in withPlainDate)

Cons:

  • Worse runtime performance. Constructors would need to compute and define a lot of own properties, most of which might never be read.
  • Currently, Temporal constructors can't fail if they are called with the proper new.target. I could imagine that there'd be some way to make the property definitions fail by messing with Object.prototype.
  • Harder to re-add custom calendars and time zones in the future.
  • Deviating from the usual ECMA-262 pattern of storing data in internal slots and exposing it with getters is unusual.
  • Less flexibility for monkeypatching.

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.

@ljharb
Copy link
Member

ljharb commented May 2, 2024

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.

@littledan
Copy link
Member

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).

@justingrant
Copy link
Collaborator

  • Worse runtime performance. Constructors would need to compute and define a lot of own properties, most of which might never be read.

Can engines optimize this overhead away? I'm not suggesting we should make this change, but was curious.

@anba
Copy link
Contributor

anba commented May 2, 2024

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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants