From b6ee8f09ad2c0bc9e83394e2427de861eb1e2a72 Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Mon, 24 Aug 2026 16:35:01 +0200 Subject: perf(readings): stack calendar layers and load the book table once per Days call mobile.Days (dlectio's 7-day calendar view) called readings.Load once per date. Load's offlineLoad re-ran caldata.Stack (parses the embedded calendar INI, plus any user calendar layer) and bible.LoadBookTable (parses the embedded books.ini, plus any user override) on every call, even though neither depends on the date -- only on cfg.Selection().Form and cfg.Use, which mobile.Days holds fixed across its whole loop. Verified before changing anything: isolated benchmarks put Stack+ LoadBookTable at ~1.1ms/call (OF) and ~1.6ms/call (EF) -- real INI parsing and, for a user override, file I/O -- and for the OF form that redundant work was ~67% of a 7-day view's total time. Load gains a Prepared value (the layer stack + book table) and a Prepare/LoadWith pair: Prepare builds a Prepared once, LoadWith reuses it across dates. Load itself is unchanged, still calling Prepare on every invocation -- every existing caller keeps its current behaviour untouched. mobile.Days now Prepares once and loops LoadWith; mobile.Day picks up the same fix for its own two same-cfg Load calls. Benchmarked (interleaved before/after pairs, to control for machine thermal drift): the OF 7-day view (BenchmarkDaysWeek) drops from ~11.5ms to ~6.4ms/op (allocs 53359 -> 16347, -69%), an EF 7-day view (BenchmarkDays7EF) from ~74ms to ~66-73ms/op (allocs 299204 -> 253815, -15%; the EF form's per-day calendar computation dominates its total cost far more than OF's does, so the fix's share of the win is smaller there), and a single Day call (BenchmarkDay1) from ~4.3ms to ~2.9ms/op (allocs 15357 -> 9192, -40%). Output identity verified separately (not part of this diff): a 288-case sweep of mobile.Day/mobile.Days across both forms, both UI languages, all four corpora, a leap day, the Sacred Triduum, a Requiem day, and windows spanning Christmas/Pentecost/Assumption/All Souls produced byte-identical (SHA-256-equal) JSON before and after this change. New TestLoadWithAgreesWithLoad asserts LoadWith(Prepare(cfg), cfg, opts) equals Load(cfg, opts) for every case, so the fast path cannot silently drift from the slow one. go test ./... and make ci (both build tags, oracle/differential suite included) are green. --- internal/readings/offline.go | 49 +++++++++++++++++++++++++++++++++----- internal/readings/readings.go | 16 ++++++++++++- internal/readings/readings_test.go | 45 ++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 7 deletions(-) (limited to 'internal') diff --git a/internal/readings/offline.go b/internal/readings/offline.go index b2c16df..546d3b0 100644 --- a/internal/readings/offline.go +++ b/internal/readings/offline.go @@ -13,6 +13,33 @@ import ( "github.com/lukaszkasprzak/lectio/internal/naming" ) +// Prepared holds the date-independent setup offlineLoad otherwise redoes on +// every call: the stacked calendar layers (caldata.Stack) and the book table +// (bible.LoadBookTable). Both depend only on cfg -- never on the date -- so a +// caller resolving many dates against the same cfg (mobile.Days's 7-day loop, +// an eventual month view) should build one Prepared with Prepare and reuse it +// via LoadWith for every date, instead of paying Stack's INI parsing and +// LoadBookTable's file read + parse once per date. See Prepare and LoadWith. +type Prepared struct { + layers []calendar.Layer + tbl *bible.BookTable +} + +// Prepare builds a Prepared for cfg: the layer stack for cfg.Selection().Form +// stacked with cfg.Use (caldata.Stack), and the book table for the user's +// books.ini override, if any (bible.LoadBookTable). Both calls already +// tolerate their own failure (Stack falls back to the embedded calendar on a +// bad user layer; LoadBookTable falls back to the embedded book table on a +// bad user override) exactly as offlineLoad always has -- Prepare changes +// only when this work happens, never what it computes or how it degrades. +func Prepare(cfg config.Config) Prepared { + sel := cfg.Selection() + dir, _ := config.CalendarsDir() + layers, _ := caldata.Stack(sel.Form, dir, cfg.Use) // Stack falls back to embedded data on error + tbl, _ := bible.LoadBookTable(config.UserBooksINI()) // nil on error -> citations shown as authored + return Prepared{layers: layers, tbl: tbl} +} + // offlineLoad resolves a day's readings entirely from the embedded calendar // engine and lectionary data -- no network. It returns the same source-agnostic // liturgy.Section / liturgy.DayInfo the CLI/TUI/web already render, so the daily @@ -20,18 +47,28 @@ import ( // lectio's English-canonical authored form; the render localises each one to // the chosen corpus's Psalter and the user's sigla dialect (see // render.GatherVersion, bible.OFRef). +// +// offlineLoad is Prepare(cfg) followed by offlineLoadWith -- a single call's +// worth of convenience for Load, which has no date to amortize Prepare's cost +// over. A caller with several dates should call Prepare once and use +// offlineLoadWith/LoadWith directly instead (see Prepared's doc comment). func offlineLoad(cfg config.Config, date string) ([]liturgy.Section, liturgy.DayInfo, error) { + return offlineLoadWith(Prepare(cfg), cfg, date) +} + +// offlineLoadWith is offlineLoad, given an already-built Prepared instead of +// building its own. Computing the day itself (calendar.Compute, the readings +// it resolves) still happens once per call, exactly as before -- only the +// layer stack and book table are reused. +func offlineLoadWith(p Prepared, cfg config.Config, date string) ([]liturgy.Section, liturgy.DayInfo, error) { d, err := time.Parse("2006-01-02", date) if err != nil { return nil, liturgy.DayInfo{}, fmt.Errorf("bad date %q (want YYYY-MM-DD)", date) } sel := cfg.Selection() - dir, _ := config.CalendarsDir() - layers, _ := caldata.Stack(sel.Form, dir, cfg.Use) // Stack falls back to embedded data on error - day := calendar.Compute(d.UTC(), sel, layers) - rs := caldata.Readings(sel, layers, d.UTC(), day) - tbl, _ := bible.LoadBookTable(config.UserBooksINI()) // nil on error -> citations shown as authored - return sectionsFor(rs, sel.Form, cfg.UILanguage, cfg.SiglaLang(), tbl), dayInfo(cfg, day), nil + day := calendar.Compute(d.UTC(), sel, p.layers) + rs := caldata.Readings(sel, p.layers, d.UTC(), day) + return sectionsFor(rs, sel.Form, cfg.UILanguage, cfg.SiglaLang(), p.tbl), dayInfo(cfg, day), nil } // citationForms renders a reading's authored (English) citation into its diff --git a/internal/readings/readings.go b/internal/readings/readings.go index d0d7bf9..29eff5a 100644 --- a/internal/readings/readings.go +++ b/internal/readings/readings.go @@ -25,8 +25,22 @@ type Options struct { // liturgical colour -- see liturgy.DayInfo) for the configured form // (cfg.Lectionary: "traditional" or "new") and applies part filtering. Every // reading is resolved offline from the embedded calendar and lectionary data. +// +// Load is LoadWith(Prepare(cfg), cfg, opts) -- a single call's worth of +// convenience. A caller resolving several dates against the same cfg (a +// week/month view) should call Prepare once and use LoadWith directly instead +// of paying Prepare's cost on every date; see Prepared's doc comment +// (internal/readings/offline.go). func Load(cfg config.Config, opts Options) ([]liturgy.Section, liturgy.DayInfo, error) { - secs, info, err := offlineLoad(cfg, opts.Date) + return LoadWith(Prepare(cfg), cfg, opts) +} + +// LoadWith is Load, given an already-built Prepared (see Prepare) instead of +// building its own. Reuse one Prepared across every date resolved against the +// same cfg to skip re-stacking the calendar layers and re-parsing the book +// table per date -- the fast path mobile.Days's multi-day loop uses. +func LoadWith(p Prepared, cfg config.Config, opts Options) ([]liturgy.Section, liturgy.DayInfo, error) { + secs, info, err := offlineLoadWith(p, cfg, opts.Date) if err != nil { return nil, liturgy.DayInfo{}, err } diff --git a/internal/readings/readings_test.go b/internal/readings/readings_test.go index 1674d85..c81e324 100644 --- a/internal/readings/readings_test.go +++ b/internal/readings/readings_test.go @@ -1,6 +1,7 @@ package readings import ( + "reflect" "strings" "testing" @@ -136,6 +137,50 @@ func TestSundayRankIsDisplayOnly(t *testing.T) { } } +// TestLoadWithAgreesWithLoad guards the fast path multi-date callers (e.g. +// mobile.Days) use to avoid re-stacking the calendar layers and re-parsing +// the book table once per date: LoadWith, given a cfg's own Prepared value, +// must return exactly what Load(cfg, opts) returns, for every date and both +// forms. This is the correctness backstop for the perf fix -- Prepare only +// hoists WHEN the date-independent setup happens, never WHAT it computes. +func TestLoadWithAgreesWithLoad(t *testing.T) { + dates := []string{ + "2026-07-22", // ordinary weekday + "2028-02-29", // leap day + "2026-04-09", // Holy Thursday 2026 + "2026-04-10", // Good Friday 2026 + "2026-04-11", // Holy Saturday 2026 + "2026-11-02", // All Souls (a Requiem day) + "2026-12-25", // Christmas + } + for _, lect := range []string{"new", "traditional"} { + cfg := config.Config{Lectionary: lect} + p := Prepare(cfg) + for _, date := range dates { + opts := Options{Date: date, All: true} + wantSecs, wantInfo, wantErr := Load(cfg, opts) + gotSecs, gotInfo, gotErr := LoadWith(p, cfg, opts) + if (wantErr == nil) != (gotErr == nil) { + t.Fatalf("%s %s: Load err=%v, LoadWith err=%v", lect, date, wantErr, gotErr) + } + if wantErr != nil { + continue + } + if gotInfo != wantInfo { + t.Errorf("%s %s: LoadWith info = %+v, want %+v", lect, date, gotInfo, wantInfo) + } + if len(gotSecs) != len(wantSecs) { + t.Fatalf("%s %s: LoadWith %d sections, want %d", lect, date, len(gotSecs), len(wantSecs)) + } + for i := range wantSecs { + if !reflect.DeepEqual(gotSecs[i], wantSecs[i]) { + t.Errorf("%s %s: section %d = %+v, want %+v", lect, date, i, gotSecs[i], wantSecs[i]) + } + } + } + } +} + // TestLoadTraditional computes the Extraordinary Form day offline: it never // needs the network, and yields the EF epistle+gospel with a header name. func TestLoadTraditional(t *testing.T) { -- cgit v1.3 From ea6e59862dbdb607b9bcb221212a85ee3e4bd84a Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Mon, 24 Aug 2026 21:42:48 +0200 Subject: perf(calendar): memoise the EF transfer plan across a shared calendar 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. --- internal/calendar/calendar.go | 9 +- internal/calendar/transfer_plan_cache.go | 187 +++++++++++++++++++++++++ internal/calendar/transfer_plan_cache_test.go | 191 ++++++++++++++++++++++++++ 3 files changed, 386 insertions(+), 1 deletion(-) create mode 100644 internal/calendar/transfer_plan_cache.go create mode 100644 internal/calendar/transfer_plan_cache_test.go (limited to 'internal') diff --git a/internal/calendar/calendar.go b/internal/calendar/calendar.go index d918547..00048b7 100644 --- a/internal/calendar/calendar.go +++ b/internal/calendar/calendar.go @@ -67,7 +67,14 @@ func computeEF(date time.Time, sel Selection, layers []Layer) LiturgicalDay { // RG 97/98: the year's impeded I-class transfers are resolved as a set, // not one at a time, so two feasts impeded by the same early Easter cannot // both claim the same free day and lose one of themselves. - plan := efTransferPlan(year, merged, sel, occ1, occ1Or2) + // + // efTransferPlan is pure in (year, merged content, sel) and identical for + // every day of the year it is asked about -- computeEF runs once PER DAY, + // so a multi-day view (mobile.Days's week, a month view) was rebuilding + // it from scratch on every single one. efTransferPlanCached memoises it; + // see transfer_plan_cache.go for the cache key and why each of its three + // parts is load-bearing. + plan := efTransferPlanCached(year, merged, sel, occ1, occ1Or2) cands := []candidate{{Cel: td.Cel, Temporal: true, Season: td.Season, Sunday: td.Sunday}} for slug, rc := range merged { cel := buildCelebration(slug, rc) diff --git a/internal/calendar/transfer_plan_cache.go b/internal/calendar/transfer_plan_cache.go new file mode 100644 index 0000000..fdf62f7 --- /dev/null +++ b/internal/calendar/transfer_plan_cache.go @@ -0,0 +1,187 @@ +package calendar + +import ( + "container/list" + "crypto/sha256" + "io" + "sort" + "sync" + "time" +) + +// efTransferPlanCacheCap bounds the memoised EF transfer-plan cache (below) +// so a long-running gomobile process cannot grow it without bound as a user +// scrolls through many years -- capacity 8417 (the whole 1583-9999 domain) +// would be a real, if small, leak risk over a long enough session; 64 keeps +// memory negligible (each entry is a handful of time.Time values) while +// still giving a warm cache for the realistic access pattern, a week/month +// view moving through nearby dates. +const efTransferPlanCacheCap = 64 + +// efTransferPlanCacheKey is everything efTransferPlan's result actually +// depends on, established by reading efTransferPlan, celebrationDate, +// temporalEF and resolveDate rather than assumed: +// +// - year: efTransferPlan's own first argument (Easter, the transfer +// window, every celebrationDate call). +// - sel: passed through to celebrationDate on every candidate, which +// currently ignores it (resolveDate never reads its sel parameter) -- +// so it is a dead dependency TODAY. It is still part of the key: the +// parameter exists on the signature for a reason, and keying on it now +// costs nothing (Selection is four small strings) while protecting +// against a future change that makes EF date resolution +// Selection-sensitive silently poisoning a cache that never accounted +// for it. +// - a content hash of merged: efTransferPlan's real data dependency. +// merged is a map -- not itself comparable, so cannot be a map key +// field -- and mergeLayers rebuilds a brand-new map with fresh Go map +// iteration order on every call even when the underlying layers are +// unchanged, so identity/pointer comparison cannot be used either only +// a hash of merged's actual content is both correct (invalidates +// whenever the content genuinely differs -- a user overlay edited, a +// different layer stack) and cache-friendly (hits whenever it does not, +// regardless of which Layer objects or map iteration produced it). See +// hashMergedForPlan for what is and is not hashed. +type efTransferPlanCacheKey struct { + year int + sel Selection + hash [32]byte +} + +// efTransferPlanCacheEntry is one memoised plan, doubling as the LRU list's +// payload so eviction and lookup share one map. +type efTransferPlanCacheEntry struct { + key efTransferPlanCacheKey + plan map[string]time.Time +} + +// efTransferPlanCache is a small, bounded, thread-safe LRU in front of +// efTransferPlan. gomobile may call Day/Days in from multiple goroutines, so +// the map and list are guarded by one mutex; efTransferPlan itself (the +// expensive part) runs OUTSIDE the lock so concurrent misses on different +// keys do not serialise behind each other -- see efTransferPlanCached. +var efTransferPlanCache = struct { + mu sync.Mutex + entries map[efTransferPlanCacheKey]*list.Element + order *list.List // front = most recently used +}{entries: map[efTransferPlanCacheKey]*list.Element{}, order: list.New()} + +// efTransferPlanCached is efTransferPlan, memoised by (year, sel, merged +// content). It is safe and correct to share across every caller in the +// process: two calls with the SAME key are, by construction of the key, +// calls efTransferPlan itself would resolve identically (same year, same +// Selection, same sanctoral content), so returning a cached result changes +// nothing about what is computed -- only when. The returned map is a fresh +// copy per call (clonePlan), never the cached instance itself, so no caller +// can mutate shared cache state even though computeEF's own use of the +// result is read-only today. +func efTransferPlanCached(year int, merged map[string]RawCelebration, sel Selection, + occupiedByClass1, occupiedByClass1Or2 func(time.Time, string) bool) map[string]time.Time { + + key := efTransferPlanCacheKey{year: year, sel: sel, hash: hashMergedForPlan(merged)} + + efTransferPlanCache.mu.Lock() + if el, ok := efTransferPlanCache.entries[key]; ok { + efTransferPlanCache.order.MoveToFront(el) + plan := el.Value.(*efTransferPlanCacheEntry).plan + efTransferPlanCache.mu.Unlock() + return clonePlan(plan) + } + efTransferPlanCache.mu.Unlock() + + // Compute outside the lock: efTransferPlan is the expensive call this + // cache exists to avoid repeating, and holding the mutex across it would + // serialise every concurrent miss on DIFFERENT keys, not just protect + // the shared map/list. + plan := efTransferPlan(year, merged, sel, occupiedByClass1, occupiedByClass1Or2) + + efTransferPlanCache.mu.Lock() + defer efTransferPlanCache.mu.Unlock() + if el, ok := efTransferPlanCache.entries[key]; ok { + // Lost a race: another goroutine populated this exact key while we + // were computing our own copy unlocked. Same key => same result by + // construction, so keep the existing entry and just bump recency. + efTransferPlanCache.order.MoveToFront(el) + return clonePlan(el.Value.(*efTransferPlanCacheEntry).plan) + } + el := efTransferPlanCache.order.PushFront(&efTransferPlanCacheEntry{key: key, plan: plan}) + efTransferPlanCache.entries[key] = el + if efTransferPlanCache.order.Len() > efTransferPlanCacheCap { + oldest := efTransferPlanCache.order.Back() + efTransferPlanCache.order.Remove(oldest) + delete(efTransferPlanCache.entries, oldest.Value.(*efTransferPlanCacheEntry).key) + } + return clonePlan(plan) +} + +func clonePlan(plan map[string]time.Time) map[string]time.Time { + out := make(map[string]time.Time, len(plan)) + for k, v := range plan { + out[k] = v + } + return out +} + +// hashMergedForPlan hashes merged's full content deterministically. Go's map +// iteration order is randomised per-process, so slugs (and, within each +// entry, its Fields and Variant keys) are sorted before hashing -- the same +// logical content always hashes identically, regardless of which Layer +// stack produced it or what order ranging over it visits entries. +// +// EVERY field of every entry is hashed, not just the ones efTransferPlan's +// own read path happens to touch today (Rank, Date via celebrationDate). +// occupiedByClass1/Or2 (closures over merged, passed in by computeEF) scan +// ALL of merged looking for competing entries of any rank, and which +// entries even qualify as class-1 in the first place is ITSELF computed +// from this data (buildCelebration's "rank" field) -- so hashing only a +// subset of fields would risk exactly the silent-stale-cache bug this +// function exists to prevent: a user overlay that changes, say, a +// "suppress" or an unrelated feast's rank would not change the hash, and a +// stale plan would keep being served. +func hashMergedForPlan(merged map[string]RawCelebration) [32]byte { + slugs := make([]string, 0, len(merged)) + for slug := range merged { + slugs = append(slugs, slug) + } + sort.Strings(slugs) + + h := sha256.New() + for _, slug := range slugs { + rc := merged[slug] + io.WriteString(h, "slug=") + io.WriteString(h, slug) + h.Write([]byte{0}) + writeSortedFields(h, rc.Fields) + variants := make([]string, 0, len(rc.Variants)) + for v := range rc.Variants { + variants = append(variants, v) + } + sort.Strings(variants) + for _, v := range variants { + io.WriteString(h, "variant=") + io.WriteString(h, v) + h.Write([]byte{0}) + writeSortedFields(h, rc.Variants[v]) + } + h.Write([]byte{1}) // record separator, so "ab"+"c" cannot collide with "a"+"bc" + } + var sum [32]byte + copy(sum[:], h.Sum(nil)) + return sum +} + +// writeSortedFields writes fields into h as sorted "key=value\x00" records +// (see hashMergedForPlan). +func writeSortedFields(h io.Writer, fields map[string]string) { + keys := make([]string, 0, len(fields)) + for k := range fields { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + io.WriteString(h, k) + h.Write([]byte{'='}) + io.WriteString(h, fields[k]) + h.Write([]byte{0}) + } +} diff --git a/internal/calendar/transfer_plan_cache_test.go b/internal/calendar/transfer_plan_cache_test.go new file mode 100644 index 0000000..6b3b48b --- /dev/null +++ b/internal/calendar/transfer_plan_cache_test.go @@ -0,0 +1,191 @@ +package calendar_test + +// Tests for the memoised EF transfer plan (transfer_plan_cache.go). These +// live in the external test package (calendar_test), exercising the real +// tridentine sanctoral data through the public Compute entry point, the same +// style precedence_ef_repro_test.go already uses -- the risk this cache +// introduces is entirely about whether the CACHE KEY captures every real +// input, which can only be demonstrated by varying an input through Compute +// and checking the observed office actually changes, not by testing the +// cache's internals in isolation. + +import ( + "sync" + "testing" + "time" + + "github.com/lukaszkasprzak/lectio/internal/caldata" + "github.com/lukaszkasprzak/lectio/internal/calendar" +) + +// TestEFTransferPlanCacheInvalidatesOnOverlay is THE bad-key test: it warms +// the memoised plan for (2008, EF, the shipped calendar) across a whole +// week -- mobile.Days's own access pattern -- then asks the SAME year +// through a layer stack with ONE user overlay added (suppressing the +// Annunciation) and asserts the observed office on 31 March and 1 April +// CHANGES accordingly. +// +// What is varied: the layer stack / merged sanctoral content (a user +// overlay), holding year and Selection fixed. This is deliberately not a +// synthetic scenario: with the Annunciation present, its RG 96(a) proper +// seat (the Monday after Low Sunday, RG 97/98) forces St Joseph -- also +// impeded that year -- to walk one day further, to 1 April +// (TestEFTwoTransfersDoNotCollide already pins this). Suppress the +// Annunciation and it no longer claims that Monday, so Joseph lands ON it, +// 31 March, and 1 April reverts to an ordinary feria. Verified empirically +// against the pre-cache code before this test was written (both outcomes +// reproduced exactly as asserted below). +// +// A cache keyed on year alone -- or on year+sel without the merged content +// -- would serve 2008's BASE-CALENDAR plan back for the overlaid query too, +// since both share every other key component; this test fails loudly if +// that happens (it would see 1 April still reporting Joseph, and 31 March +// still reporting the Annunciation, from the first, unrelated warm-up). +func TestEFTransferPlanCacheInvalidatesOnOverlay(t *testing.T) { + sel := calendar.DefaultSelection() + sel.Form = "old" + base := caldata.Tridentine() + baseLayers := []calendar.Layer{base} + + compute := func(layers []calendar.Layer, date string) string { + d, err := time.Parse("2006-01-02", date) + if err != nil { + t.Fatalf("bad test date %q: %v", date, err) + } + return calendar.Compute(d.UTC(), sel, layers).Observed.Slug + } + + // Warm the cache for (2008, sel, base-only-hash) across a whole week, + // exactly like mobile.Days resolving seven consecutive dates against the + // same Prepared layer stack. + for _, date := range []string{ + "2008-03-26", "2008-03-27", "2008-03-28", "2008-03-29", + "2008-03-30", "2008-03-31", "2008-04-01", "2008-04-02", + } { + compute(baseLayers, date) + } + if got := compute(baseLayers, "2008-03-31"); got != "annunciation-of-the-blessed-virgin-mary" { + t.Fatalf("baseline 2008-03-31 = %q, want annunciation-of-the-blessed-virgin-mary", got) + } + if got := compute(baseLayers, "2008-04-01"); got != "joseph-spouse-of-the-bl-virgin-mary" { + t.Fatalf("baseline 2008-04-01 = %q, want joseph-spouse-of-the-bl-virgin-mary", got) + } + + // Same year, same Selection, ONE overlay layer added: suppress the + // Annunciation. If the cache key omitted the sanctoral content, these + // two calls would silently return the baseline plan warmed above. + overlay := calendar.Layer{ID: "user", Cels: map[string]calendar.RawCelebration{ + "annunciation-of-the-blessed-virgin-mary": { + Fields: map[string]string{"suppress": "true"}, + Variants: map[string]map[string]string{}, + }, + }} + overlaid := []calendar.Layer{base, overlay} + + if got := compute(overlaid, "2008-03-31"); got != "joseph-spouse-of-the-bl-virgin-mary" { + t.Errorf("overlaid 2008-03-31 = %q, want joseph-spouse-of-the-bl-virgin-mary (Joseph now lands here, the Annunciation no longer claims it)", got) + } + if got := compute(overlaid, "2008-04-01"); got == "joseph-spouse-of-the-bl-virgin-mary" { + t.Errorf("overlaid 2008-04-01 = %q, want NOT joseph (a stale cache hit from the base-calendar warm-up)", got) + } + + // And the base calendar's own answer must be unaffected by having since + // computed the overlaid one -- the two keys must not collide either way. + if got := compute(baseLayers, "2008-03-31"); got != "annunciation-of-the-blessed-virgin-mary" { + t.Errorf("base calendar 2008-03-31 after overlaid query = %q, want annunciation-of-the-blessed-virgin-mary (unaffected)", got) + } + if got := compute(baseLayers, "2008-04-01"); got != "joseph-spouse-of-the-bl-virgin-mary" { + t.Errorf("base calendar 2008-04-01 after overlaid query = %q, want joseph-spouse-of-the-bl-virgin-mary (unaffected)", got) + } +} + +// TestEFTransferPlanCacheInvalidatesOnYear is a lighter companion: the same +// week-then-query pattern, varying the YEAR instead of the overlay (2008 vs +// 2035, both real Joseph/Annunciation collision years -- see +// TestEFTwoTransfersDoNotCollide -- but with different transfer targets). +// year is an explicit field of the cache key already, so this mainly guards +// against a key struct refactor accidentally dropping it; the overlay test +// above is the one guarding the field that is easy to omit by mistake. +func TestEFTransferPlanCacheInvalidatesOnYear(t *testing.T) { + sel := calendar.DefaultSelection() + sel.Form = "old" + layers := []calendar.Layer{caldata.Tridentine()} + + compute := func(date string) string { + d, err := time.Parse("2006-01-02", date) + if err != nil { + t.Fatalf("bad test date %q: %v", date, err) + } + return calendar.Compute(d.UTC(), sel, layers).Observed.Slug + } + + if got := compute("2008-04-01"); got != "joseph-spouse-of-the-bl-virgin-mary" { + t.Fatalf("2008-04-01 = %q, want joseph-spouse-of-the-bl-virgin-mary", got) + } + // 2035's Joseph lands on 2035-04-03, not 04-01 (a later Easter shifts the + // whole window). If the cache ignored the year, this would wrongly + // return 2008's plan. + if got := compute("2035-04-01"); got == "joseph-spouse-of-the-bl-virgin-mary" { + t.Errorf("2035-04-01 = %q, want NOT joseph (that is 2008's landing day, not 2035's)", got) + } + if got := compute("2035-04-03"); got != "joseph-spouse-of-the-bl-virgin-mary" { + t.Errorf("2035-04-03 = %q, want joseph-spouse-of-the-bl-virgin-mary", got) + } +} + +// TestEFTransferPlanCacheConcurrentUse exercises the memoised plan from many +// goroutines at once -- gomobile may call Day/Days in from multiple threads, +// and none of the tests above (all sequential) can catch a data race on the +// shared cache map/list. Run with -race; it is the actual proof of the +// "thread-safe" claim, not merely built with a mutex and assumed correct. +func TestEFTransferPlanCacheConcurrentUse(t *testing.T) { + sel := calendar.DefaultSelection() + sel.Form = "old" + base := caldata.Tridentine() + overlay := calendar.Layer{ID: "user", Cels: map[string]calendar.RawCelebration{ + "annunciation-of-the-blessed-virgin-mary": { + Fields: map[string]string{"suppress": "true"}, + Variants: map[string]map[string]string{}, + }, + }} + baseLayers := []calendar.Layer{base} + overlaidLayers := []calendar.Layer{base, overlay} + + years := []int{2008, 2011, 2035, 2046} + + var wg sync.WaitGroup + for g := 0; g < 12; g++ { + g := g + wg.Add(1) + go func() { + defer wg.Done() + layers := baseLayers + if g%2 == 0 { + layers = overlaidLayers // different goroutines hammer different cache keys + } + for i := 0; i < 6; i++ { + y := years[(g+i)%len(years)] + for _, md := range []string{"03-19", "03-31", "04-01"} { + d, err := time.Parse("2006-01-02", time.Date(y, 1, 1, 0, 0, 0, 0, time.UTC).Format("2006")+"-"+md) + if err != nil { + t.Errorf("bad date: %v", err) + return + } + _ = calendar.Compute(d.UTC(), sel, layers).Observed.Slug + } + } + }() + } + wg.Wait() + + // After the concurrent hammering, correctness must still hold for both + // keys -- the concurrency test is not a substitute for the correctness + // test above, so re-assert both outcomes here too. + d, _ := time.Parse("2006-01-02", "2008-04-01") + if got := calendar.Compute(d.UTC(), sel, baseLayers).Observed.Slug; got != "joseph-spouse-of-the-bl-virgin-mary" { + t.Errorf("after concurrent use, base 2008-04-01 = %q, want joseph-spouse-of-the-bl-virgin-mary", got) + } + if got := calendar.Compute(d.UTC(), sel, overlaidLayers).Observed.Slug; got == "joseph-spouse-of-the-bl-virgin-mary" { + t.Errorf("after concurrent use, overlaid 2008-04-01 = %q, want NOT joseph", got) + } +} -- cgit v1.3 From b24702cc9b8d7f2e408bb92032e8f7fa2aaa836b Mon Sep 17 00:00:00 2001 From: Lukasz Kasprzak Date: Mon, 24 Aug 2026 22:02:39 +0200 Subject: perf(calendar): precompute the EF occupancy index alongside the transfer plan 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. --- internal/calendar/calendar.go | 79 +++++++------ internal/calendar/transfer_plan_cache.go | 154 ++++++++++++++++++++------ internal/calendar/transfer_plan_cache_test.go | 154 +++++++++++++++++++++++--- 3 files changed, 294 insertions(+), 93 deletions(-) (limited to 'internal') diff --git a/internal/calendar/calendar.go b/internal/calendar/calendar.go index 00048b7..a033c81 100644 --- a/internal/calendar/calendar.go +++ b/internal/calendar/calendar.go @@ -36,45 +36,22 @@ func computeEF(date time.Time, sel Selection, layers []Layer) LiturgicalDay { td := temporalEF(date) merged := mergeLayers(layers) year := date.Year() - // occupiedByRank reports whether some OTHER fixed-date sanctoral - // celebration whose rank passes `allowed` resolves onto d this year. - // transferIfImpededEF uses this at two different thresholds: class 1 - // only, to decide whether a candidate is impeded in the first place (a - // class-2 occupant never impedes a class-1 feast -- class 1 always beats - // class 2 outright, no tie exists); and class 1 OR 2, for RG 96's "next - // day that is not I or II class" once a transfer is already under way - // (e.g. the Visitation, 2 July, blocking the Precious Blood's transfer - // off 1 July in 2011). - occupiedByRank := func(d time.Time, exceptSlug string, allowed func(Rank) bool) bool { - for slug2, rc2 := range merged { - if slug2 == exceptSlug { - continue - } - cel2 := buildCelebration(slug2, rc2) - if !allowed(cel2.Rank) { - continue - } - if when2, ok := celebrationDate(cel2, year, sel); ok && sameDay(when2, d) { - return true - } - } - return false - } - isClass1 := func(r Rank) bool { return r == RankClass1 } - isClass1Or2 := func(r Rank) bool { return r == RankClass1 || r == RankClass2 } - occ1 := func(d time.Time, except string) bool { return occupiedByRank(d, except, isClass1) } - occ1Or2 := func(d time.Time, except string) bool { return occupiedByRank(d, except, isClass1Or2) } // RG 97/98: the year's impeded I-class transfers are resolved as a set, // not one at a time, so two feasts impeded by the same early Easter cannot // both claim the same free day and lose one of themselves. // - // efTransferPlan is pure in (year, merged content, sel) and identical for - // every day of the year it is asked about -- computeEF runs once PER DAY, - // so a multi-day view (mobile.Days's week, a month view) was rebuilding - // it from scratch on every single one. efTransferPlanCached memoises it; - // see transfer_plan_cache.go for the cache key and why each of its three - // parts is load-bearing. - plan := efTransferPlanCached(year, merged, sel, occ1, occ1Or2) + // Both the plan and the occupancy index it is built from are pure in + // (year, merged content, sel) and identical for every day of the year + // they are asked about -- computeEF runs once PER DAY, so a multi-day + // view (mobile.Days's week, a month view) was rebuilding both from + // scratch on every single one. efTransferPlanCached memoises them + // together; see transfer_plan_cache.go for the cache key, why each of + // its three parts is load-bearing, and what the occupancy index is an + // index OF (every merged entry's own ORIGINAL, untransferred date -- + // never a transfer TARGET, which is decided during planning and tracked + // separately, only within one planning pass, by efTransferPlan's own + // `claimed`). + plan, occ := efTransferPlanCached(year, merged, sel) cands := []candidate{{Cel: td.Cel, Temporal: true, Season: td.Season, Sunday: td.Sunday}} for slug, rc := range merged { cel := buildCelebration(slug, rc) @@ -90,9 +67,22 @@ func computeEF(date time.Time, sel Selection, layers []Layer) LiturgicalDay { } effective, planned := plan[cel.Slug] if !planned { + // occ.occupied answers exactly what computeEF's own former + // occupiedByRank closure did -- "does some OTHER fixed-date + // sanctoral celebration whose rank passes `allowed` resolve onto + // d this year" -- at the same two thresholds transferIfImpededEF + // has always used: class 1 only, to decide whether a candidate is + // impeded in the first place (a class-2 occupant never impedes a + // class-1 feast -- class 1 always beats class 2 outright, no + // tie-break is even reached); and class 1 OR 2, for RG 96's "next + // day that is not I or II class" once a transfer is already under + // way (e.g. the Visitation, 2 July, blocking the Precious + // Blood's transfer off 1 July in 2011). It now reads a + // precomputed index instead of scanning merged afresh -- see + // transfer_plan_cache.go. effective = transferIfImpededEF(cel, when, - func(d time.Time) bool { return occ1(d, cel.Slug) }, - func(d time.Time) bool { return occ1Or2(d, cel.Slug) }) + func(d time.Time) bool { return occ.occupied(d, cel.Slug, isClass1Rank) }, + func(d time.Time) bool { return occ.occupied(d, cel.Slug, isClass1Or2Rank) }) } if sameDay(effective, date) { cands = append(cands, candidate{Cel: cel, Temporal: false, Season: td.Season}) @@ -248,6 +238,14 @@ func transferIfImpeded(cel Celebration, when time.Time, sel Selection) time.Time return day } +// isClass1Rank and isClass1Or2Rank are the two occupancy thresholds +// efTransferPlan and transferIfImpededEF (via computeEF's call site) test +// against efOccupancyIndex -- see transfer_plan_cache.go's doc comment on +// efOccupancyIndex for what "occupied" means and why it is safe to +// precompute once per (year, merged content, sel). +func isClass1Rank(r Rank) bool { return r == RankClass1 } +func isClass1Or2Rank(r Rank) bool { return r == RankClass1 || r == RankClass2 } + // efTransferPlan resolves ALL of a year's impeded I-class transfers together, // which RG 97/98 require and which resolving them one at a time cannot do. // @@ -268,8 +266,7 @@ func transferIfImpeded(cel Celebration, when time.Time, sel Selection) time.Time // takes its proper seat on 2 April; St Joseph, impeded on the 19th, walks past // Holy Week, the Easter octave and that claimed Monday to 3 April. Before this, // St Joseph was observed on no day of 2008, 2035 or 2046 at all. -func efTransferPlan(year int, merged map[string]RawCelebration, sel Selection, - occupiedByClass1, occupiedByClass1Or2 func(time.Time, string) bool) map[string]time.Time { +func efTransferPlan(year int, merged map[string]RawCelebration, sel Selection, occ efOccupancyIndex) map[string]time.Time { type pending struct { slug string @@ -297,7 +294,7 @@ func efTransferPlan(year int, merged map[string]RawCelebration, sel Selection, st := temporalEF(when) tCand := candidate{Cel: st.Cel, Temporal: true, Season: st.Season, Sunday: st.Sunday} sCand := candidate{Cel: cel, Temporal: false} - if precedenceEF(tCand) >= precedenceEF(sCand) && !occupiedByClass1(when, cel.Slug) { + if precedenceEF(tCand) >= precedenceEF(sCand) && !occ.occupied(when, cel.Slug, isClass1Rank) { continue // not impeded; stays put } // RG 96(a): a proper seat, claimed before anything queues. Guarded to @@ -328,7 +325,7 @@ func efTransferPlan(year int, merged map[string]RawCelebration, sel Selection, for i := 0; i < 60; i++ { b := temporalEF(day) isHighClass := b.Cel.Rank == RankClass1 || b.Cel.Rank == RankClass2 - if isHighClass || occupiedByClass1Or2(day, p.cel.Slug) || claimed[day.Format("2006-01-02")] { + if isHighClass || occ.occupied(day, p.cel.Slug, isClass1Or2Rank) || claimed[day.Format("2006-01-02")] { day = day.AddDate(0, 0, 1) continue } diff --git a/internal/calendar/transfer_plan_cache.go b/internal/calendar/transfer_plan_cache.go index fdf62f7..f03e17b 100644 --- a/internal/calendar/transfer_plan_cache.go +++ b/internal/calendar/transfer_plan_cache.go @@ -18,9 +18,12 @@ import ( // view moving through nearby dates. const efTransferPlanCacheCap = 64 -// efTransferPlanCacheKey is everything efTransferPlan's result actually -// depends on, established by reading efTransferPlan, celebrationDate, -// temporalEF and resolveDate rather than assumed: +// efTransferPlanCacheKey is everything efTransferPlan's result -- and, since +// ef_transfer_plan_cache.go v2, efOccupancyIndex's result -- actually depend +// on, established by reading efTransferPlan, celebrationDate, temporalEF and +// resolveDate rather than assumed. Both the plan and the occupancy index are +// pure functions of exactly these three inputs (see efOccupancyIndex's own +// doc comment for occupancy specifically), so one key correctly covers both: // // - year: efTransferPlan's own first argument (Easter, the transfer // window, every celebrationDate call). @@ -48,11 +51,14 @@ type efTransferPlanCacheKey struct { hash [32]byte } -// efTransferPlanCacheEntry is one memoised plan, doubling as the LRU list's -// payload so eviction and lookup share one map. +// efTransferPlanCacheEntry is one memoised (plan, occupancy index) pair, +// doubling as the LRU list's payload so eviction and lookup share one map. +// The two are cached together because they are built from the same data in +// one pass and share the same cache key -- see efTransferPlanCacheKey. type efTransferPlanCacheEntry struct { - key efTransferPlanCacheKey - plan map[string]time.Time + key efTransferPlanCacheKey + plan map[string]time.Time + occupancy efOccupancyIndex } // efTransferPlanCache is a small, bounded, thread-safe LRU in front of @@ -66,34 +72,109 @@ var efTransferPlanCache = struct { order *list.List // front = most recently used }{entries: map[efTransferPlanCacheKey]*list.Element{}, order: list.New()} -// efTransferPlanCached is efTransferPlan, memoised by (year, sel, merged -// content). It is safe and correct to share across every caller in the -// process: two calls with the SAME key are, by construction of the key, -// calls efTransferPlan itself would resolve identically (same year, same -// Selection, same sanctoral content), so returning a cached result changes -// nothing about what is computed -- only when. The returned map is a fresh -// copy per call (clonePlan), never the cached instance itself, so no caller -// can mutate shared cache state even though computeEF's own use of the -// result is read-only today. -func efTransferPlanCached(year int, merged map[string]RawCelebration, sel Selection, - occupiedByClass1, occupiedByClass1Or2 func(time.Time, string) bool) map[string]time.Time { +// efOccupancyEntry is one merged entry's slug and rank, indexed under its own +// ORIGINAL (untransferred) date -- see efOccupancyIndex. +type efOccupancyEntry struct { + slug string + rank Rank +} + +// efOccupancyIndex answers exactly the question computeEF's old +// occupiedByRank closure scanned all of merged for on every single call: +// "does some entry other than exceptSlug, with a rank `allowed` accepts, +// resolve to date d this year?" -- but as a precomputed lookup instead of a +// fresh O(len(merged)) scan. +// +// BE PRECISE ABOUT WHAT "OCCUPIED" DEPENDS ON, because this is the question +// that decides whether precomputing it is safe: an entry's ORIGINAL date +// (buildCelebration + celebrationDate, exactly as this index computes it) is +// a pure function of (merged content, year, sel) alone -- it never considers +// whether that entry has since been TRANSFERRED (celebrationDate calls +// resolveDate directly; nothing about transfer resolution feeds into it) and +// never depends on `date`, the day computeEF happens to be resolving. That +// is what makes one index, built once per (year, merged, sel) and reused for +// every day and every candidate resolved against it, correct. +// +// This is a DIFFERENT question from efTransferPlan's own `claimed` map, +// which tracks which TARGET days a transfer walk has already assigned +// DURING one planning pass -- claimed genuinely mutates as planning +// proceeds and has no meaning outside that single call, so it is (correctly) +// still computed fresh inside efTransferPlan every time, never cached here. +// Conflating the two -- treating "is some entry's ORIGINAL date d" as if it +// captured "has some transfer already CLAIMED d" -- would be the exact +// silent-wrong-answer failure mode this comment exists to rule out; they are +// checked as three independent conditions everywhere they are used +// together (see efTransferPlan's own forward-walk loop). +type efOccupancyIndex map[string][]efOccupancyEntry // "2006-01-02" -> entries + +// buildEFOccupancyIndex computes the index in one O(len(merged)) pass. +func buildEFOccupancyIndex(merged map[string]RawCelebration, year int, sel Selection) efOccupancyIndex { + idx := efOccupancyIndex{} + for slug, rc := range merged { + cel := buildCelebration(slug, rc) + when, ok := celebrationDate(cel, year, sel) + if !ok { + continue + } + key := when.Format("2006-01-02") + idx[key] = append(idx[key], efOccupancyEntry{slug: slug, rank: cel.Rank}) + } + return idx +} + +// occupied reports whether some entry other than exceptSlug at date d has a +// rank allowed accepts -- computeEF's former occupiedByRank closure's exact +// semantics, read from the precomputed index instead of a fresh scan. Safe +// to call concurrently: idx is never mutated after buildEFOccupancyIndex +// returns it (Go's map type permits unlimited concurrent reads with no +// concurrent write). +func (idx efOccupancyIndex) occupied(d time.Time, exceptSlug string, allowed func(Rank) bool) bool { + for _, e := range idx[d.Format("2006-01-02")] { + if e.slug == exceptSlug { + continue + } + if allowed(e.rank) { + return true + } + } + return false +} + +// efTransferPlanCached is (efTransferPlan, buildEFOccupancyIndex), memoised +// together by (year, sel, merged content). It is safe and correct to share +// across every caller in the process: two calls with the SAME key are, by +// construction of the key, calls efTransferPlan/buildEFOccupancyIndex +// themselves would resolve identically (same year, same Selection, same +// sanctoral content), so returning a cached result changes nothing about +// what is computed -- only when. The returned plan is a fresh copy per call +// (clonePlan), never the cached instance, so no caller can mutate shared +// cache state even though computeEF's own use of it is read-only today. The +// occupancy index is returned WITHOUT copying (unlike plan, it can hold one +// entry per merged slug, up to ~330 on shipped data, so copying it on every +// call -- most of them hits -- would give back a real slice of the win this +// exists to capture); this is safe only because it is genuinely immutable +// after construction (see efOccupancyIndex's own doc comment) and no code +// anywhere writes to a returned index. +func efTransferPlanCached(year int, merged map[string]RawCelebration, sel Selection) (map[string]time.Time, efOccupancyIndex) { key := efTransferPlanCacheKey{year: year, sel: sel, hash: hashMergedForPlan(merged)} efTransferPlanCache.mu.Lock() if el, ok := efTransferPlanCache.entries[key]; ok { efTransferPlanCache.order.MoveToFront(el) - plan := el.Value.(*efTransferPlanCacheEntry).plan + entry := el.Value.(*efTransferPlanCacheEntry) + plan, occ := entry.plan, entry.occupancy efTransferPlanCache.mu.Unlock() - return clonePlan(plan) + return clonePlan(plan), occ } efTransferPlanCache.mu.Unlock() - // Compute outside the lock: efTransferPlan is the expensive call this - // cache exists to avoid repeating, and holding the mutex across it would - // serialise every concurrent miss on DIFFERENT keys, not just protect - // the shared map/list. - plan := efTransferPlan(year, merged, sel, occupiedByClass1, occupiedByClass1Or2) + // Compute outside the lock: building the index and the plan is the + // expensive work this cache exists to avoid repeating, and holding the + // mutex across it would serialise every concurrent miss on DIFFERENT + // keys, not just protect the shared map/list. + occ := buildEFOccupancyIndex(merged, year, sel) + plan := efTransferPlan(year, merged, sel, occ) efTransferPlanCache.mu.Lock() defer efTransferPlanCache.mu.Unlock() @@ -102,16 +183,17 @@ func efTransferPlanCached(year int, merged map[string]RawCelebration, sel Select // were computing our own copy unlocked. Same key => same result by // construction, so keep the existing entry and just bump recency. efTransferPlanCache.order.MoveToFront(el) - return clonePlan(el.Value.(*efTransferPlanCacheEntry).plan) + entry := el.Value.(*efTransferPlanCacheEntry) + return clonePlan(entry.plan), entry.occupancy } - el := efTransferPlanCache.order.PushFront(&efTransferPlanCacheEntry{key: key, plan: plan}) + el := efTransferPlanCache.order.PushFront(&efTransferPlanCacheEntry{key: key, plan: plan, occupancy: occ}) efTransferPlanCache.entries[key] = el if efTransferPlanCache.order.Len() > efTransferPlanCacheCap { oldest := efTransferPlanCache.order.Back() efTransferPlanCache.order.Remove(oldest) delete(efTransferPlanCache.entries, oldest.Value.(*efTransferPlanCacheEntry).key) } - return clonePlan(plan) + return clonePlan(plan), occ } func clonePlan(plan map[string]time.Time) map[string]time.Time { @@ -128,16 +210,16 @@ func clonePlan(plan map[string]time.Time) map[string]time.Time { // logical content always hashes identically, regardless of which Layer // stack produced it or what order ranging over it visits entries. // -// EVERY field of every entry is hashed, not just the ones efTransferPlan's -// own read path happens to touch today (Rank, Date via celebrationDate). -// occupiedByClass1/Or2 (closures over merged, passed in by computeEF) scan -// ALL of merged looking for competing entries of any rank, and which -// entries even qualify as class-1 in the first place is ITSELF computed -// from this data (buildCelebration's "rank" field) -- so hashing only a -// subset of fields would risk exactly the silent-stale-cache bug this +// EVERY field of every entry is hashed, not just the ones the plan's or the +// occupancy index's own read paths happen to touch today (Rank, Date via +// celebrationDate). efOccupancyIndex indexes ALL of merged at every rank +// (not just class-1/2 -- occ.occupied's `allowed` parameter is arbitrary), +// and which entries even qualify as class-1 in the first place is ITSELF +// computed from this data (buildCelebration's "rank" field) -- so hashing +// only a subset of fields would risk exactly the silent-stale-cache bug this // function exists to prevent: a user overlay that changes, say, a // "suppress" or an unrelated feast's rank would not change the hash, and a -// stale plan would keep being served. +// stale plan or index would keep being served. func hashMergedForPlan(merged map[string]RawCelebration) [32]byte { slugs := make([]string, 0, len(merged)) for slug := range merged { diff --git a/internal/calendar/transfer_plan_cache_test.go b/internal/calendar/transfer_plan_cache_test.go index 6b3b48b..1592644 100644 --- a/internal/calendar/transfer_plan_cache_test.go +++ b/internal/calendar/transfer_plan_cache_test.go @@ -150,37 +150,69 @@ func TestEFTransferPlanCacheConcurrentUse(t *testing.T) { }} baseLayers := []calendar.Layer{base} overlaidLayers := []calendar.Layer{base, overlay} + // A third, disjoint key exercising the occupancy-index collision path + // (TestEFOccupancyIndexDetectsSameDateClass1Collision) concurrently too + // -- the plan alone does not touch efOccupancyIndex.occupied unless a + // candidate's own precedence doesn't already decide it, which the + // Joseph/Annunciation scenario above never triggers (see that test's own + // doc comment). + collideLayers := []calendar.Layer{base, {ID: "user", Cels: map[string]calendar.RawCelebration{ + "zz-test-alpha": {Fields: map[string]string{"rank": "class-1", "date": "07-06", "name.en": "ZZ Test Alpha"}, Variants: map[string]map[string]string{}}, + "zz-test-beta": {Fields: map[string]string{"rank": "class-1", "date": "07-06", "name.en": "ZZ Test Beta"}, Variants: map[string]map[string]string{}}, + }}} years := []int{2008, 2011, 2035, 2046} var wg sync.WaitGroup - for g := 0; g < 12; g++ { + for g := 0; g < 15; g++ { g := g wg.Add(1) go func() { defer wg.Done() - layers := baseLayers - if g%2 == 0 { - layers = overlaidLayers // different goroutines hammer different cache keys - } - for i := 0; i < 6; i++ { - y := years[(g+i)%len(years)] - for _, md := range []string{"03-19", "03-31", "04-01"} { - d, err := time.Parse("2006-01-02", time.Date(y, 1, 1, 0, 0, 0, 0, time.UTC).Format("2006")+"-"+md) - if err != nil { - t.Errorf("bad date: %v", err) - return + switch g % 3 { + case 0: + for i := 0; i < 6; i++ { + y := years[(g+i)%len(years)] + for _, md := range []string{"03-19", "03-31", "04-01"} { + d, err := time.Parse("2006-01-02", time.Date(y, 1, 1, 0, 0, 0, 0, time.UTC).Format("2006")+"-"+md) + if err != nil { + t.Errorf("bad date: %v", err) + return + } + _ = calendar.Compute(d.UTC(), sel, baseLayers).Observed.Slug + } + } + case 1: + for i := 0; i < 6; i++ { + y := years[(g+i)%len(years)] + for _, md := range []string{"03-19", "03-31", "04-01"} { + d, err := time.Parse("2006-01-02", time.Date(y, 1, 1, 0, 0, 0, 0, time.UTC).Format("2006")+"-"+md) + if err != nil { + t.Errorf("bad date: %v", err) + return + } + _ = calendar.Compute(d.UTC(), sel, overlaidLayers).Observed.Slug + } + } + default: + for i := 0; i < 6; i++ { + for _, ymd := range []string{"2026-07-06", "2026-07-07", "2026-07-08"} { + d, err := time.Parse("2006-01-02", ymd) + if err != nil { + t.Errorf("bad date: %v", err) + return + } + _ = calendar.Compute(d.UTC(), sel, collideLayers).Observed.Slug } - _ = calendar.Compute(d.UTC(), sel, layers).Observed.Slug } } }() } wg.Wait() - // After the concurrent hammering, correctness must still hold for both - // keys -- the concurrency test is not a substitute for the correctness - // test above, so re-assert both outcomes here too. + // After the concurrent hammering, correctness must still hold for all + // three keys -- the concurrency test is not a substitute for the + // correctness tests above, so re-assert their outcomes here too. d, _ := time.Parse("2006-01-02", "2008-04-01") if got := calendar.Compute(d.UTC(), sel, baseLayers).Observed.Slug; got != "joseph-spouse-of-the-bl-virgin-mary" { t.Errorf("after concurrent use, base 2008-04-01 = %q, want joseph-spouse-of-the-bl-virgin-mary", got) @@ -188,4 +220,94 @@ func TestEFTransferPlanCacheConcurrentUse(t *testing.T) { if got := calendar.Compute(d.UTC(), sel, overlaidLayers).Observed.Slug; got == "joseph-spouse-of-the-bl-virgin-mary" { t.Errorf("after concurrent use, overlaid 2008-04-01 = %q, want NOT joseph", got) } + d2, _ := time.Parse("2006-01-02", "2026-07-06") + if got := calendar.Compute(d2.UTC(), sel, collideLayers).Observed.Slug; got != "ef-time-after-pentecost-6-monday" { + t.Errorf("after concurrent use, colliding 2026-07-06 = %q, want ef-time-after-pentecost-6-monday", got) + } +} + +// TestEFOccupancyIndexDetectsSameDateClass1Collision covers a path the tests +// above do not: efOccupancyIndex.occupied's use from computeEF's +// transferIfImpededEF call site (the per-candidate fallback for a class-1 +// entry efTransferPlan judged NOT impeded by temporal precedence alone), and +// efOccupancyIndex's own use inside efTransferPlan's initial impeded check +// (occupiedByClass1(when, cel.Slug) -- "does some OTHER fixed-date class-1 +// SANCTORAL feast already sit on `when`", the doc comment's own example, RG +// 97/98). Neither TestEFTwoTransfersDoNotCollide nor the overlay/year tests +// above exercise it: St Joseph and the Annunciation are impeded by HOLY +// WEEK'S OWN temporal precedence, on DIFFERENT original dates -- never by +// colliding with each other's original date -- so no existing test had ever +// driven two class-1 SANCTORAL entries onto the exact same calendar date. +// +// A synthetic overlay is used because no two class-1 feasts share a fixed +// date in the shipped 1962 calendar (an ordinary Time-after-Pentecost +// Monday, 6 July 2026, was checked empirically before writing this test: +// alone, a single synthetic class-1 entry there is simply observed, since +// nothing outranks or occupies it; the real collision case exists only by +// construction). +// +// What is varied: whether a SECOND class-1 entry shares the first one's +// date -- both entries otherwise identical (rank class-1, real content +// unrelated to any liturgical rule under test). If efOccupancyIndex failed +// to detect the collision (or wrongly matched exceptSlug against ITSELF, +// the most dangerous failure shape -- see the mutation proof below), 6 July +// would keep reporting the lone entry regardless. +func TestEFOccupancyIndexDetectsSameDateClass1Collision(t *testing.T) { + sel := calendar.DefaultSelection() + sel.Form = "old" + base := caldata.Tridentine() + + compute := func(layers []calendar.Layer, date string) string { + d, err := time.Parse("2006-01-02", date) + if err != nil { + t.Fatalf("bad test date %q: %v", date, err) + } + return calendar.Compute(d.UTC(), sel, layers).Observed.Slug + } + + alpha := calendar.RawCelebration{ + Fields: map[string]string{"rank": "class-1", "date": "07-06", "name.en": "ZZ Test Alpha"}, + Variants: map[string]map[string]string{}, + } + beta := calendar.RawCelebration{ + Fields: map[string]string{"rank": "class-1", "date": "07-06", "name.en": "ZZ Test Beta"}, + Variants: map[string]map[string]string{}, + } + + soloLayers := []calendar.Layer{base, {ID: "user", Cels: map[string]calendar.RawCelebration{ + "zz-test-alpha": alpha, + }}} + collideLayers := []calendar.Layer{base, {ID: "user", Cels: map[string]calendar.RawCelebration{ + "zz-test-alpha": alpha, + "zz-test-beta": beta, + }}} + + // Alone, alpha is simply observed on its own date: nothing occupies 6 + // July, so efTransferPlan judges it unimpeded and computeEF's fallback + // (occ.occupied via transferIfImpededEF) must agree and leave it there. + if got := compute(soloLayers, "2026-07-06"); got != "zz-test-alpha" { + t.Fatalf("alpha alone, 2026-07-06 = %q, want zz-test-alpha", got) + } + + // Add beta on the SAME date. Both now occupy each other's date, so BOTH + // are impeded (RG 97/98) and transfer forward in table/impeded-first + // order (alpha to 7 July, beta to 8 -- empirically confirmed before + // writing this test); 6 July itself reverts to the ordinary temporal + // office, since NEITHER candidate keeps its place. + if got := compute(collideLayers, "2026-07-06"); got != "ef-time-after-pentecost-6-monday" { + t.Errorf("alpha+beta colliding, 2026-07-06 = %q, want ef-time-after-pentecost-6-monday (a stale/broken occupancy index would still show zz-test-alpha)", got) + } + if got := compute(collideLayers, "2026-07-07"); got != "zz-test-alpha" { + t.Errorf("alpha+beta colliding, 2026-07-07 = %q, want zz-test-alpha (transferred here)", got) + } + if got := compute(collideLayers, "2026-07-08"); got != "zz-test-beta" { + t.Errorf("alpha+beta colliding, 2026-07-08 = %q, want zz-test-beta (transferred here)", got) + } + + // And alpha alone (no beta) must be unaffected by having since computed + // the colliding scenario -- the two overlays must not cross-contaminate + // the shared cache. + if got := compute(soloLayers, "2026-07-06"); got != "zz-test-alpha" { + t.Errorf("alpha alone after collision query, 2026-07-06 = %q, want zz-test-alpha (unaffected)", got) + } } -- cgit v1.3