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

Editorial: Fewer GetOffsetNanosecondsFor calls during InterpretISODateTimeOffset (prefer/reject) #2789

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

arshaw
Copy link
Contributor

@arshaw arshaw commented Feb 27, 2024

It's possible to avoid the GetOffsetNanosecondsFor call. For each candidate instant, you can derive its offset by comparing it to the UTC-zoned year/month/day/etc. Applicable to offset:prefer/reject only.

Modified (and passing) test262:
tc39/test262@main...fullcalendar:test262:temporal-fewer-calls-offset-prefer-reject

@arshaw arshaw force-pushed the fewer-calls-offset-prefer-reject branch from f383a68 to 8c1b2c9 Compare February 27, 2024 01:36
Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (02a6d14) to head (177e3e1).
Report is 146 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2789   +/-   ##
=======================================
  Coverage   96.60%   96.60%           
=======================================
  Files          23       23           
  Lines       12312    12324   +12     
  Branches     2272     2272           
=======================================
+ Hits        11894    11906   +12     
  Misses        356      356           
  Partials       62       62           

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

@ptomato
Copy link
Collaborator

ptomato commented Feb 28, 2024

My personal opinion on this: at this point in the process I don't want to bring further optimizations of user calls to TC39. We already had a normative change last year to fix the most egregious ones. I'm happy to add optimizations in cases where we already requested and approved a normative change due to an outright bug, like #2758 or #2760, especially if you are taking the initiative and I don't have to come up with them myself! But I'm strongly in favour of no new normative changes, unless there is really a bug that gives actual wrong results.

It's certainly possible to squeeze more out of this, we can potentially keep auditing user code calls indefinitely, but it effectively pushes out the ship date in browsers by 2 months every time we do it. Custom calendars are a niche use case, in the normal string-calendar-ID case browsers can optimize all of these calls away because they're not observable, so to me it's just not worth it.

@arshaw Are you joining the Temporal meeting tomorrow? We can talk about this and hear from others.

@arshaw
Copy link
Contributor Author

arshaw commented Feb 28, 2024

@ptomato, could you please give me info about meeting time/invite? I'm not really in the loop.

@ptomato
Copy link
Collaborator

ptomato commented Feb 28, 2024

@arshaw My bad, I thought you had a standing invitation. The time and video link are the same as last time you attended. I believe they are also on the TC39 public calendar.

@arshaw arshaw changed the title Call GetOffsetNanosecondsFor fewer times for InterpretISODateTimeOffset prefer/reject Fewer Calls: InterpretISODateTimeOffset (prefer/reject) & GetOffsetNanosecondsFor Feb 29, 2024
@arshaw
Copy link
Contributor Author

arshaw commented Feb 29, 2024

thanks @ptomato , I'll be in the meeting, but 10 mins late (just got your email).

See this comment for an aggregation of all reduced-user-calls I found:
fullcalendar/temporal-polyfill#3 (comment)

@arshaw arshaw changed the title Fewer Calls: InterpretISODateTimeOffset (prefer/reject) & GetOffsetNanosecondsFor Fewer GetOffsetNanosecondsFor calls during InterpretISODateTimeOffset (prefer/reject) Feb 29, 2024
@arshaw arshaw force-pushed the fewer-calls-offset-prefer-reject branch 2 times, most recently from d3d856e to 177e3e1 Compare March 7, 2024 21:46
@ptomato
Copy link
Collaborator

ptomato commented Apr 18, 2024

After #2826 we should rewrite this to be an editorial PR. Marking as draft for now.

@ptomato ptomato marked this pull request as draft April 18, 2024 20:48
arshaw and others added 2 commits September 10, 2024 10:38
…Offset

Spec text algorithm corresponding to the previous commit.
@ptomato ptomato force-pushed the fewer-calls-offset-prefer-reject branch from 177e3e1 to 9bc5d04 Compare September 10, 2024 17:41
@ptomato ptomato marked this pull request as ready for review September 10, 2024 17:42
@ptomato ptomato changed the title Fewer GetOffsetNanosecondsFor calls during InterpretISODateTimeOffset (prefer/reject) Editorial: Fewer GetOffsetNanosecondsFor calls during InterpretISODateTimeOffset (prefer/reject) Sep 10, 2024
@ptomato
Copy link
Collaborator

ptomato commented Sep 10, 2024

Rebased. This is now an editorial change and doesn't require any test262 changes. I added the corresponding spec text.

@ptomato ptomato merged commit 5d67c89 into tc39:main Sep 10, 2024
5 checks passed
@ptomato ptomato deleted the fewer-calls-offset-prefer-reject branch September 10, 2024 17:45
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

Successfully merging this pull request may close these issues.

2 participants