Normative: Audit of user-visible operations in Temporal #2519

ptomato · 2023-03-11T06:36:00Z

This is an audit of the whole proposal for opportunities where we can reduce the number of calls we make into user code.

Very large, recommend reviewing commit by commit.

I will push a test262 branch with changes in the order-of-operations tests, so that it's easier to see what the effects are on user code calls: https://github.com/tc39/test262/compare/main...ptomato:user-code-calls?expand=1
Calendar Record commit to follow tomorrow morning. I'm still doing a bunch of search-and-replace on that one.

codecov · 2023-03-11T06:40:21Z

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (63c1d3a) 96.35% compared to head (f77ca4e) 96.36%.

❗ Current head f77ca4e differs from pull request most recent head 8234ec4. Consider uploading reports for the commit 8234ec4 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##             main    #2519    +/-   ##
========================================
  Coverage   96.35%   96.36%            
========================================
  Files          20       21     +1     
  Lines       12196    12535   +339     
  Branches     2274     2346    +72     
========================================
+ Hits        11752    12079   +327     
- Misses        389      401    +12     
  Partials       55       55

Files	Coverage Δ
polyfill/lib/duration.mjs	`96.04% <100.00%> (+0.23%)`	⬆️
polyfill/lib/intl.mjs	`97.80% <100.00%> (+0.02%)`	⬆️
polyfill/lib/now.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/plaindate.mjs	`99.68% <100.00%> (+0.01%)`	⬆️
polyfill/lib/plaindatetime.mjs	`98.81% <100.00%> (+<0.01%)`	⬆️
polyfill/lib/plainmonthday.mjs	`97.61% <100.00%> (+0.11%)`	⬆️
polyfill/lib/plaintime.mjs	`98.26% <100.00%> (+0.01%)`	⬆️
polyfill/lib/plainyearmonth.mjs	`98.32% <100.00%> (+0.04%)`	⬆️
polyfill/lib/timezone.mjs	`100.00% <100.00%> (ø)`
polyfill/lib/zoneddatetime.mjs	`100.00% <100.00%> (ø)`
... and 2 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ptomato · 2023-03-11T20:48:05Z

The Calendar Record commit landed on this PR a couple of hours after the official TC39 agenda deadline. If this interferes with your review of this PR, please let me know; I'm happy to go over it with you.

polyfill/lib/ecmascript.mjs

spec/duration.html

spec/plainmonthday.html

justingrant

I'm amazed that any single human being made a PR of this scope. Very impressive work here. It's like running a nerdy ultra-marathon!

I focused my review of the logic on the polyfill code, and assumed that the spec matched the polyfill because I was exhausted by the time I got to the spec part. :-)

Only a few minor comments/nits.

polyfill/lib/duration.mjs

polyfill/lib/ecmascript.mjs

spec/timezone.html

spec/duration.html

When GetMethod is used to lookup the dateAdd or dateUntil method of a calendar, we need to account for the case where the calendar is a builtin calendar represented by a string. This was an oversight in #2482. In hindsight, the lookups of dateAdd and dateUntil in BalanceDurationRelative should have been grouped together as they were in UnbalanceDurationRelative. But since that will be addressed anyway by the Calendar Method Records in the yet-to-be-merged PR #2519, I'm not planning to change that here. Closes: #2547

Just as in CalendarDateAdd and CalendarDateUntil, add optional parameters to GetPossibleInstantsFor and GetOffsetNanosecondsFor which can supply the method to be called, in case the caller has already looked it up. This doesn't make any observable change, but it will be used for optimizations in #2519.

Do the same for CalendarDateFromFields as in CalendarDateAdd and CalendarDateUntil: add an optional parameter to CalendarFields which can supply the method to be called, in case the caller has already looked it up. This doesn't make any observable change, but it will be used for optimizations in #2519.

ptomato · 2023-04-22T01:25:07Z

I rebased this and it's now stacked on top of #2562. There are still some review comments left to address.

Just as in CalendarDateAdd and CalendarDateUntil, add optional parameters to GetPossibleInstantsFor and GetOffsetNanosecondsFor which can supply the method to be called, in case the caller has already looked it up. This doesn't make any observable change, but it will be used for optimizations in #2519.

Do the same for CalendarDateFromFields as in CalendarDateAdd and CalendarDateUntil: add an optional parameter to CalendarFields which can supply the method to be called, in case the caller has already looked it up. This doesn't make any observable change, but it will be used for optimizations in #2519.

spec/plainyearmonth.html

cjtenny

There are a couple of cases where assertions are backwards or prose in descriptions differs from assertions in the text; see comments. The rest of my comments are editorial questions/suggestions only. Beyond those couple of minor conditionals, this all looks great and correct to me, and makes the user code interactions a lot more predictable. I'd say, ship it!

spec/timezone.html

cjtenny · 2023-10-24T21:11:11Z

spec/zoneddatetime.html

        </dd>
      </dl>
      <emu-alg>
        1. Assert: IsValidISODate(_year_, _month_, _day_) is *true*.
        1. Let _dateTime_ be ? CreateTemporalDateTime(_year_, _month_, _day_, _hour_, _minute_, _second_, _millisecond_, _microsecond_, _nanosecond_, *"iso8601"*).
        1. If _offsetBehaviour_ is ~wall~ or _offsetOption_ is *"ignore"*, then
-          1. Let _instant_ be ? GetInstantFor(_timeZone_, _dateTime_, _disambiguation_).
+          1. Assert: TimeZoneMethodsRecordHasLookedUp(_timeZoneRec_, ~getPossibleInstantsFor~) is *true*.


This assertion conflicts with the paragraph added above to the operation's description.
This path can be taken if _offsetBehavior_ is ~exact~ and _offsetOption_ is *"ignore"*, or if _offsetOption_ is *"use"* and _offsetBehavior_ is ~wall~.

In all uses of this abstract operation, the assertion won't be violated, but the paragraph above still editorially conflicts with the assertion step, since ~wall~ or *"ignore"* are checked first.

I think a more accurate version would be:
Unless (_offsetBehavior_ is ~exact~ or _offsetOption_ is *"use"*) and (_offsetBehavior is not ~wall~ and _offsetOption~ is not *"ignore"*), ...

But that sounds unwieldy. I think there's a better way to place this assertion / order these tests, but maybe that'd move some observable effects around or affect the results?

For now I'll remove the prose description. I'll submit a separate editorial change to clarify that offsetOption should be ignored unless offsetBehaviour is ~option~.

spec/zoneddatetime.html

spec/calendar.html

spec/plaindate.html

polyfill/lib/ecmascript.mjs

Introduce a new spec type, Time Zone Methods Record, which stores a time zone object's getOffsetNanosecondsFor and getPossibleInstantsFor methods once they have been observably looked up. The record is passed around into abstract operations instead of the time zone object itself. Most API operations that do time zone calculations need to look up both getOffsetNanosecondsFor and getPossibleInstantsFor unconditionally. Only in the case where we convert a plain time to an exact time, it's possible we only need to look up getPossibleInstantsFor, or neither (if the offset is supplied.) This is a large commit but most of it is mechanical replacement of Temporal.TimeZone variables with Time Zone Record variables.

Introduce a new spec type, Calendar Methods Record, which stores a calendar object's methods once they have been observably looked up. The record is passed around into abstract operations instead of the calendar object itself. The mechanism doesn't currently allow for storing all Temporal.Calendar methods, only the ones that are explicitly called internally in other places than the Zoned and Plain types' methods of the same name, as part of a different API call. Some methods not included in the record are only called by the same-named methods on other Temporal types (example, Calendar.p.dayOfWeek is only called by ZonedDateTime.p.dayOfWeek, PlainDateTime.p.dayOfWeek, and PlainDate.p.dayOfWeek.) Other methods not included in the record are potentially called through lookups in PrepareTemporalFields, so it's not possible to pre-fetch them without making polyfilling difficult. (Example, Calendar.p.monthCode is called in `plainYearMonth.toPlainDate(...)` by looking up the `plainYearMonth.monthCode` property in PrepareTemporalFields. Shortcutting that lookup would mean that PlainDate.p.monthCode could never be polyfilled.) This is a large commit but most of it is mechanical replacement of Temporal.Calendar variables with Calendar Methods Record variables.

