From 71956250ddc7faee2a813c293f4ebf91a1509afa Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Mon, 29 Jun 2020 14:27:25 -0700 Subject: [PATCH] perf(compiler-cli): fix memory leak in retained incremental state (#37835) Incremental compilation allows for the output state of one compilation to be reused as input to the next compilation. This involves retaining references to instances from prior compilations, which must be done carefully to avoid memory leaks. This commit fixes such a leak with a complicated retention chain: * `TrackedIncrementalBuildStrategy` unnecessarily hangs on to the previous `IncrementalDriver` (state of the previous compilation) once the current compilation completes. In general this is unnecessary, but should be safe as long as the chain only goes back one level - if the `IncrementalDriver` doesn't retain any previous `TrackedIncrementalBuildStrategy` instances. However, this does happen: * `NgCompiler` indirectly causes retention of previous `NgCompiler` instances (and thus previous `TrackedIncrementalBuildStrategy` instances) through accidental capture of the `this` context in a closure created in its constructor. This closure is wrapped in a `ts.ModuleResolutionCache` used to create a `ModuleResolver` class, which is passed to the program's `TraitCompiler` on construction. * The `IncrementalDriver` retains a reference to the `TraitCompiler` of the previous compilation, completing the reference chain. The final retention chain thus looks like: * `TrackedIncrementalBuildStrategy` of current program * `.previous`: `IncrementalDriver` of previous program * `.lastGood.traitCompiler`: `TraitCompiler` * `.handlers[..].moduleResolver.moduleResolutionCache`: cache * (via `getCanonicalFileName` closure): `NgCompiler` * `.incrementalStrategy`: `TrackedIncrementalBuildStrategy` of previous program. The closure link is the "real" leak here. `NgCompiler` is creating a closure for `getCanonicalFileName`, delegating to its `this.adapter.getCanonicalFileName`, for the purposes of creating a `ts.ModuleResolutionCache`. The fact that the closure references `NgCompiler` thus eventually causes previous `NgCompiler` iterations to be retained. This is also potentially problematic due to the shared nature of `ts.ModuleResolutionCache`, which is potentially retained across multiple compilations intentionally. This commit fixes the first two links in the retention chain: the build strategy is patched to not retain a `previous` pointer, and the `NgCompiler` is patched to not create a closure in the first place, but instead pass a bound function. This ensures that the `NgCompiler` does not retain previous instances of itself in the first place, even if the build strategy does end up retaining the previous incremental state unnecessarily. The third link (`IncrementalDriver` unnecessarily retaining the whole `TraitCompiler`) is not addressed in this commit as it's a more architectural problem that will require some refactoring. However, the leak potential of this retention is eliminated thanks to fixing the first two issues. PR Close #37835 --- packages/compiler-cli/src/ngtsc/core/src/compiler.ts | 8 +++++++- .../src/ngtsc/incremental/src/strategy.ts | 12 +++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts index bcb9b6a470..c5077cb3fa 100644 --- a/packages/compiler-cli/src/ngtsc/core/src/compiler.ts +++ b/packages/compiler-cli/src/ngtsc/core/src/compiler.ts @@ -116,7 +116,13 @@ export class NgCompiler { const moduleResolutionCache = ts.createModuleResolutionCache( this.adapter.getCurrentDirectory(), - fileName => this.adapter.getCanonicalFileName(fileName)); + // Note: this used to be an arrow-function closure. However, JS engines like v8 have some + // strange behaviors with retaining the lexical scope of the closure. Even if this function + // doesn't retain a reference to `this`, if other closures in the constructor here reference + // `this` internally then a closure created here would retain them. This can cause major + // memory leak issues since the `moduleResolutionCache` is a long-lived object and finds its + // way into all kinds of places inside TS internal objects. + this.adapter.getCanonicalFileName.bind(this.adapter)); this.moduleResolver = new ModuleResolver(tsProgram, this.options, this.adapter, moduleResolutionCache); this.resourceManager = new AdapterResourceLoader(adapter, this.options); diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/strategy.ts b/packages/compiler-cli/src/ngtsc/incremental/src/strategy.ts index 8e8b3379f6..b7d60b9856 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/strategy.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/strategy.ts @@ -42,20 +42,22 @@ export class NoopIncrementalBuildStrategy implements IncrementalBuildStrategy { * Tracks an `IncrementalDriver` within the strategy itself. */ export class TrackedIncrementalBuildStrategy implements IncrementalBuildStrategy { - private previous: IncrementalDriver|null = null; - private next: IncrementalDriver|null = null; + private driver: IncrementalDriver|null = null; + private isSet: boolean = false; getIncrementalDriver(): IncrementalDriver|null { - return this.next !== null ? this.next : this.previous; + return this.driver; } setIncrementalDriver(driver: IncrementalDriver): void { - this.next = driver; + this.driver = driver; + this.isSet = true; } toNextBuildStrategy(): TrackedIncrementalBuildStrategy { const strategy = new TrackedIncrementalBuildStrategy(); - strategy.previous = this.next; + // Only reuse a driver that was explicitly set via `setIncrementalDriver`. + strategy.driver = this.isSet ? this.driver : null; return strategy; } }