Hebrew calendar fast paths + shared calendar helpers#2028
Conversation
Adds _CalendarProtocol.nextDate(after:matching:direction:) as an
optional fast-path hook (default returns nil). _CalendarHebrew
implements it for {month, day}, {month, weekday, weekdayOrdinal},
{month, weekday, weekOfMonth}, and time-only patterns. Adds
RecurrenceRule single-combination, multi-combination cartesian, and
negative-ordinal short-circuits.
Non-implementing calendars are unchanged (the default nil leaves
existing paths intact).
Benchmarks (debug, vs ICU-backed Hebrew baseline):
- nextThousandThanksgivings: ~250x faster
- nextThousandThursdaysInTheFourthWeekOfNovember: ~107x faster
- RecurrenceRuleThanksgivings: 19x faster
- RecurrenceRuleDailyWithTimes: ~8x faster
Suite C (HebrewRecurrenceRuleParityProbe): 13 tests, 392 rule shapes
x 2088 date comparisons, 0 divergences vs Foundation's ICU-backed
Hebrew calendar.
…ility Addresses PR swiftlang#1953 review feedback (comments swiftlang#6 + swiftlang#7) by introducing shared static helpers for time-unit constants, hash(into:), firstWeekday, minimumDaysInFirstWeek, copy(), and isDateInWeekend. _CalendarHebrew adopts the helpers in this commit; _CalendarGregorian adoption follows in a subsequent PR. The structural value: single source of truth for the shared logic, and ~85 lines of boilerplate skipped per future calendar port (Islamic / Persian / Coptic / Japanese / etc.). Hebrew's adoption demonstrates the pattern. Side effect: _CalendarHebrew.isDateInWeekend now matches _CalendarGregorian.isDateInWeekend exactly (resolves a fractional- second divergence the extraction surfaced).
|
|
||
| // Probe: commit to the fast loop if the calendar recognizes this pattern. | ||
| if validates && matchingPolicy == .nextTime && repeatedTimePolicy == .first, | ||
| calendar._calendarNextDate(after: start, matching: matchingComponents, direction: direction) != nil { |
There was a problem hiding this comment.
It looks like we don't really need to call this function. We're only calling it to see if the specific calendar implements this function.
If my understanding of this is correct, this is probably not the best way to do it because
- It's possible for the protocol witness to have a fast path, but returns
nilwith this given date. In that case we'd miss out the opportunity for the fast path - it's possible this function is actually expensive. So it seems wasteful to call it even when we don't really need the result
I can think of two ways to do it
- Separate this fast path into a separate conformance
- Add another variable for the witness to implement to signify if they opt into fast path or not
There was a problem hiding this comment.
Good point. Added supportsNextDateFastPath boolean property to _CalendarProtocol (default false). _CalendarHebrew returns true. The probe call is gone, check the boolean instead of calling nextDate speculatively.
|
|
||
| // Fast-path: ask the calendar directly. Returns nil for unrecognized patterns. | ||
| if matchingPolicy == .nextTime && repeatedTimePolicy == .first { | ||
| if let fast = _calendarNextDate(after: searchingDate, matching: matchingComponents, direction: direction) { |
There was a problem hiding this comment.
Another issue with using _calendarNextDate(after: searchingDate, matching: matchingComponents, direction: direction) to check if it's a fast path is that we will be doing repeated work again if this function returns nil. We don't know if it's nil because there's no fast path, or if this function genuinely has to return nil
There was a problem hiding this comment.
Fixed by the same change as #1. The boolean gates all fast path entry points, so _calendarNextDate is only called when we know the calendar implements it. A nil return now unambiguously means "no match in this direction."
| //===----------------------------------------------------------------------===// | ||
|
|
||
| /// Time-unit constants shared across calendar implementations. | ||
| internal enum _CalendarConstants { |
There was a problem hiding this comment.
Why do we need a new scope instead of just using struct Calendar?
There was a problem hiding this comment.
Moved to extension Calendar instead.
| return false | ||
| } | ||
|
|
||
| /// Expand `_DateComponentCombinations` into a flat array of single-valued |
There was a problem hiding this comment.
Like mentioned in https://github.com/swiftlang/swift-foundation/blob/main/CONTRIBUTION_GUIDELINE.md we don't wrap comments
| weeksOfYearIdx: woyIdx, | ||
| hoursIdx: hIdx, minutesIdx: miIdx, | ||
| secondsIdx: sIdx) else { return nil } | ||
| result.append(dc) |
There was a problem hiding this comment.
This looks extremely inefficient to me... for one thing, DateComponents is a pretty expensive struct to create in the first place because it has storage for all the fields. Also we're in a gigantic nested loop here. There has to be a better way to do this, right?
There was a problem hiding this comment.
Replaced the deep nested loop with an iterative expansion: build a single base DateComponents with all single valued axes pre-filled, then only iterate over axes that actually have multiple values, cloning and patching the base. For example, a rule with one month, one weekday, and two hours creates only two DateComponents (the base is cloned once per hour value) instead of running through all nesting levels and rebuilding from scratch each time
…tions Back-syncs upstream commit 0127031 (PR swiftlang#2028 review feedback): - supportsNextDateFastPath Bool opt-in property on _CalendarProtocol - _CalendarConstants moved to Calendar extension with _kSecondsIn* prefix - _expandedDateComponents refactored from 8-deep loops to axis-based - Fast-path entry conditions consolidated - Verbose doc comments trimmed Two local-only corrections that do NOT exist upstream: - Per-call probe added to Calendar.enumerateDates fast-path gate. - Per-call probe added to DatesByMatching.Iterator.init usesFastPath chain. Without the probes, Hebrew opts in via the Bool but returns nil for patterns it can't fast-path (year, era, weekOfYear, dayOfYear, weekday+day, etc.). Framework commits to fast loop and emits nil to the user. With the probes, framework falls through to the generic enumerate path for unsupported patterns. Restores per-pattern fall-through that the pre-v26 framework had naturally. Hebrew extension: nextDate now handles partial time patterns (hour-only, minute-only, second-only) via new nextTimeOfDayPeriodicMatch helper. Fixes 14 metonicCycle_nineteenYears divergences from Suite B. Verified: 211/211 tests in 15 suites pass; Suite C 0 divergences. Divergence tracking: backup/LOCAL_VS_UPSTREAM_DIVERGENCE.md is authoritative — must be preserved across all future back-syncs. Snapshots: - backup/v25-frozen-pre-v26/ — pre-v26 state. - backup/v26-pr2028-review-feedback/ — post-v26 state with README.
3 participants
Hebrew calendar fast paths + shared calendar helpers
Follow up to PR #1953 (Hebrew calendar port, merged as #2017).
Summary
Commit 1: Add Hebrew calendar fast paths + shared protocol method
Adds
_CalendarProtocol.nextDate(after:matching:direction:)as an optional fast path hook (default returns nil, existing paths unchanged for all other calendars)._CalendarHebrewimplements it for:{month, day}(annual recurrence, e.g. Hanukkah){month, weekday, weekdayOrdinal}(Nth weekday of month){month, weekday, weekOfMonth}(weekday in Nth week of month){hour, minute, second}Calendar_Enumerate.swiftprobes the fast path at init;Calendar_Recurrence.swiftadds single combination, multi combination cartesian, and negative ordinal translation short circuits forRecurrenceRule.Commit 2: Extract shared calendar helpers into
_CalendarConstants+_CalendarUtilityAddresses PR #1953 review feedback (comments #6 + #7). Introduces shared static helpers for time unit constants,
hash(into:),firstWeekday,minimumDaysInFirstWeek,copy(), andisDateInWeekend. Hebrew adopts them; Gregorian adoption deferred to a follow up PR.Performance
Measured on arm64 (M3 Max), release build,
swift-benchmark. Zero heap allocations in all cases.nextThousandHanukkahs(enumerate {month,day})dateComponents{year,month,day} (10K dates)roundTripDateComponents(10K dates)date(byAdding:)The 325× enumeration speedup comes from O(1) direct computation fast paths for common date matching patterns, bypassing the generic iterate and test framework entirely.
Testing
HebrewRecurrenceRuleParityProbe.swift: 13 tests, 392 rule shapes × 2,088 date comparisons, 0 divergences vs Foundation's ICU backed Hebrew calendar.Cross calendar safety
RecurrenceRuleshort circuits probe with a sentinel_calendarNextDatecall first; non Hebrew calendars bail before any expensive work.