Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Fully filled in up to 30c187f4b7
- Deprecate `Date::new_from_iso`/`Date::to_iso` (unicode-org#7287)
- Optimize Hebrew and Julian calendars (unicode-org#7213)
- Optimize day/week diffing to use RDs (unicode-org#7308)
- Optimize `until` month and day calculation performance
- `icu_casemap`
- General changes only
- `icu_collections`
Expand Down
4 changes: 2 additions & 2 deletions components/calendar/benches/until.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ fn bench_calendar<C: Copy + Calendar>(
}
}

fn convert_benches(c: &mut Criterion) {
fn until_benches(c: &mut Criterion) {
let mut group = c.benchmark_group("until/years");
bench_all_calendars!(group, bench_calendar, DateDurationUnit::Years);
group.finish();
Expand All @@ -55,5 +55,5 @@ fn convert_benches(c: &mut Criterion) {
group.finish();
}

criterion_group!(benches, convert_benches);
criterion_group!(benches, until_benches);
criterion_main!(benches);
5 changes: 5 additions & 0 deletions components/calendar/src/cal/coptic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ impl DateFieldsResolver for Coptic {
13
}

#[inline]
fn min_months_from_inner(_start: Self::YearInfo, years: i64) -> i64 {
13 * years
}

#[inline]
fn extended_year_from_era_year_unchecked(
&self,
Expand Down
5 changes: 5 additions & 0 deletions components/calendar/src/cal/ethiopian.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ impl DateFieldsResolver for Ethiopian {
Coptic::months_in_provided_year(year)
}

#[inline]
fn min_months_from_inner(start: Self::YearInfo, years: i64) -> i64 {
Coptic::min_months_from_inner(start, years)
}

#[inline]
fn extended_year_from_era_year_unchecked(
&self,
Expand Down
11 changes: 11 additions & 0 deletions components/calendar/src/cal/hebrew.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,17 @@ impl DateFieldsResolver for Hebrew {
12 + year.keviyah.is_leap() as u8
}

#[inline]
fn min_months_from_inner(start: HebrewYear, years: i64) -> i64 {
// (7y+1)/19 is the number of leap years before year y. Compute the
// number of leap years in the start year, up to but not including the
// end year.
let count_leaps = |y: i64| (7 * y + 1).div_euclid(19);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this probably doesn't need to be micro-optimised, but:

this does two euclidean integer divisions, which is going to be branchy and expensive. try to rewrite this in terms of normal divisions. you know that the divisor is positive, so all the div_euclid does is subtract 1 if y is negative. but since you're computing a lower bound, you can just subtract 1 unconditionally and call it a day.

having done that, you might be able to go from two divisions to one by distributing over the subtraction correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but since you're computing a lower bound, you can just subtract 1 unconditionally and call it a day.

That makes this O(n) again: having min_months be a lower bound that is always close to the actual months value is part of the goal. As long as min_months is guaranteed to be at most a constant number of months off from months, the month search routine is constant time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I think this math can be far simpler: we just need to compute y * 235 / 19 (235 months in a 19 year metonic cycle)

This math produces a good lower bound: it's 12.38 months per year. At the "edge" of the calculation this will always be lower than the actual number of leap years when rounded down: the hebrew calendar has at least one leap year in every 3 year span anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes this O(n) again

1 is a constant though? this is only called once, so subtracting one too much is one extra loop execution

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually started with y * 235 / 19 but then I confused myself by reading Reingold (7y + 1 mod 19 < 7 == is_leap_year). Let me try going back to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in commit f3800d1.

let start_year = start.value.into();
let end_year = start_year + years;
12 * years + (count_leaps(end_year) - count_leaps(start_year)).abs()
}

#[inline]
fn extended_year_from_era_year_unchecked(
&self,
Expand Down
67 changes: 39 additions & 28 deletions components/calendar/src/calendar_arithmetic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,14 @@ pub(crate) trait DateFieldsResolver: Calendar {
12
}

/// The minimum number of months over `years` years, starting from the given year.
///
/// The default impl is for non-lunisolar calendars with 12 months!
#[inline]
fn min_months_from_inner(_start: Self::YearInfo, years: i64) -> i64 {
12 * years
}

/// Calculates the ordinal month for the given year and month code.
///
/// The default impl is for non-lunisolar calendars!
Expand Down Expand Up @@ -902,7 +910,7 @@ impl<C: DateFieldsResolver> ArithmeticDate<C> {

/// Implements the Temporal abstract operation `NonISODateUntil`.
///
/// This takes a duration (`self`) and a date (`other`), then returns a duration that, when
/// This takes two dates (`self` and `other`), then returns a duration that, when
/// added to `self`, results in `other`, with largest unit according to `options`.
pub(crate) fn until(
&self,
Expand Down Expand Up @@ -992,14 +1000,12 @@ impl<C: DateFieldsResolver> ArithmeticDate<C> {
let mut candidate_months = sign;

if options.largest_unit == Some(DateDurationUnit::Months) && min_years != 0 {
// Optimization: No current calendar supports years with month length < 12.
// If something is at least N full years away, it is also at least 12*N full months away.
//
// In the future we can introduce per-calendar routines that are better at estimating a month count.
//
// We only need to apply this optimization for largest_unit = Months. If the largest_unit is years then
// our candidate date is already pretty close and won't need more than 12 iterations to get there.
let min_months = min_years * 12;
// If largest_unit = Months, then compute the calendar-specific minimum number of
// months corresponding to min_years. For solar calendars, this is 12 * min_years.
// For the Hebrew calendar, a leap month is added for 7 out of 19 years. East Asian
// Calendars do not provide a specialized implementation of `min_months_from()`
// because it would be too expensive to calculate; they default to 12 * min_years.
Comment on lines +1004 to +1007
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: don't really have to list out all the different implementations of min_months_from_inner, this will just become stale

let min_months = self.min_months_from(min_years);
debug_assert!(!self.surpasses(
other,
DateDuration::from_signed_ymwd(years, min_months, 0, 0),
Expand Down Expand Up @@ -1038,28 +1044,33 @@ impl<C: DateFieldsResolver> ArithmeticDate<C> {
candidate_weeks += sign;
}
}
// 1. Let _days_ be 0.
// 1. Let _candidateDays_ be _sign_.
// 1. Repeat, while NonISODateSurpasses(_calendar_, _sign_, _one_, _two_, _years_, _months_, _weeks_, _candidateDays_) is *false*,
// 1. Set _days_ to _candidateDays_.
// 1. Set _candidateDays_ to _candidateDays_ + _sign_.
let mut days = 0;
// There is no pressing need to optimize candidate_days here: the early-return RD arithmetic
// optimization will be hit if the largest_unit is weeks/days, and if it is months or years we will
// arrive here with a candidate date that is at most 31 days off. We can run this loop 31 times.
let mut candidate_days = sign;
while !self.surpasses(
other,
DateDuration::from_signed_ymwd(years, months, weeks, candidate_days),
sign,
cal,
) {
days = candidate_days;
candidate_days += sign;
}

// Now that we have `years`, `months`, and `weeks`, we can compute
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I'm not 100% convinced this works

added() is not intended to be the inverse of until; and verifying which cases this is the case for is quite tricky. In particular, Constrain/Reject behavior makes this hard to reason about.

We considered doing this early on and decided against it. As documented in #7682 the primary plan for optimizing this part of the code is to optimize surpasses(): note that at the moment this code won't be called more than 31 times.

We can also reduce that with a binary search, which I was originally going t oimplement, and @robertbastian has some code for.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree -- for fields larger than Day, or if Overflow::Reject is used.

But for the Day field, everything "left" winds up in Day. Running a loop to find !surpasses(d) && surpasses(d+1), is functionally the same as doing a RD subtraction.

Stated another way, once we are at the bottom of until(), Year/Month/Week values are frozen. All we're doing is nailing down the Day field. This is now a linear space problem, and the RD subtraction is mathematically the same as doing the loop. added() is appropriate here not because it is the inverse of until(), but because it uses the same new_balanced and regulated day logic as surpasses().

Incidentally, I first tried to compute both Week and Day using RD subtraction. This failed spectacularly, breaking a bunch of tests. When I investigated why, I saw that RD subtraction would never work for Week/Day because of the Constrain logic for fields > Day. No such problem when dealing only with Day.

Note that all the date_arithmetic_snapshot.rs tests are passing, across all calendars. Invoked via cargo insta test -p icu_calendar --all-features --review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incidentally, I first tried to compute both Week and Day using RD subtraction. This failed spectacularly, breaking a bunch of tests. When I investigated why, I saw that RD subtraction would never work for Week/Day because of the Constrain logic for fields > Day. No such problem when dealing only with Day.

FWIW I got this to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit f3800d1: Reverted to follow the CalendarDateUntil spec in computing the Day field.

// `days` directly by subtracting RD values of the intermediate date
// (`self`` + YMW) and the target date (`other`).
#[allow(clippy::expect_used)] // added() cannot fail: years/months/weeks validated above
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added() has quite a few error cases, I'm not immediately convinced that this unwrap is safe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in commit 659278f.

let from = self
.added(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might be worth inlining added and removing unnecessary code, such as all the error paths that you say can't happen

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Attempted in commit 659278f. Created a copy of added() without the bounds checking, and assuming Constrains behavior.

Pro: No expect() required, because written to be infallible.

Con: Duplication of the added() logic.

I couldn't think of a clean way to not have the code duplicated. I guess as long as the NonISODateAdd algorithm is stable it might not be an issue, but it is a code smell.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The algorithm isn't stable yet.

DateDuration::from_signed_ymwd(years, months, weeks, 0),
cal,
DateAddOptions {
overflow: Some(Overflow::Constrain),
..Default::default()
},
)
.expect("Intermediate date should be valid")
.to_rata_die();
let to = other.to_rata_die();
let days = to - from;

// 1. Return ! CreateDateDurationRecord(_years_, _months_, _weeks_, _days_).
DateDuration::from_signed_ymwd(years, months, weeks, days)
}

/// The minimum number of months over `years` years, starting from `self.year()`.
pub(crate) fn min_months_from(self, years: i64) -> i64 {
C::min_months_from_inner(self.year(), years)
}
}

#[cfg(test)]
Expand Down
2 changes: 1 addition & 1 deletion components/calendar/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ mod unstable {
/// let mut options_days = options_default;
/// options_days.largest_unit = Some(DateDurationUnit::Days);
/// assert_eq!(
/// d1.try_until_with_options(&d2, options_default).unwrap(),
/// d1.try_until_with_options(&d2, options_days).unwrap(),
/// DateDuration::for_days(410)
/// );
///
Expand Down