Skip to content

Commit

Permalink
Temporal: Remove BigInt arithmetic and loops in BalanceDurationRelative
Browse files Browse the repository at this point in the history
A few results change because the algorithm previously used for rounding
didn't always add duration units to dates in RFC 5545 order, and we also
introduce a special case for rounding with largestUnit years or months and
smallestUnit weeks.
  • Loading branch information
ptomato committed Nov 16, 2023
1 parent 50abc12 commit e43e20a
Show file tree
Hide file tree
Showing 35 changed files with 331 additions and 579 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,11 @@ const relativeTo = new Temporal.ZonedDateTime(0n, timeZone, calendar);
// RoundDuration ->
// MoveRelativeZonedDateTime -> AddZonedDateTime -> calendar.dateAdd()
// MoveRelativeDate -> calendar.dateAdd()
// BalanceDurationRelative ->
// MoveRelativeDate -> calendar.dateAdd() (2x)
// calendar.dateAdd()
// BalanceDateDurationRelative -> calendar.dateAdd()

const instance1 = new Temporal.Duration(1, 1, 1, 1, 1);
instance1.round({ smallestUnit: "weeks", relativeTo });
assert.sameValue(calendar.dateAddCallCount, 5, "rounding with calendar smallestUnit");
assert.sameValue(calendar.dateAddCallCount, 3, "rounding with calendar smallestUnit");

// Rounding with a non-default largestUnit to cover the path in
// UnbalanceDurationRelative where larger units are converted into smaller
Expand All @@ -37,8 +35,7 @@ assert.sameValue(calendar.dateAddCallCount, 5, "rounding with calendar smallestU
// UnbalanceDurationRelative -> MoveRelativeDate -> calendar.dateAdd()
// RoundDuration ->
// MoveRelativeDate -> calendar.dateAdd() (5x)
// BalanceDurationRelative
// MoveRelativeDate -> calendar.dateAdd()
// BalanceDateDurationRelative -> calendar.dateAdd()
// MoveRelativeZonedDateTime -> AddZonedDateTime -> calendar.dateAdd()

calendar.dateAddCallCount = 0;
Expand All @@ -52,12 +49,10 @@ assert.sameValue(calendar.dateAddCallCount, 8, "rounding with non-default larges
// Duration.round() ->
// RoundDuration ->
// MoveRelativeZonedDateTime -> AddZonedDateTime -> calendar.dateAdd()
// BalanceDurationRelative ->
// MoveRelativeDate -> calendar.dateAdd() (2x)
// calendar.dateAdd()
// BalanceDateDurationRelative -> calendar.dateAdd()

calendar.dateAddCallCount = 0;

const instance3 = new Temporal.Duration(1, 1, 1, 1, 1);
instance3.round({ smallestUnit: "days", relativeTo });
assert.sameValue(calendar.dateAddCallCount, 4, "rounding with days smallestUnit");
assert.sameValue(calendar.dateAddCallCount, 2, "rounding with days smallestUnit");
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ features: [Temporal]
// should result in one call to dateUntil() originating from
// AdjustRoundedDurationDays, with largestUnit equal to the largest unit in
// the duration higher than "day".
// Additionally one call with largestUnit: "month" in BalanceDurationRelative
// when the largestUnit given to round() is "year".
// Additionally one call in BalanceDateDurationRelative with the same
// largestUnit.
// Other calls have largestUnit: "day" so the difference is taken in ISO
// calendar space.

Expand All @@ -99,9 +99,9 @@ TemporalHelpers.checkCalendarDateUntilLargestUnitSingular(
duration.round({ largestUnit, roundingIncrement: 2, roundingMode: 'ceil', relativeTo });
},
{
years: ["year", "month"],
months: ["month"],
weeks: ["week"],
years: ["year", "year"],
months: ["month", "month"],
weeks: ["week", "week"],
days: [],
hours: [],
minutes: [],
Expand All @@ -122,9 +122,9 @@ TemporalHelpers.checkCalendarDateUntilLargestUnitSingular(
duration.round({ largestUnit, relativeTo });
},
{
years: ["month", "month", "month", "month", "month", "month"],
months: ["month"],
weeks: [],
years: ["year"],
months: ["month", "month"],
weeks: ["week"],
days: [],
hours: [],
minutes: [],
Expand All @@ -147,8 +147,8 @@ TemporalHelpers.checkCalendarDateUntilLargestUnitSingular(
duration.round({ largestUnit, smallestUnit: largestUnit, relativeTo });
}, {
years: ["year"],
months: [],
weeks: [],
months: ["month"],
weeks: ["week"],
days: []
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ assert.throws(
"rounding a week Duration fails without largest/smallest unit"
);

TemporalHelpers.assertDuration(duration3.round(relativeToYears), 0, 0, 5, 0, 0, 0, 0, 0, 0, 0, "round week duration to years");
TemporalHelpers.assertDuration(duration3.round(relativeToMonths), 0, 0, 5, 0, 0, 0, 0, 0, 0, 0, "round week duration to months");
TemporalHelpers.assertDuration(duration3.round(relativeToYears), 0, 1, 0, 4, 0, 0, 0, 0, 0, 0, "round week duration to years");
TemporalHelpers.assertDuration(duration3.round(relativeToMonths), 0, 1, 0, 4, 0, 0, 0, 0, 0, 0, "round week duration to months");
TemporalHelpers.assertDuration(duration3.round(relativeToWeeks), 0, 0, 5, 0, 0, 0, 0, 0, 0, 0, "round week duration to weeks");
TemporalHelpers.assertDuration(duration3.round(relativeToDays), 0, 0, 0, 35, 0, 0, 0, 0, 0, 0, "round week duration to days");

Expand Down
35 changes: 11 additions & 24 deletions test/built-ins/Temporal/Duration/prototype/round/dateuntil-field.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
/*---
esid: sec-temporal.duration.prototype.round
description: >
When consulting calendar.dateUntil() to calculate the number of months in a
year, the months property is not accessed on the result Duration
When consulting calendar.dateUntil() to balance or unbalance a duration, no
properties are accessed on the result Duration
includes: [compareArray.js, temporalHelpers.js]
features: [Temporal]
---*/
Expand All @@ -16,36 +16,23 @@ class CalendarDateUntilObservable extends Temporal.Calendar {
dateUntil(...args) {
actual.push("call dateUntil");
const returnValue = super.dateUntil(...args);
TemporalHelpers.observeProperty(actual, returnValue, "years", Infinity);
TemporalHelpers.observeProperty(actual, returnValue, "months", Infinity);
TemporalHelpers.observeProperty(actual, returnValue, "weeks", Infinity);
TemporalHelpers.observeProperty(actual, returnValue, "days", Infinity);
return returnValue;
}
}

const calendar = new CalendarDateUntilObservable("iso8601");
const relativeTo = new Temporal.PlainDate(2018, 10, 12, calendar);

// One path, through UnbalanceDateDurationRelative, calls dateUntil()

const expected1 = [
"call dateUntil",
const expected = [
"call dateUntil", // UnbalanceDateDurationRelative
"call dateUntil", // BalanceDateDurationRelative
];

const years = new Temporal.Duration(2);
const result1 = years.round({ largestUnit: "months", relativeTo });
TemporalHelpers.assertDuration(result1, 0, 24, 0, 0, 0, 0, 0, 0, 0, 0, "result");
assert.compareArray(actual, expected1, "operations");

// There is a second path, through BalanceDurationRelative, that calls
// dateUntil() in a loop for each year in the duration plus one extra time

actual.splice(0); // reset calls for next test
const expected2 = [
"call dateUntil",
"call dateUntil",
"call dateUntil",
];

const months = new Temporal.Duration(0, 24);
const result2 = months.round({ largestUnit: "years", relativeTo });
TemporalHelpers.assertDuration(result2, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, "result");
assert.compareArray(actual, expected2, "operations");
const result = years.round({ largestUnit: "months", relativeTo });
TemporalHelpers.assertDuration(result, 0, 24, 0, 0, 0, 0, 0, 0, 0, 0, "result");
assert.compareArray(actual, expected, "operations");
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ features: [Temporal]

// Based on a test case by André Bargull <andre.bargull@gmail.com>

// Note: February in a leap year.
const relativeTo = new Temporal.PlainDate(1972, 2, 1);
// Note: One day after February in a leap year.
const relativeTo = new Temporal.PlainDate(1972, 3, 1);

const options = {
largestUnit: "years",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,8 @@ const expectedOpsForYearRounding = expectedOpsForPlainRelativeTo.concat([
"call options.relativeTo.calendar.dateAdd", // 7.s MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 7.y MoveRelativeDate
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateAdd", // 11.c MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 11.g MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 11.k
"call options.relativeTo.calendar.dateUntil", // 11.p
"call options.relativeTo.calendar.dateAdd", // 9.c
"call options.relativeTo.calendar.dateUntil", // 9.d
]);
const instanceYears = new Temporal.Duration(1, 12, 0, 0, /* hours = */ 2400);
instanceYears.round(createOptionsObserver({ smallestUnit: "years", relativeTo: plainRelativeTo }));
Expand All @@ -159,8 +157,9 @@ const expectedOpsForMonthRounding = expectedOpsForPlainRelativeTo.concat([
"call options.relativeTo.calendar.dateAdd", // 10.e
"call options.relativeTo.calendar.dateAdd", // 10.k MoveRelativeDate
], Array(2).fill("call options.relativeTo.calendar.dateAdd"), [ // 2× 10.n.iii MoveRelativeDate
// BalanceDurationRelative
"call options.relativeTo.calendar.dateAdd",
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateAdd", // 10.d
"call options.relativeTo.calendar.dateUntil", // 10.e
]);
const instance2 = new Temporal.Duration(1, 0, 0, 62);
instance2.round(createOptionsObserver({ largestUnit: "months", smallestUnit: "months", relativeTo: plainRelativeTo }));
Expand All @@ -174,8 +173,9 @@ const expectedOpsForWeekRounding = expectedOpsForPlainRelativeTo.concat([
// RoundDuration
"call options.relativeTo.calendar.dateAdd", // 11.d MoveRelativeDate
], Array(58).fill("call options.relativeTo.calendar.dateAdd"), [ // 58× 11.g.iii MoveRelativeDate (52 + 4 + 2)
// BalanceDurationRelative
"call options.relativeTo.calendar.dateAdd", // 12.c
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateAdd", // 16
"call options.relativeTo.calendar.dateUntil", // 17
]);
const instance3 = new Temporal.Duration(1, 1, 0, 15);
instance3.round(createOptionsObserver({ largestUnit: "weeks", smallestUnit: "weeks", relativeTo: plainRelativeTo }));
Expand All @@ -191,14 +191,10 @@ instance4.round(createOptionsObserver({ largestUnit: "days", smallestUnit: "days
assert.compareArray(actual, expectedOpsForDayRounding, "order of operations with largestUnit = smallestUnit = days");
actual.splice(0); // clear

// code path through BalanceDurationRelative balancing from days up to years:
// code path through BalanceDateDurationRelative balancing from days up to years:
const expectedOpsForDayToYearBalancing = expectedOpsForPlainRelativeTo.concat([
"call options.relativeTo.calendar.dateAdd", // 10.b MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 10.e.iv MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 10.f MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 10.i.iv MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 10.j
"call options.relativeTo.calendar.dateUntil", // 10.n
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateUntil", // 9.d
]);
const instance5 = new Temporal.Duration(0, 0, 0, 0, /* hours = */ 396 * 24);
instance5.round(createOptionsObserver({ largestUnit: "years", smallestUnit: "days", relativeTo: plainRelativeTo }));
Expand All @@ -211,33 +207,27 @@ const expectedOpsForMonthToYearBalancing = expectedOpsForPlainRelativeTo.concat(
"call options.relativeTo.calendar.dateAdd", // 10.c
"call options.relativeTo.calendar.dateAdd", // 10.e
"call options.relativeTo.calendar.dateAdd", // 10.k MoveRelativeDate
// BalanceDurationRelative
"call options.relativeTo.calendar.dateAdd", // 10.b MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 10.f MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 10.j
"call options.relativeTo.calendar.dateUntil", // 10.n
"call options.relativeTo.calendar.dateAdd", // 10.p.iv
"call options.relativeTo.calendar.dateUntil", // 10.p.vii
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateAdd", // 9.c
"call options.relativeTo.calendar.dateUntil", // 9.d
]);
const instance6 = new Temporal.Duration(0, 12);
instance6.round(createOptionsObserver({ largestUnit: "years", smallestUnit: "months", relativeTo: plainRelativeTo }));
assert.compareArray(actual, expectedOpsForMonthToYearBalancing, "order of operations with largestUnit = years, smallestUnit = months");
actual.splice(0); // clear

const expectedOpsForDayToMonthBalancing = expectedOpsForPlainRelativeTo.concat([
// BalanceDurationRelative
"call options.relativeTo.calendar.dateAdd", // 11.b MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 11.e.iv MoveRelativeDate
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateUntil", // 10.e
]);
const instance7 = new Temporal.Duration(0, 0, 0, 0, /* hours = */ 32 * 24);
instance7.round(createOptionsObserver({ largestUnit: "months", smallestUnit: "days", relativeTo: plainRelativeTo }));
assert.compareArray(actual, expectedOpsForDayToMonthBalancing, "order of operations with largestUnit = months, smallestUnit = days");
actual.splice(0); // clear

const expectedOpsForDayToWeekBalancing = expectedOpsForPlainRelativeTo.concat([
// BalanceDurationRelative
"call options.relativeTo.calendar.dateAdd", // 12.c MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 12.f.iv MoveRelativeDate
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateUntil", // 17
]);
const instance8 = new Temporal.Duration(0, 0, 0, 0, /* hours = */ 8 * 24);
instance8.round(createOptionsObserver({ largestUnit: "weeks", smallestUnit: "days", relativeTo: plainRelativeTo }));
Expand Down Expand Up @@ -403,10 +393,8 @@ const expectedOpsForYearRoundingZoned = expectedOpsForZonedRelativeTo.concat([
"call options.relativeTo.calendar.dateAdd", // 7.s MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 7.y MoveRelativeDate
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateAdd", // 11.c MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 11.g MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 11.k
"call options.relativeTo.calendar.dateUntil", // 11.p
"call options.relativeTo.calendar.dateAdd", // 9.c
"call options.relativeTo.calendar.dateUntil", // 9.d
]);
instanceYears.round(createOptionsObserver({ smallestUnit: "years", relativeTo: zonedRelativeTo }));
assert.compareArray(
Expand All @@ -431,12 +419,35 @@ const expectedOpsForUnbalanceRoundBalance = expectedOpsForZonedRelativeTo.concat
// RoundDuration
"call options.relativeTo.calendar.dateAdd", // 8.g MoveRelativeDate
// BalanceDateDurationRelative
"call options.relativeTo.calendar.dateAdd", // 13.c MoveRelativeDate
"call options.relativeTo.calendar.dateAdd", // 10.d
"call options.relativeTo.calendar.dateUntil", // 10.e
]);
new Temporal.Duration(0, 1, 1).round(createOptionsObserver({ largestUnit: "months", smallestUnit: "weeks", relativeTo: zonedRelativeTo }));
new Temporal.Duration(0, 1, 1).round(createOptionsObserver({ largestUnit: "years", smallestUnit: "weeks", relativeTo: zonedRelativeTo }));
assert.compareArray(
actual,
expectedOpsForUnbalanceRoundBalance,
"order of operations with largestUnit = years, smallestUnit = weeks, and ZonedDateTime relativeTo"
);
actual.splice(0); // clear

// code path that skips user code calls in BalanceDateDurationRelative due to
// special case for largestUnit months and smallestUnit weeks
const expectedOpsForWeeksSpecialCase = expectedOpsForZonedRelativeTo.concat([
// ToTemporalDate
"call options.relativeTo.timeZone.getOffsetNanosecondsFor",
// lookup in Duration.p.round
"get options.relativeTo.calendar.dateAdd",
"get options.relativeTo.calendar.dateUntil",
// RoundDuration → MoveRelativeZonedDateTime → AddZonedDateTime
"call options.relativeTo.calendar.dateAdd",
"call options.relativeTo.timeZone.getPossibleInstantsFor", // 13. GetInstantFor
// RoundDuration
"call options.relativeTo.calendar.dateAdd", // 14.p MoveRelativeDate
]);
new Temporal.Duration(0, 1, 1).round(createOptionsObserver({ largestUnit: "months", smallestUnit: "weeks", relativeTo: zonedRelativeTo }));
assert.compareArray(
actual,
expectedOpsForWeeksSpecialCase,
"order of operations with largestUnit = months, smallestUnit = weeks, and ZonedDateTime relativeTo"
);
actual.splice(0); // clear

This file was deleted.

Loading

0 comments on commit e43e20a

Please sign in to comment.