This is an addendum to part 3 (#2671) that removes a ZonedDateTime to PlainDateTime conversion in one place where a fast path doesn't need the PlainDateTime. This is observable in the case where you add or subtract two Temporal.Durations that don't have any units higher than hours. Credit to Anba for spotting this. Closes: #2696

ptomato · 2023-11-06T22:52:46Z

Thanks for the review! We did get some last-minute improvements in there. I'm excited to merge this after almost 8 months! 🎉

This is to address a belated review comment from Richard on #2519. For each operation in which we need to observably look up time zone methods, make those lookups unconditional for every method that might be called. See: #2724

This is to address a belated review comment from Richard on #2519. For each operation in which we need to observably look up calendar methods, make those lookups unconditional for every method that might be called. See: #2724

This is to address a belated review comment from Richard on #2519. For each operation in which we need to observably look up time zone methods, make those lookups unconditional for every method that might be called. See: #2724

This is to address a belated review comment from Richard on #2519. For each operation in which we need to observably look up calendar methods, make those lookups unconditional for every method that might be called. See: #2724

This is to address a belated review comment from Richard on #2519. For each operation in which we need to observably look up time zone methods, make those lookups unconditional for every method that might be called. See: #2724

This is to address a belated review comment from Richard on #2519. For each operation in which we need to observably look up calendar methods, make those lookups unconditional for every method that might be called. See: #2724

This was an omission in #2519, a recently-merged normative change, that Anba spotted after the fact. Possibly a rebase error. Makes getOffsetStringFor consistent with getPlainDateTimeFor and getInstantFor. The reference code was already correct here. Closes: #2775

This is a very large change, as it not only removes Temporal.Calendar and Temporal.TimeZone, but also tries to eliminate any extra complexity due to no longer having to deal with user code calls for calendar and time zone calculations. Some of the things that are removed or simplified include: - No more Calendar Method Records and Time Zone Method Records - In many places, no need to pass around the user's original options bag - In many places, no need to pass around the user's original PlainDate or Instant; use epoch nanoseconds, ISO Date Records, and ISO Date-Time Records instead - No more copying the own properties of options bags - Most of the calendar and time zone operations are now infallible - The set of extra calendar fields that used to be returned by Temporal.Calendar.prototype.fields() is now static; so no need to have the complicated PrepareTemporalFields operation that returns a null- prototype object with own data properties that correspond to arbitrary user fields. Dates in calendar space can be represented by a Calendar Fields Record with known fields. - Much of the special-casing to avoid user calls that was added in #2519 and similar PRs is now unobservable and is removed. Closes: #2836 Closes: #2853 Closes: #2854

ptomato force-pushed the user-code-calls branch from 63c4b04 to a2f79ba Compare March 11, 2023 20:46

This was referenced Mar 30, 2023

Reorder endNs/endInstant computation in NanosecondsToDays #2529

Closed

DifferenceTemporalZonedDateTime: Move time zone equality check next to calendar check? #2533

Closed

Ms2ger approved these changes Apr 4, 2023

View reviewed changes

polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved

spec/duration.html Outdated Show resolved Hide resolved

ljharb reviewed Apr 4, 2023

View reviewed changes

spec/plainmonthday.html Outdated Show resolved Hide resolved

justingrant approved these changes Apr 4, 2023

View reviewed changes

justingrant mentioned this pull request Apr 13, 2023

CalendarFields returns Array instead of List for string calendars #2551

Closed

ptomato commented Apr 14, 2023

View reviewed changes

spec/duration.html Outdated Show resolved Hide resolved

ptomato mentioned this pull request Apr 18, 2023

Fixes to builtins-as-strings change #2553

Merged

ptomato mentioned this pull request Apr 22, 2023

Improvements to Temporal tests tc39/test262#3819

Merged

ptomato mentioned this pull request Apr 22, 2023

Editorial improvements #2562

Merged

ptomato force-pushed the user-code-calls branch from a2f79ba to 717d602 Compare April 22, 2023 01:23

ptomato force-pushed the user-code-calls branch 2 times, most recently from 5e0c619 to a656703 Compare April 27, 2023 18:10

ptomato commented May 2, 2023

View reviewed changes

spec/plainyearmonth.html Outdated Show resolved Hide resolved

ptomato mentioned this pull request May 3, 2023

Normative: Correctly respect largestUnit option in duration rounding #2571

Merged

gibson042 mentioned this pull request May 11, 2023

Editorial: Pass Duration Records rather than individual components #2290

Open

ptomato force-pushed the user-code-calls branch 3 times, most recently from f5540ce to c68dccc Compare May 13, 2023 00:56

ptomato force-pushed the user-code-calls branch from 584fd67 to ac190ac Compare October 10, 2023 22:56

ptomato mentioned this pull request Oct 10, 2023

AddDuration: Unnecessary call to GetPlainDateTimeFor #2696

Closed

ptomato force-pushed the user-code-calls branch from d54febf to 7ec6df3 Compare October 18, 2023 17:18

ptomato force-pushed the user-code-calls branch from 7ec6df3 to f77ca4e Compare October 26, 2023 17:52

cjtenny approved these changes Nov 6, 2023

View reviewed changes

ptomato added 4 commits November 6, 2023 14:36

Update test262

8234ec4

ptomato force-pushed the user-code-calls branch from f77ca4e to 8234ec4 Compare November 6, 2023 22:48

ptomato merged commit 49b3b77 into main Nov 6, 2023
5 checks passed

ptomato deleted the user-code-calls branch November 6, 2023 22:52

ptomato mentioned this pull request Nov 7, 2023

Data-driven exceptions in calendar method lookups #2724

Closed

ptomato mentioned this pull request Nov 10, 2023

Remove conditional lookups #2726

Merged

ptomato mentioned this pull request Nov 16, 2023

Operation steps perform too many observable Gets #2289

Closed

ptomato mentioned this pull request Feb 14, 2024

Fallbacks to DefaultMergeCalendarFields should be removed (or made unconditional) #2463

Closed

ptomato mentioned this pull request Feb 21, 2024

Temporal.TimeZone.prototype.getOffsetStringFor: Move CreateTimeZoneMethodsRecord after ToTemporalInstant? #2775

Closed

ptomato mentioned this pull request Mar 5, 2024

Fewer CalendarDateAdd/DateUntil calls during day-rounding w/ PlainDate #2791

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Audit of user-visible operations in Temporal #2519

Normative: Audit of user-visible operations in Temporal #2519

ptomato commented Mar 11, 2023 •

edited

Loading

codecov bot commented Mar 11, 2023 •

edited

Loading

ptomato commented Mar 11, 2023

justingrant left a comment

ptomato commented Apr 22, 2023

cjtenny left a comment

cjtenny Oct 24, 2023

ptomato Nov 6, 2023 •

edited

Loading

ptomato commented Nov 6, 2023

Normative: Audit of user-visible operations in Temporal #2519

Normative: Audit of user-visible operations in Temporal #2519

Conversation

ptomato commented Mar 11, 2023 • edited Loading

codecov bot commented Mar 11, 2023 • edited Loading

Codecov Report

ptomato commented Mar 11, 2023

justingrant left a comment

Choose a reason for hiding this comment

ptomato commented Apr 22, 2023

cjtenny left a comment

Choose a reason for hiding this comment

cjtenny Oct 24, 2023

Choose a reason for hiding this comment

ptomato Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

ptomato commented Nov 6, 2023

ptomato commented Mar 11, 2023 •

edited

Loading

codecov bot commented Mar 11, 2023 •

edited

Loading

ptomato Nov 6, 2023 •

edited

Loading