Thread through caching in chinese based calendars #4411
Conversation
|
fyi @sffc . The bulk of the PR should still be reviewable, if the other PR lands feel free to start reviewing just tell me you're doing so. |
Manishearth
force-pushed
the
chinese-yearinfo
branch
3 times, most recently
from
24fb99b to
f823da1
Compare
|
Tests pass! @sffc this is ready for review, you can review the other PR first if you'd like, or everything at once |
Manishearth
force-pushed
the
chinese-yearinfo
branch
from
5af4569 to
368a106
Compare
| @@ -304,11 +297,13 @@ impl Calendar for Chinese { | |||
| fn day_of_year_info(&self, date: &Self::DateInner) -> types::DayOfYearInfo { | |||
| let prev_year = date.0 .0.year.saturating_sub(1); | |||
| let next_year = date.0 .0.year.saturating_add(1); | |||
| // TODO(#3933): we should maybe have this cached already | |||
| let prev_info = self.get_precomputed_data().load_or_compute_info(prev_year); | |||
There was a problem hiding this comment.
This is not a super hot code path (week of year) so maybe fine if it is a little slower
There was a problem hiding this comment.
(result from discussion: this is a hot code path due to ExtractedDateTimeInfo. we could change how that works if necessary, but it's a nontrivial refactor)
| ret[12] = next_new_year - 1; // last day of last month is the day before the next new year | ||
|
|
||
| let mut current_month_start = new_year; | ||
| let mut current_month_major_solar_term = major_solar_term_from_fixed::<C>(new_year); |
There was a problem hiding this comment.
Nit: I prefer not introducing new code that uses the astronomy functions directly. Prefer to use abstractions we already have such as days_in_month.
There was a problem hiding this comment.
That recomputes a lot of things. I think this is the best way to do this, and honestly easier to understand when debugging.
There was a problem hiding this comment.
ok, not super comfortable reviewing astronomy code that isn't a direct port from lisp but thanks for writing the unit tests below
| ret[12] = next_new_year - 1; // last day of last month is the day before the next new year | ||
|
|
||
| let mut current_month_start = new_year; | ||
| let mut current_month_major_solar_term = major_solar_term_from_fixed::<C>(new_year); |
There was a problem hiding this comment.
ok, not super comfortable reviewing astronomy code that isn't a direct port from lisp but thanks for writing the unit tests below
| let mut current_month_major_solar_term = major_solar_term_from_fixed::<C>(new_year); | ||
| let mut leap_month_index = None; | ||
| for i in 0u8..12 { | ||
| let next_month_start = new_moon_on_or_after::<C>((current_month_start + 28).as_moment()); |
There was a problem hiding this comment.
Question: does 28 work? I remember Reingold using various constants in different contexts
There was a problem hiding this comment.
Yeah, since on-or-after starts at midnight, so the potential off-by-one error is in the other direction in our favor. We know that next_month_start must be current_month_start + 29 or 30.
Progress on #3933
Builds on #4407.
This only caches the yearinfo, it does not yet load data from data sources. That may happen in a separate PR.