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 CalendarDateAdd/DateUntil calls during day-rounding w/ PlainDate #2791

Closed
arshaw opened this issue Feb 29, 2024 · 4 comments
Closed

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

arshaw opened this issue Feb 29, 2024 · 4 comments
Milestone

Comments

@arshaw
Copy link
Contributor

arshaw commented Feb 29, 2024

To the extent we care about reducing calls to user-code, it's possible to not always get the Calendar's dateAdd and dateUntil if you know your not gonna need it for relative-rounding. The case where I've found it's unnecessarily being called is for when relativeTo is a PlainDate and the largest needed unit is <= day. You don't need to involve the calendar for that.

I've removed the relevant calls from the test262 tests (might not be comprehensive):
Branch: https://github.com/fullcalendar/test262/tree/temporal-fewer-calls-rounding-day-pd
Diff: tc39/test262@main...fullcalendar:test262:temporal-fewer-calls-rounding-day-pd

Affects more than just Duration::round. Affects the since/until methods too.

@arshaw arshaw changed the title Fewer Calls: Calendar dateAdd/dateUntil and rounding w/ PlainDate Fewer CalendarDateAdd/DateUntil calls during non-relative rounding w/ PlainDate Feb 29, 2024
@arshaw arshaw changed the title Fewer CalendarDateAdd/DateUntil calls during non-relative rounding w/ PlainDate Fewer CalendarDateAdd/DateUntil calls during day-rounding w/ PlainDate Feb 29, 2024
@ptomato
Copy link
Collaborator

ptomato commented Mar 5, 2024

There's a tradeoff between complexity in figuring out in advance whether you need to Get the method, and unnecessarily Getting it. For sure at one point #2519 was more complicated in this regard than what it currently is. I think it'd be informative if we could see what the effect on the spec text would be.

@arshaw
Copy link
Contributor Author

arshaw commented Mar 5, 2024

In the current code, the place where this decision should be made is very close to plainDateTimeOrRelativeToWillBeUsed. Not literally using that variable, but making a similar variable very near that position.

After that point in the code, RoundDuration, (BalanceTimeDuration or AdjustRoundedDurationDays), and BalanceDateDurationRelative all expect calendarRecs, but if the refactor that #2792 proposes happens, new functions will be written and it'll lead to a much more natural point at which to short-circuit.

I propose we reassess this issue after further exploring #2792

@ptomato ptomato added this to the Stage "3.5" milestone Apr 18, 2024
@ptomato
Copy link
Collaborator

ptomato commented Apr 18, 2024

After #2826, we can rewrite this to be an editorial change. At that point, none of the test262 tests should be affected. It looks like the only thing that changed in your diff above was user code calls.

@ptomato
Copy link
Collaborator

ptomato commented Sep 10, 2024

This is obsoleted by #2925 - since there is no longer a method to Get, I don't think there's anything left to do here.

@ptomato ptomato closed this as completed Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants