Fix memory leak in experimental mutator engine's seen cache#11121
Fix memory leak in experimental mutator engine's seen cache#11121Copilot wants to merge 1 commit into
Conversation
commit: |
|
You can try these changes here
|
…mutation The experimental mutator engine's module-level `seen` cache was never cleared, pinning the type graph of every mutated program in memory for the lifetime of the process. Scoping it per-engine (as previously attempted) breaks recursive type graphs, because mutators such as @typespec/http's merge-patch transform re-enter mutateSubgraph and rely on the cache being shared across the nested engines to terminate cycles. Instead, keep the cache shared while a logical mutation runs and clear it once the outermost mutation completes, tracked via a synchronous depth counter. This frees the mutated type graph for garbage collection without reintroducing unbounded recursion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
9364203 to
7e1949f
Compare
|
All changed packages have been documented.
Show changes
|
| ): { realm: Realm | null; type: MutableTypeWithNamespace } { | ||
| const engine = createMutatorEngine(program, mutators, { | ||
| mutateNamespaces: true, | ||
| return runRootMutation(() => { |
There was a problem hiding this comment.
Another option thinking if it was working before, would using the program as a key in a weakmap to cache all those 3 customkeymap and retrieve them work? This is the pattern we have been doing when needing this kind of "global" state
There was a problem hiding this comment.
I think the argument here is around should this cache be within a call to mutateSubgraph (the behavior in this PR) or within the lifetime of the Program? While we can certainly cache things for the duration of the Program, this is going to incur a much larger cost across a large spec that mutates many models.
Furthermore, because the cache is shared across calls a second call to mutateSubgraph gets a cache hit from the first call and you get back a type from the realm created by the first call that doesn't exist in the realm created by the second.
This is actually a flaw in the current design, if I'm understanding the intent of how mutateSubgraph is supposed to work, given its assertion from its ref doc that it @returns an object containing the mutated type and a nullable Realm in which the mutated type resides. See the test for does not share the mutation cache across top-level calls which would fail if we moved to a WeakMap on program.
There was a problem hiding this comment.
that test is added exactly in this PR though no? this define this spec right here, isn't that incorrect with the original intent?
timotheeguerin
left a comment
There was a problem hiding this comment.
Actually didn't realized the test was added there
The experimental mutator engine (
packages/compiler/src/experimental/mutators.ts) holds a module-level, never-cleared, strongly-referencedseencache. EverymutateSubgraph/mutateSubgraphWithNamespacecall insertsTypevalues into it, pinning the entire type-graph of every mutated program in memory for the process lifetime — heap grows unbounded for hosts that drivecompile()in a loop (e.g. emitter test suites, versioning mutation via TCGC).Why the cache can't simply be scoped per-engine
The obvious fix — moving the cache into
createMutatorEngineso it dies with the engine — breaks recursive type graphs. Mutators such as@typespec/http's merge-patch transform callmutateSubgraphre-entrantly from inside their ownmutatefunction, and each call spins up a fresh engine. Theseencache must be shared across those nested engines so that a self-referential type (e.g.model Resource { related?: Record<Resource> }) is cloned once instead of recursing forever. A per-engine cache regresses this intoRangeError: Maximum call stack size exceeded(observed in the@typespec/httpmerge-patch and@typespec/samplesvisibility suites).Changes
seencache stays module-scoped (so it is shared across nested re-entrant mutations and continues to break cycles), but amutationDepthcounter tracks re-entrancy and clears the cache once the outermostmutateSubgraph/mutateSubgraphWithNamespacecall returns. The cache — and everyTypeit references — therefore becomes eligible for GC once the mutation pass completes, instead of living for the process lifetime. Mutations run synchronously, so the depth counter reliably identifies the outermost call, and clearing happens even if a mutator throws.CustomKeyMap.clear()— small helper used to reset the cache.packages/compiler/test/experimental/mutator.test.ts):does not share the mutation cache across top-level calls— two independent top-level mutations of the same type/mutator produce distinct clones in distinct realms.breaks cycles across re-entrant mutateSubgraph calls— a self-referential model run through a mutator that re-entersmutateSubgraph(mirroring merge-patch) must not overflow the stack. Verified to fail withRangeError: Maximum call stack size exceededunder a per-engine cache, confirming it guards the regression.This fixes the leak without reintroducing unbounded recursion, and keeps separate top-level mutations independent (each gets a clean cache and its own realm).