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

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

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

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 from 8c1b2c9 to d3d856e Compare March 7, 2024 21:40
@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
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.

None yet

2 participants