polyfill: optimize rounding durations to years #1390
Conversation
ryzokuken
added
non-prod-polyfill
Codecov Report
@@ Coverage Diff @@
## main #1390 +/- ##
===========================================
- Coverage 95.73% 49.01% -46.73%
===========================================
Files 19 18 -1
Lines 11147 4984 -6163
Branches 1806 1089 -717
===========================================
- Hits 10672 2443 -8229
- Misses 472 2130 +1658
- Partials 3 411 +408
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ryzokuken
force-pushed
the
duration/roundduration/years
branch
from
4743dca
to
406644d
Compare
spec/duration.html
Outdated
| @@ -1289,27 +1289,33 @@ <h1>RoundDuration ( _years_, _months_, _weeks_, _days_, _hours_, _minutes_, _sec | |||
| 1. Let _monthsWeeksInDays_ be ? DaysUntil(_yearsLater_, _yearsMonthsWeeksLater_). | |||
| 1. Set _relativeTo_ to _yearsLater_. | |||
| 1. Let _days_ be _days_ + _monthsWeeksInDays_.. | |||
| 1. Let _daysDuration_ be ? CreateTemporalDuration(0, 0, 0, _days_, 0, 0, 0, 0, 0, 0). | |||
| 1. Let _daysLater_ be ? Call(_dateAdd_, _calendar_, « _relativeTo_, _daysDuration_, _options_, %Temporal.PlainDate% »). | |||
There was a problem hiding this comment.
_options_ is currently undefined if _relativeTo_ is undefined, but an empty object otherwise; mind fixing that while you're here for correctness/consistency throughout RoundDuration?
There was a problem hiding this comment.
This is a bug in the spec. The polyfill throws if unit is "years" and there is no calendar (basically no relativeTo). Should I fix it here or in a second PR?
There was a problem hiding this comment.
Second PR is fine, thanks!
ryzokuken
force-pushed
the
duration/roundduration/years
branch
from
406644d
to
643816f
Compare
| 1. Let _fractionalYears_ be _years_ + _days_ / abs(_oneYearDays_). | ||
| 1. Set _years_ to ? RoundNumberToIncrement(_fractionalYears_, _increment_, _roundingMode_). | ||
| 1. Set _remainder_ to _fractionalYears_ - _years_. | ||
| 1. Set _months_, _weeks_, and _days_ to 0. | ||
| 1. Else if _unit_ is *"months"*, then | ||
| 1. Let _yearsMonths_ be ? CreateTemporalDuration(_years_, _months_, 0, 0, 0, 0, 0, 0, 0, 0). | ||
| 1. Let _yearsMonthsLater_ be ? Call(_dateAdd_, _calendar_, « _relativeTo_, _yearsMonths_, _options_, %Temporal.PlainDate%). | ||
| 1. Let _yearsMonthsLater_ be ? Call(_dateAdd_, _calendar_, « _relativeTo_, _yearsMonths_, _options_, %Temporal.PlainDate% »). |
There was a problem hiding this comment.
This isn't a blocker for this PR, but FYI these should also use CalendarDateAdd but previously didn't because we stored the function in dateAdd so as not to repeatedly observably Get it. This is one mark in favour of the eager-reading proposed in #1294 and I'll fix it up one way or the other when resolving that.
Rounding durations to years was an operation that was previously of the order O(Years), since it looped over the number of days. This commit optimizes that logic by deferring that to calendar-specific PlainDate arithmetic, essentially making it O(1). Fixes: tc39#1242
Port over polyfill optimizations done against tc39#1242 to the spec.
ryzokuken
force-pushed
the
duration/roundduration/years
branch
from
a4ccbc7
to
f1476d4
Compare
Rounding durations to years was an operation that was previously of the
order O(Years), since it looped over the number of days. This commit
optimizes that logic by deferring that to calendar-specific PlainDate
arithmetic, essentially making it O(1).
Fixes: #1242
/cc @cjtenny @ptomato @justingrant