| |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| |
The prior commit (ea6e598) memoised efTransferPlan but left the SAME
occupiedByClass1/Or2 O(len(merged)) scan pattern in a second place:
transferIfImpededEF's own fallback path, called from computeEF once per
day for every class-1 candidate the (now cached) plan does not resolve
-- confirmed by CPU profile, not assumed: computeEF.func1 (the old
occupiedByRank closure) was ~62% of BenchmarkDays7EF's total time,
almost all of it inside buildCelebration, called from BOTH efTransferPlan
internally AND this second, uncached site.
Before optimising, established precisely what "occupied" depends on, per
the coordinator's warning that occupancy might genuinely mutate during
planning (efTransferPlan's own `claimed` map suggested as much). It does
not: occupiedByRank asks only "does some OTHER entry's ORIGINAL,
untransferred date (buildCelebration + celebrationDate, which never
considers a transfer) equal d" -- a pure function of (merged content,
year, sel) alone, identical to what the transfer-plan cache already
keys on. `claimed`, by contrast, genuinely mutates during one planning
pass (it tracks which TARGET days a transfer walk has already assigned)
and is NOT part of occupiedByRank's computation at all -- it remains
computed fresh inside efTransferPlan every call, untouched by this
change. These are independent, not the same thing wearing two names:
efTransferPlan's own forward-walk loop already checks both, plus a
third condition (the target day's own temporal class), as separate
disjuncts.
Given that, an occupancy INDEX -- not a second cache, and not a
"we already know the answer" shortcut derived from plan's absence (which
would have been correct for the transferIfImpededEF fallback's own
control flow ONLY by coincidence: it ignores the unconditional All Souls
Sunday-transfer special case that runs before the class-1 check on ANY
rank, so a shortcut skipping straight past it would misfire the moment a
user overlay retagged All Souls class-1, however unlikely on shipped
data) -- is the safe fix: buildEFOccupancyIndex does the same
merged-scan ONCE, into date -> []{slug, rank}, and .occupied does the
exact O(1)-ish lookup + tiny-list filter occupiedByRank always computed,
just precomputed. transferIfImpededEF's own signature, control flow and
All Souls handling are completely unchanged; only what its two closure
parameters read from changed.
The index shares the transfer-plan cache's existing key (year, Selection,
SHA-256 of merged) rather than adding a new one -- both are pure in
exactly those three inputs, built in the same pass, so one key correctly
covers both. Only the plan is copied per call (clonePlan); the index,
which can hold one entry per merged slug (~330 on shipped data), is
returned uncopied and documented immutable-after-construction -- safe
under Go's concurrent-read guarantee since nothing anywhere writes to a
returned index.
New TestEFOccupancyIndexDetectsSameDateClass1Collision covers a path
no existing test reached: two class-1 SANCTORAL entries sharing one
ORIGINAL date (St Joseph/the Annunciation, covered by the prior commit's
tests, collide via HOLY WEEK's temporal precedence on DIFFERENT dates,
never with each other). A synthetic overlay (Compute's own public API,
same style as the existing tests) puts two class-1 entries on the same
otherwise-ordinary date: alone, either is simply observed; both
together, RG 97/98 transfer both forward and the shared date reverts to
its temporal office. Mutation-proved: dropping the index's exceptSlug
self-exclusion (reverted after) made the test fail immediately -- every
class-1 entry saw itself in the index and wrongly self-impeded, even the
single-entry case. TestEFTransferPlanCacheConcurrentUse gained a third,
disjoint key exercising this same collision path from goroutines
alongside the two existing ones; `go test -race` on the whole package is
clean.
Benchmarked (interleaved before/after, same method throughout this
branch): BenchmarkDays7EF drops from ~46-48ms (the prior commit's own
plan-cache-only state) to ~15-18ms/op (allocs 208639 -> 98727, -53%;
bytes 16.1MB -> 4.2MB, -74%) -- roughly a further 3x, ~4.6x cumulative
against the original ~74ms. BenchmarkDaysWeek (OF, which never touches
any of this) is unaffected: ~4.9-5.9ms/op both before and after, with
byte-for-byte identical allocs/bytes in every run -- the ms-level
wobble is machine noise, not a regression.
A fresh CPU profile confirms the new remaining bottleneck precisely:
writeSortedFields/hashMergedForPlan (the cache key's own SHA-256 of
merged, ~330 entries) is now ~35% of total time, because it still runs
on EVERY day (7x/week) to know whether a call is a cache hit, even
though the work it gates now mostly isn't. Not fixed here: hoisting the
key computation itself up to mobile.Days's batch level (mirroring the
first commit on this branch, b6ee8f0) would need Compute's public
signature to accept a precomputed key, a bigger surface change than this
task's scope, reported rather than taken unilaterally.
Output identity re-verified: the same 492-case sweep (both forms, both
UI languages, all four corpora, the leap day/Triduum/Requiem/season-
boundary dates, and the three Joseph/Annunciation years) is
byte-identical (SHA-256-equal) before and after -- the same SHA-256 as
the prior commit's own sweep, confirming zero output drift across the
whole chain. go test ./... and make ci (both build tags,
oracle/differential suite included) are green; go test -race on the
whole internal/calendar package is clean.
|
|
|
computeEF calls efTransferPlan once per day, but the plan is pure in
(year, the merged sanctoral content, Selection) and identical for every
day sharing those three -- e.g. every date in one mobile.Days week.
Rebuilding it per day was a real cost: efTransferPlan walks every
merged entry looking for class-1 candidates, and for each one it
considers, occupiedByClass1/Or2 (an occurrence check it also uses)
walks merged AGAIN -- confirmed by CPU profile, not just by reading the
code (github.com/lukaszkasprzak/lectio/internal/calendar.computeEF.func1,
the occupiedByRank closure, at ~62% of BenchmarkDays7EF's total time
before this fix, almost all of it inside buildCelebration).
efTransferPlanCached (transfer_plan_cache.go) wraps efTransferPlan with
a small, bounded, thread-safe LRU (container/list + sync.Mutex, capped
at 64 entries -- gomobile may call in from multiple goroutines, and an
unbounded map keyed by year would grow as a user scrolls through
decades). The cache key is (year, Selection, a SHA-256 of merged's full
content): merged is a map, so it cannot be a map key field itself, and
Go's randomised map iteration order means two calls with identical
content can visit it differently, so the hash sorts slugs and, within
each entry, its Fields/Variant keys before hashing, and covers every
field of every entry -- not just Rank/Date, the ones efTransferPlan's
own read path happens to touch today, because which entries even
qualify as class-1 is itself computed from that data, and
occupiedByClass1/Or2 scan ALL of merged, not just the class-1 subset.
Selection is included even though EF date resolution ignores it today
(resolveDate never reads its sel parameter) -- keying on it costs
nothing (four small strings) and protects a future change from
silently poisoning a cache that never accounted for it. The returned
map is always a fresh copy (clonePlan), never the cached instance, so
sharing it across goroutines needs no further synchronisation.
Verified the key is complete rather than trusted: with the content hash
temporarily dropped from the key (mutation test, not committed),
TestEFTransferPlanCacheInvalidatesOnOverlay failed immediately -- a
plan warmed for the shipped 2008 calendar was wrongly served back for
the same year with a user overlay applied (the Annunciation suppressed,
which changes where the RG 96(a)/97/98 collision sends St Joseph: 31
March instead of 1 April, empirically confirmed against the pre-cache
code before the test was written). TestEFTransferPlanCacheInvalidatesOnYear
is a lighter companion covering the year field. TestEFTransferPlanCacheConcurrentUse
hammers the cache from 12 goroutines across two different keys and
reasserts correctness afterward; clean under `go test -race`.
Benchmarked (interleaved before/after, same method as the readings.Prepare
commit, to control for machine thermal drift): BenchmarkDays7EF drops
from ~48-51ms to ~32-35ms/op (allocs 253810 -> 208639, -18%; bytes
28.1MB -> 16.1MB, -43%), roughly a third faster. BenchmarkDaysWeek (OF,
which never calls efTransferPlan at all) is unaffected, ~3.5-4.1ms/op
both before and after -- within noise, confirming this change is
EF-only as intended.
EF remains well outside OF's range (~32ms vs ~4ms), and a fresh CPU
profile after this fix places the dominant remaining cost precisely: it
is the SAME occupiedByClass1/Or2 pattern, but living OUTSIDE
efTransferPlan -- transferIfImpededEF's own fallback path, called once
per day for every class-1 candidate NOT already resolved by the (now
cached) plan, i.e. the ordinarily-unimpeded ones (~15-20 of them),
each triggering another O(len(merged)) scan. That call site was not
part of what this task named, and memoising it is a materially
different change (it is keyed per-candidate, not once per day), so it
is reported here rather than folded into this commit.
Output identity re-verified: a 492-case sweep of mobile.Day/mobile.Days
(both forms, both UI languages, all four corpora, a leap day, the
Sacred Triduum, a Requiem day, Christmas/Pentecost/Assumption/All Souls
windows, and the three Joseph/Annunciation transfer-collision years
this fix specifically touches -- 2008, 2035, 2046) produced
byte-identical (SHA-256-equal) JSON before and after. go test ./... and
make ci (both build tags, oracle/differential suite included) are
green.
|
