-
Notifications
You must be signed in to change notification settings - Fork 264
Improve the performance of ArithmeticDate.until()
#7739
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -587,6 +587,13 @@ impl<R: Rules> DateFieldsResolver for EastAsianTraditional<R> { | |
| 12 + year.packed.leap_month().is_some() as u8 | ||
| } | ||
|
|
||
| #[inline] | ||
| fn min_months_from_inner(_start: Self::YearInfo, years: i64) -> i64 { | ||
| // A lunisolar leap month is inserted at least every 3 years. Reingold denies | ||
| // that the Chinese calendar determines leap years using the 19-year Metonic cycle. | ||
| 12 * years + (years / 3) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there are a two issues with this:
|
||
| } | ||
|
|
||
| #[inline] | ||
| fn extended_year_from_era_year_unchecked( | ||
| &self, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -134,6 +134,12 @@ impl DateFieldsResolver for Hebrew { | |||||||||||||||||||||||||||||||||||||||||
| 12 + year.keviyah.is_leap() as u8 | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| #[inline] | ||||||||||||||||||||||||||||||||||||||||||
| fn min_months_from_inner(_start: HebrewYear, years: i64) -> i64 { | ||||||||||||||||||||||||||||||||||||||||||
| // There are 7 leap years in every 19-year Metonic cycle. | ||||||||||||||||||||||||||||||||||||||||||
| 235 * years / 19 | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+139
to
+140
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Explain why 235 * years / 19 is a lower bound. I understand that it is the average. and you are flooring the value. We end up with:
Note: the Hebrew metonic cycle has leap years in 3, 6, 8, 11, 14, 17, and 19, i.e., leap year gaps of +3, +3, +2, +3, +3, +3, +2. I think your formula is the theoretical minimum bound. I think, no matter where I start in the metonic cycle, the number of months I get from your formula is always less than or equal to the actual number of months. For example, if I start in year 3 month M01 and add 5 years, I need to cross over 2 leap months, one in year 3 and one in year 6. Your formula agrees. The "worst case" is I think month M06 in year 8 to year 18, a span that has only 3 leap months. That is a 10-year difference, and your formula correctly predicts that there are a minimum of 3 leap months during that span. Please try to summarize why your bound is the theoretical minimum bound in a concise and compelling way.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do have the start of the hustification written in #7739 (comment)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I know this PR is closed/merged, but this seems like the right place for this comment.) My original plan was to use the start year parameter to tell exactly where we were in the cycle, using the |
||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| #[inline] | ||||||||||||||||||||||||||||||||||||||||||
| fn extended_year_from_era_year_unchecked( | ||||||||||||||||||||||||||||||||||||||||||
| &self, | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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! | ||
|
|
@@ -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, | ||
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| let min_months = self.min_months_from(min_years); | ||
robertbastian marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| debug_assert!(!self.surpasses( | ||
| other, | ||
| DateDuration::from_signed_ymwd(years, min_months, 0, 0), | ||
|
|
@@ -1038,6 +1044,7 @@ 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*, | ||
|
|
@@ -1057,9 +1064,15 @@ impl<C: DateFieldsResolver> ArithmeticDate<C> { | |
| days = candidate_days; | ||
| candidate_days += sign; | ||
| } | ||
|
|
||
| // 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)] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought: This is much better, but we could in principle be even tighter. The 19-year bound is too small, but there should be something between that and what you have here. It would be nice to leave open a low-priority issue to investigate and implement the theoretical minimal bound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was working on that math earlier but did not finish it.