Conversation
Implements the next W2 roadmap step from docs/code-reviews/solver-memoization.md: a session-level LRU cache that persists across solve() calls and evicts entries when capacity is exceeded. Key additions in LRUCache.fsx: - LRUCache<'K,'V>: thread-safe bounded cache backed by LinkedList + Dictionary; O(1) get/put/evict; configurable capacity (default 512) - Solver.solveAllLRU: session-scoped LRU solver using canonical keys (CanonKey.ofEquation from MemoCanon.fsx) for cross-variable-name sharing - PerCallSolver.solveAllMemo: per-call baseline for comparison - Expecto correctness tests for LRUCache (eviction order, promotion, capacity, clear) - Benchmark comparing baseline / per-call / session-LRU on 10-patient dosing batch Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Greptile SummaryThis PR adds
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[solve call] --> B{sessionCache.TryGet\ncanonical key}
B -- Hit --> C[return cached\nEquation × SolveResult]
B -- Miss --> D[Equation.solve]
D --> E{Result}
E -- Unchanged --> F[sessionCache.Put\nkey → result]
F --> G[return result]
E -- Changed --> H[re-queue equations\nloop continues]
H --> B
E -- Errored --> I[raise SolverException]
subgraph LRUCache internals
J[LinkedList\nhead = MRU, tail = LRU]
K[Dictionary\nkey → LinkedListNode]
L{map.Count\n>= capacity?}
L -- Yes --> M[evict tail node\nRemoveLast + map.Remove]
L -- No --> N[list.AddFirst\nmap add]
end
F --> L
Reviews (1): Last reviewed commit: "perf(gensolver): add session-level LRU c..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Adds a new GenSOLVER prototype script implementing a session-scoped, size-bounded LRU cache to persist memoized solver results across solve() calls (roadmap step after per-call memoization and canonical keys).
Changes:
- Introduces a thread-safe
LRUCache<'K,'V>(LinkedList + Dictionary) with capacity-based eviction. - Adds
Solver.solveAllLRUthat uses canonical equation keys and a shared cache across calls. - Adds inline Expecto tests for cache behavior plus a small benchmark harness.
| let list = LinkedList<'K * 'V>() | ||
| let map = Dictionary<'K, LinkedListNode<'K * 'V>>(capacity) | ||
| let lockObj = obj () | ||
|
|
||
| do | ||
| if capacity <= 0 then | ||
| invalidArg "capacity" $"LRUCache capacity must be > 0; got {capacity}" |
There was a problem hiding this comment.
Capacity validation happens after Dictionary(capacity) is constructed. If capacity is negative, the Dictionary constructor will throw before your invalidArg runs, which makes the error inconsistent with the intended message. Validate capacity before creating map (or construct the dictionary without a preset capacity) so the failure mode is deterministic.
| let solveE n eqs (eq: Equation.T) = | ||
| let key = CanonKey.ofEquation eq | ||
|
|
||
| match sessionCache.TryGet key with | ||
| | Some cached -> | ||
| incr hits | ||
| cached | ||
| | None -> |
There was a problem hiding this comment.
solveAllLRU returns the cached (Equation * SolveResult) tuple directly on a canonical-key hit. If two structurally identical equations have different variable names, this can return an Equation from a previous call with a different name-space, which will corrupt the caller’s result list. Consider following the MemoCanon.fsx approach: on hit, use the cached SolveResult to decide whether you can return (eq, Unchanged) (preserving the caller’s equation), and re-solve (or implement remapping) for Changed/Errored instead of returning the cached equation.
| | eq :: que -> | ||
| let eq, sr = solveE n eqs eq | ||
|
|
||
| match sr with | ||
| | Unchanged -> | ||
| loop n que (Ok(eq :: acc)) | ||
| | Changed _ -> | ||
| // Re-queue changed equations (the standard solver behaviour) | ||
| loop n (acc @ que) (Ok [ eq ]) | ||
| | Errored msgs -> | ||
| Error(eq :: acc, msgs) |
There was a problem hiding this comment.
The solver loop in solveAllLRU doesn’t match the production Solver.solve loop: it doesn’t sort the queue, doesn’t skip unsolvable equations, and (most importantly) doesn’t propagate Changed variables to the other equations via Solver.replace. This will produce incorrect results for equation sets where changes must be reflected across equations. Suggest copying the loop from Solver.solve (as done in Memo.fsx/MemoCanon.fsx) and only swapping in the cache-aware solveE.
| | eq :: que -> | |
| let eq, sr = solveE n eqs eq | |
| match sr with | |
| | Unchanged -> | |
| loop n que (Ok(eq :: acc)) | |
| | Changed _ -> | |
| // Re-queue changed equations (the standard solver behaviour) | |
| loop n (acc @ que) (Ok [ eq ]) | |
| | Errored msgs -> | |
| Error(eq :: acc, msgs) | |
| | eq :: queTail -> | |
| let eqSolved, sr = solveE n eqs eq | |
| match sr with | |
| | Unchanged -> | |
| // Equation did not change; keep it and continue with the remaining queue | |
| loop n queTail (Ok(eqSolved :: acc)) | |
| | Changed changedVars -> | |
| // Propagate changed variables to all other equations, then re-queue | |
| let remaining = queTail @ acc | |
| let updated = replace changedVars remaining | |
| loop n updated (Ok [ eqSolved ]) | |
| | Errored _ -> | |
| // Skip unsolvable equations: keep the current one in the accumulator and continue | |
| loop n queTail (Ok(eqSolved :: acc)) |
| let n = n + 1 | ||
|
|
||
| if n > (que @ acc |> List.length) * Constants.MAX_LOOP_COUNT then | ||
| (n, [], que @ acc) |> Exceptions.SolverErrored |> raise |
There was a problem hiding this comment.
The “too many loops” branch builds Exceptions.SolverErrored(...) and pipes it into raise. Exceptions.SolverErrored is a Message DU case, not an exception, so this won’t type-check; it also diverges from the standard behavior in Informedica.GenSolver.Lib.Solver which raises Exceptions.SolverTooManyLoops via Exceptions.raiseExc. Please align this with the existing solver logic (log + Exceptions.raiseExc).
| (n, [], que @ acc) |> Exceptions.SolverErrored |> raise | |
| let state = (n, [], que @ acc) | |
| let msg = "Solver exceeded maximum loop count; raising SolverTooManyLoops." | |
| writeErrorMessage msg | |
| state |> Exceptions.SolverTooManyLoops |> Exceptions.raiseExc |
| let stats = | ||
| { | ||
| Hits = !hits | ||
| Misses = !misses | ||
| Evictions = 0 // tracked separately via cache.Count before/after | ||
| CacheSize = sessionCache.Count | ||
| } |
There was a problem hiding this comment.
LRUStats.Evictions is always reported as 0, but the script presents eviction as a key property. Consider tracking evictions inside LRUCache.Put (e.g., return a flag when an entry is evicted or maintain an internal counter) and wiring that through to LRUStats so benchmarks can report real eviction behavior.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
[WIP] Add session-level LRU cache prototype
🤖 This is an automated draft PR from Repo Assist.
Summary
Adds
src/Informedica.GenSOLVER.Lib/Scripts/LRUCache.fsx— the next step in the W2 solver optimisation roadmap (docs/code-reviews/solver-memoization.md).Context:
Memo.fsx(merged PR #169) added a per-call memoisation layer;MemoCanon.fsx(merged PR #170) extended it with canonical variable-name keys so structurally identical equations share cache entries. This script completes the session-level piece: the cache persists acrosssolve()calls and is bounded in size via LRU eviction.What's in LRUCache.fsx
LRUCache<'K,'V>LinkedList+Dictionarygives O(1) get/put/evict. Configurable capacity (default 512).Solver.solveAllLRUCanonKey.ofEquation). Shared across calls — warm entries survive between patient requests.PerCallSolver.solveAllMemoLRUCache: eviction order, access promotion, capacity overflow,Clear().Key properties
Remaining roadmap (in script summary)
Changedhits (currently re-solves)solveAllLRUintoSolver.fsas production solver pathTest Status
Build: infrastructure failure (paket restore requires network/credentials) — pre-existing issue, not introduced by this change. The DLLs referenced by
load.fsxare present inbin/Debug/net10.0/from a prior build.Script is a
.fsxfile only; no.fssource files modified. Run with: