From 17441c4cdbbacd67378cec31734e4cd255d96ae1 Mon Sep 17 00:00:00 2001 From: Dan Crosby Date: Wed, 28 Jan 2026 12:20:47 -0800 Subject: [PATCH] Add design doc for notification diff batching optimization (#5674) Documents analysis of why client notification generators currently require per-action-result game state diffs, and proposes a solution to enable diff batching by moving display data to server-side. Deferred for now as current performance is acceptable. Co-authored-by: Claude Opus 4.5 --- docs/NOTIFICATION_DIFF_BATCHING.md | 150 +++++++++++++++++++++++++++++ 1 file changed, 150 insertions(+) create mode 100644 docs/NOTIFICATION_DIFF_BATCHING.md diff --git a/docs/NOTIFICATION_DIFF_BATCHING.md b/docs/NOTIFICATION_DIFF_BATCHING.md new file mode 100644 index 0000000000..860b19217a --- /dev/null +++ b/docs/NOTIFICATION_DIFF_BATCHING.md @@ -0,0 +1,150 @@ +# Notification Diff Batching Optimization + +## Problem + +`ActionResultFilter.filterForPlayer` generates per-action-result game state diffs, which is expensive. Profiling shows significant time spent in `filteredGameStateDiff` → `GameStateViewFilter.filteredGameState` (called twice per result) → `GameStateViewDiffer.diff`. + +A potential optimization is to batch diffs: instead of computing N diffs for N action results, compute one combined diff representing the final state change. However, this is blocked by how client notification generators work. + +## Current Architecture + +### Server Side +1. `ActionResultFilter.filterForPlayer` processes each action result +2. For each result, computes `filteredGameStateDiff(before, after, factionId)` +3. Returns `Vector[ActionResultView]` where each view has its own `gameStateDiff` + +### Client Side +1. `EagleGameModel.HandleNewHistoryEntry` processes each `ActionResultView`: + ```csharp + private void HandleNewHistoryEntry(ActionResultView arv) { + _currentModel.HistoryCount++; + MaybeSendNotification(arv); // Uses currentModel state + ApplyGameStateViewDiff(arv.GameStateDiff); // Updates currentModel + } + ``` + +2. Notification generators receive `(ActionResultView, IGameModel)` and look up display data from the model: + ```csharp + var province = currentModel.Provinces[details.ProvinceId]; + var factionName = currentModel.FactionName(details.FactionId); + var hero = currentModel.Heroes[heroId]; + var affectedProvinces = currentModel.ProvincesForFaction(factionId); + ``` + +### The Problem +Notification for action N sees model state after actions 1..N-1 have been applied. If we batch diffs, notification N would see model state before ANY actions, potentially showing stale data. + +Example: +1. Action 1: Province X conquered by Faction B (was Faction A) +2. Action 2: Notification needs to show Province X's current ruler + +With batching, action 2's notification would incorrectly show Faction A. + +## Audit of Client Notification Generators + +~50 generators access `currentModel`. Key lookup patterns: + +| Lookup | Count | Mutable? | Risk | +|--------|-------|----------|------| +| `currentModel.PlayerId` | 22 | No | Safe | +| `currentModel.Provinces[id]` | 17 | Yes | `RulingFactionId` changes on conquest | +| `currentModel.Heroes[id]` | ~20 | Mostly safe | Hero data stable, used for display | +| `currentModel.FactionName(id)` | ~10 | No | Names don't change | +| `currentModel.MaybeDestroyedFaction(id)` | ~15 | Yes | Faction may be destroyed | +| `currentModel.ProvincesForFaction(id)` | ~15 | Yes | Changes on conquest | + +### State-Changing Action Types +- `ProvinceConquered` - changes province ownership +- `FactionDestroyed` - removes faction +- `FactionLeaderRemoved` - changes faction head + +## Proposed Solution: Server-Side Display Data + +Eliminate client model lookups by including all display data in server-generated notifications. + +### Current Notification Structure +```scala +case class NotificationC( + details: NotificationDetails, + targetFactionIds: Vector[FactionId], + affectedProvinceIds: Vector[ProvinceId], // Already exists, underused + affectedHeroIds: Vector[HeroId], // Already exists, underused + llm: NotificationT.Llm, + deferred: Boolean +) +``` + +### Proposed Changes + +**Option A: Add fields to NotificationC** +```scala +case class NotificationC( + details: NotificationDetails, + targetFactionIds: Vector[FactionId], + affectedProvinceIds: Vector[ProvinceId], + affectedHeroIds: Vector[HeroId], + // New fields: + factionNames: Map[FactionId, String], + displayedHeroViews: Vector[HeroView], + provinceNames: Map[ProvinceId, String], + llm: NotificationT.Llm, + deferred: Boolean +) +``` + +**Option B: Enrich each NotificationDetails type** +```scala +case class TruceAccepted( + offeringFactionId: FactionId, + offeringFactionName: String, // New + targetFactionId: FactionId, + targetFactionName: String, // New + ambassadorHeroId: HeroId, + ambassadorHeroView: HeroView // New +) +``` + +### Implementation Steps + +1. **Proto changes**: Add new fields to `Notification` message +2. **Server**: Populate display fields when creating notifications +3. **Client**: Update ~50 generators to use notification fields +4. **Test**: Add test that greps for `currentModel.` access in generators (excluding `PlayerId`) +5. **Server optimization**: With client decoupled from model state, batch diffs in `filterForPlayer` + +### Trade-offs + +**Pros:** +- Clean separation: server provides all display data +- Enables diff batching optimization +- Easier to reason about notification correctness +- Test can enforce the invariant + +**Cons:** +- Larger notification messages (includes names, hero views) +- Proto changes required +- ~50 generators need updating (mechanical but tedious) +- Server must know what display data each notification type needs + +## Alternative Approaches Considered + +### A: Selective Per-Action Diffs +Only generate individual diffs for action results with notifications that need mutable model state. Requires tracking which notification types need which state. + +**Rejected because:** Fragile; easy to add a new generator that breaks the invariant. + +### B: State-Change-Triggered Diffs +If batch contains state-changing action types (ProvinceConquered, etc.), generate individual diffs from that point. Otherwise batch. + +**Rejected because:** Still conservative; many batches would fall back to individual diffs. + +## Status + +**Deferred** - Current performance is acceptable. This doc captures the analysis for future reference if optimization becomes necessary. + +## References + +- `ActionResultFilter.scala` - Server-side filtering +- `EagleGameModel.cs` - Client-side model updates +- `Assets/Eagle/Notifications/` - All notification generators +- `NotificationT.scala` - Server notification types