Repository navigation
Fix quadratic performance on long SLA durations - #21
Merged
Merged
Conversation
Adds 28 new tests (tests/SLAEdgeCasesTest.php) covering previously untested behaviour: single-day string schedules, weekend-only schedules, multi-period schedules via andFrom, the Weekly agenda addTimePeriods/clearTimePeriods API, schedule effectiveFrom switches mid-duration, boundary conditions (start/end exactly at schedule edges), multi-week durations, overnight schedules, pause and holiday edge cases (outside window, full-window, multi-day, arrays, weekend holidays), the breaches() accessor, breach boundary semantics, and SLAPause/SLAHoliday day periods. Coverage: 96.3% -> 98.4%. The new overnight-schedule tests exposed a real bug: midnight-crossing schedules (e.g. 22:00 -> 02:00) silently produced zero SLA time because CarbonPeriod::create(start, end) with end < start yields an empty period. Weekly::toPeriods now keeps overnight periods as a single period spanning midnight, which the per-day overlap logic in SLA::calculate clips exactly. A 24/7 schedule (equal from/to times) is now also covered.
Calculating a duration across more than a month with pauses or holidays was quadratic in the number of days and pauses: for every day, every coverage period was diffed against ALL pause periods of the whole subject duration via spatie's subtractAll/overlapAll cross-product. At ~120 pauses this grew from 1s to 3s+ for a month, and a 180-day duration exhausted the 128MB memory limit. The daily loop now: - clips the SLA periods to those overlapping the current day before the overlap map and dedup reduce (was O(periods^2) per day over the full subject range) - clips pause periods to those overlapping the current day before the diff, and precomputes them once instead of per coverage period - sums each day's interval seconds directly instead of cascading an ever-growing interval Results (weekday schedule, 120 pauses): 30 days: 1061ms -> 30ms 60 days: 2133ms -> 46ms 90 days: 3098ms -> 63ms 180 days: memory exhaustion -> 149ms 365 days: n/a -> 318ms (and 419ms with 365 holidays) A pause exactly touching the coverage boundary no longer removes a boundary second (previously an artifact of diffing against non-overlapping pauses). Adds regression tests for year-long calculations with 365 holidays and weekly pauses, which previously died with a memory error.
…math The per-day loop still spent most of its time constructing CarbonPeriod objects and running spatie/period diffs for the coverage dedup and pause subtraction (447ms of a 367ms... dominant cost for 5 years), plus CarbonInterval cascade combining (152ms). Coverage is now clipped and merged as integer timestamp pairs (union of overlapping periods), and pauses are subtracted with ps-1/pe+1 boundary adjustments that reproduce spatie's closed-interval semantics exactly (all existing boundary-sensitive tests pass unchanged, including the 2-seconds-per-pause year test). Also removes the now-unused calculate_interval/combine_intervals helpers and the phpstan ignore for the removed diff() calls; the 'collapses intervals' test now asserts the same behaviour through the public API with overlapping schedule windows. Results (weekday schedule): 120 pauses, 30d: 31ms -> 11ms 120 pauses, 90d: 63ms -> 14ms 120 pauses, 365d: 222ms -> 36ms 365 holidays, 1yr: 243ms -> 52ms 730 holidays, 2yr: 495ms -> 103ms no pauses, 5yr: 367ms -> 161ms no pauses, 10yr: - -> 305ms
CarbonPeriod::create measured at ~28us per object vs 0.1us for a
timestamp pair; building 2610 of them for a 10-year calculation was
144ms of the 305ms total.
- AgendaInterface::toPeriods and Weekly::toPeriods now return
start/end Carbon pairs instead of CarbonPeriod objects (the only
consumer is SLA::calculate, which only reads the bounds). BC note:
direct toPeriods consumers now receive array{0,1} pairs.
- The daily loop no longer materialises the day list or clones Carbon
objects: it iterates the day period directly, computes day bounds
from integer timestamps (+86400 reproduces the previous
addHours(24) semantics exactly), and only touches Carbon for the
schedule lookup on the already-anchored day instance.
- The enabled-schedule lookup inlines the valid_from comparison with
unixes precomputed once per calculate() instead of parsing the
string every day.
Results:
120 pauses, 30d: 11ms -> 7ms
120 pauses, 365d: 36ms -> 15ms
365 holidays, 1yr: 52ms -> 27ms
no pauses, 5yr: 161ms -> 41ms
no pauses, 10yr: 305ms -> 86ms
- Remove the stale disclaimer that sla-timer is inefficient over periods longer than a month (fixed in #21: a year with daily holidays now calculates in ~30ms, 10-year spans in ~90ms) and replace it with a short Performance section - Document overnight (midnight-crossing) schedules and 24/7 coverage via equal from/to times in the schedule-building guide (newly working behaviour from #20)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the performance problem calculating SLA durations across more than a month when pauses or holidays are involved. No, this was not fixed by the earlier work — it's new in this PR.
Root cause
The daily loop in
SLA::calculate()ran against the entire subject duration every day:$period->diff(...$pause_periods)— spatie'ssubtractAll/overlapAlldoes an O(n×m) cross-product (120+ variadic args, 128-param signature)$p->diff(...$carry)) was O(periods²) per day over the full-range period listFix (four commits)
ps-1/pe+1boundaries reproduce spatie's closed-interval semantics exactly — all boundary tests unchanged)AgendaInterface::toPeriodsnow returns start/end Carbon pairs instead ofCarbonPeriod[](measured ~28µs per CarbonPeriod vs ~0.1µs per pair — 144ms of a 305ms 10-year calc was just constructing them). The daily loop iterates the day period directly with integer bounds (+86400reproduces the oldaddHours(24)semantics exactly), and thevalid_fromparse is precomputed once per call. BC note: directtoPeriods()consumers now receivearray{0: CarbonInterface, 1: CarbonInterface}pairs.Results (weekday schedule, 09:00–17:00)
The year-long daily-holiday case previously died with a 128MB memory error and now runs in ~27ms.
Behaviour notes
calculate_interval/combine_intervalsprivate helpers; thecollapses intervalstest now asserts the same behaviour through the public API with overlapping schedule windowsTests
Regression tests for year-long subjects: 365 daily holidays (was a memory fatal), 52 weekly pauses, unpaused year — 51 tests total, all green; PHPStan level 8 and Pint clean.