From e24c6aefc87842c560c613dcefa8c4ffdc3b91b6 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Wed, 17 May 2023 10:13:26 -0700 Subject: [PATCH] Add unstable_transformResultKey and prune unmodified modules from deltas Summary: `Graph.traverseDependencies`, at the core of delta calculation and therefore Fast Refresh and incremental builds, relies on being given a list of modified file paths, and derives a graph delta from them. Currently, if a path corresponds to a module in a given graph, that module will be returned as a "modified" module, serialised, and sent to the client for execution. This diff introduces modified module pruning, by a) being more selective about when we regard a module as *potentially* modified during traversal, and then b) by actually diffing the modules as a final pass before returning a delta. We do this by storing the transformer's cache key as a "`transformResultKey`", diffing that, and also diffing dependencies and inverse dependencies to account for resolution changes. ## Package exports and symlinks Because we don't *yet* have smart-enough incremental invalidation of resolutions, we have to invalidate the whole of every graph for any symlink or `package.json#exports` change. We implement this by calling `Graph.traverseDependencies` with *every* path in the graph. Currently, without pruning unmodified modules, this results in enormous deltas of hundreds of nodes. With pruning, although we must still re-traverse and re-resolve everything, we can avoid sending anything to the client unless there are any actual changes (and then, only send the minimum), and consequently do much less injection/execution on the client. This results in correct Fast Refresh of symlink and `package.json#exports` changes in subsecond time, and (IMO) unblocks symlinks-by-default. ``` - **[Performance]**: Prune unmodified modules from delta updates before sending them to the client ``` Reviewed By: Andjeliko Differential Revision: D45691844 fbshipit-source-id: df09a3ec9016f691ef0bfaea5a723a9ec57d9d79 --- .../metro/src/DeltaBundler/DeltaCalculator.js | 7 + packages/metro/src/DeltaBundler/Graph.js | 129 +++++++++++++----- .../metro/src/DeltaBundler/Transformer.js | 1 + .../src/DeltaBundler/__tests__/Graph-test.js | 42 ++++-- .../__snapshots__/Graph-test.js.snap | 5 + packages/metro/src/DeltaBundler/types.flow.js | 16 ++- 6 files changed, 153 insertions(+), 47 deletions(-) diff --git a/packages/metro/src/DeltaBundler/DeltaCalculator.js b/packages/metro/src/DeltaBundler/DeltaCalculator.js index 6b371b9452..da104ac7e7 100644 --- a/packages/metro/src/DeltaBundler/DeltaCalculator.js +++ b/packages/metro/src/DeltaBundler/DeltaCalculator.js @@ -323,10 +323,17 @@ class DeltaCalculator extends EventEmitter { }; } + debug('Traversing dependencies for %s paths', modifiedDependencies.length); const {added, modified, deleted} = await this._graph.traverseDependencies( modifiedDependencies, this._options, ); + debug( + 'Calculated graph delta {added: %s, modified: %d, deleted: %d}', + added.size, + modified.size, + deleted.size, + ); return { added, diff --git a/packages/metro/src/DeltaBundler/Graph.js b/packages/metro/src/DeltaBundler/Graph.js index 3361c9d759..a5506e3251 100644 --- a/packages/metro/src/DeltaBundler/Graph.js +++ b/packages/metro/src/DeltaBundler/Graph.js @@ -176,17 +176,24 @@ export class Graph { // Record the paths that are part of the dependency graph before we start // traversing - we'll use this to ensure we don't report modules modified - // that only exist as part of the graph mid-traversal. - const existingPaths = paths.filter(path => this.dependencies.has(path)); + // that only exist as part of the graph mid-traversal, and to eliminate + // modules that end up in the same state that they started from the delta. + const originalModules = new Map>(); + for (const path of paths) { + const originalModule = this.dependencies.get(path); + if (originalModule) { + originalModules.set(path, originalModule); + } + } - for (const path of existingPaths) { + for (const [path] of originalModules) { // Traverse over modules that are part of the dependency graph. // // Note: A given path may not be part of the graph *at this time*, in // particular it may have been removed since we started traversing, but // in that case the path will be visited if and when we add it back to // the graph in a subsequent iteration. - if (this.dependencies.get(path)) { + if (this.dependencies.has(path)) { await this._traverseDependenciesForSingleFile( path, delta, @@ -208,17 +215,35 @@ export class Graph { // but may not actually differ, may be new, or may have been deleted after // processing. The actually-modified modules are the intersection of // delta.modified with the pre-existing paths, minus modules deleted. - for (const path of existingPaths) { + for (const [path, originalModule] of originalModules) { invariant( !delta.added.has(path), 'delta.added has %s, but this path was already in the graph.', path, ); if (delta.modified.has(path)) { - // Only report a module as modified if we're not already reporting it - // as added or deleted. + // It's expected that a module may be both modified and subsequently + // deleted - we'll only return it as deleted. if (!delta.deleted.has(path)) { - modified.set(path, nullthrows(this.dependencies.get(path))); + // If a module existed before and has not been deleted, it must be + // in the dependencies map. + const newModule = nullthrows(this.dependencies.get(path)); + if ( + // Module.dependencies is mutable, so it's not obviously the case + // that referential equality implies no modification. However, we + // only mutate dependencies in two cases: + // 1. Within _processModule. In that case, we always mutate a new + // module and set a new reference in this.dependencies. + // 2. During _releaseModule, when recursively removing + // dependencies. In that case, we immediately discard the module + // object. + // TODO: Refactor for more explicit immutability + newModule !== originalModule || + transfromOutputMayDiffer(newModule, originalModule) || + !allDependenciesEqual(newModule, originalModule) + ) { + modified.set(path, newModule); + } } } } @@ -290,10 +315,6 @@ export class Graph { ): Promise> { const resolvedContext = this.#resolvedContexts.get(path); - // Mark any module processed as potentially modified. Once we've finished - // traversing we'll filter this set down. - delta.modified.add(path); - // Transform the file via the given option. // TODO: Unbind the transform method from options const result = await options.transform(path, resolvedContext); @@ -306,48 +327,67 @@ export class Graph { options, ); - const previousModule = this.dependencies.get(path) || { - inverseDependencies: - delta.earlyInverseDependencies.get(path) || new CountingSet(), - path, - }; - const previousDependencies = previousModule.dependencies || new Map(); + const previousModule = this.dependencies.get(path); - // Update the module information. - const module = { - ...previousModule, + const previousDependencies = previousModule?.dependencies ?? new Map(); + + const nextModule = { + ...(previousModule ?? { + inverseDependencies: + delta.earlyInverseDependencies.get(path) ?? new CountingSet(), + path, + }), dependencies: new Map(previousDependencies), getSource: result.getSource, output: result.output, + unstable_transformResultKey: result.unstable_transformResultKey, }; - this.dependencies.set(module.path, module); + + // Update the module information. + this.dependencies.set(nextModule.path, nextModule); // Diff dependencies (1/2): remove dependencies that have changed or been removed. + let dependenciesRemoved = false; for (const [key, prevDependency] of previousDependencies) { const curDependency = currentDependencies.get(key); if ( !curDependency || !dependenciesEqual(prevDependency, curDependency, options) ) { - this._removeDependency(module, key, prevDependency, delta, options); + dependenciesRemoved = true; + this._removeDependency(nextModule, key, prevDependency, delta, options); } } // Diff dependencies (2/2): add dependencies that have changed or been added. - const promises = []; + const addDependencyPromises = []; for (const [key, curDependency] of currentDependencies) { const prevDependency = previousDependencies.get(key); if ( !prevDependency || !dependenciesEqual(prevDependency, curDependency, options) ) { - promises.push( - this._addDependency(module, key, curDependency, delta, options), + addDependencyPromises.push( + this._addDependency(nextModule, key, curDependency, delta, options), ); } } - await Promise.all(promises); + if ( + previousModule && + !transfromOutputMayDiffer(previousModule, nextModule) && + !dependenciesRemoved && + addDependencyPromises.length === 0 + ) { + // We have not operated on nextModule, so restore previousModule + // to aid diffing. + this.dependencies.set(previousModule.path, previousModule); + return previousModule; + } + + delta.modified.add(path); + + await Promise.all(addDependencyPromises); // Replace dependencies with the correctly-ordered version. As long as all // the above promises have resolved, this will be the same map but without @@ -357,13 +397,13 @@ export class Graph { // Catch obvious errors with a cheap assertion. invariant( - module.dependencies.size === currentDependencies.size, + nextModule.dependencies.size === currentDependencies.size, 'Failed to add the correct dependencies', ); - module.dependencies = currentDependencies; + nextModule.dependencies = currentDependencies; - return module; + return nextModule; } async _addDependency( @@ -470,7 +510,10 @@ export class Graph { /** * Collect a list of context modules which include a given file. */ - markModifiedContextModules(filePath: string, modifiedPaths: Set) { + markModifiedContextModules( + filePath: string, + modifiedPaths: Set | CountingSet, + ) { for (const [absolutePath, context] of this.#resolvedContexts) { if ( !modifiedPaths.has(absolutePath) && @@ -814,6 +857,23 @@ function dependenciesEqual( ); } +function allDependenciesEqual( + a: Module, + b: Module, + options: $ReadOnly<{lazy: boolean, ...}>, +): boolean { + if (a.dependencies.size !== b.dependencies.size) { + return false; + } + for (const [key, depA] of a.dependencies) { + const depB = b.dependencies.get(key); + if (!depB || !dependenciesEqual(depA, depB, options)) { + return false; + } + } + return true; +} + function contextParamsEqual( a: ?RequireContextParams, b: ?RequireContextParams, @@ -829,3 +889,10 @@ function contextParamsEqual( a.mode === b.mode) ); } + +function transfromOutputMayDiffer(a: Module, b: Module): boolean { + return ( + a.unstable_transformResultKey == null || + a.unstable_transformResultKey !== b.unstable_transformResultKey + ); +} diff --git a/packages/metro/src/DeltaBundler/Transformer.js b/packages/metro/src/DeltaBundler/Transformer.js index 50b293b59f..8f7a9dbf86 100644 --- a/packages/metro/src/DeltaBundler/Transformer.js +++ b/packages/metro/src/DeltaBundler/Transformer.js @@ -155,6 +155,7 @@ class Transformer { return { ...data.result, + unstable_transformResultKey: fullKey.toString(), getSource(): Buffer { if (fileBuffer) { return fileBuffer; diff --git a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js b/packages/metro/src/DeltaBundler/__tests__/Graph-test.js index 10b8f07a76..dc1b814a01 100644 --- a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/Graph-test.js @@ -43,7 +43,14 @@ let mockedDependencyTree: Map< }>, >, > = new Map(); -const files = new Set(); + +/* `files` emulates the changed paths typically aggregated by DeltaCalcutor. + * Paths will be added to this set by any addition, deletion or modification, + * respecting getModifiedModulesForDeletedPath. Each such operation will + * increment the count - we'll intepret count as a file revision number, with + * a changed count reflected in a change to the transform output key. + */ +const files = new CountingSet(); let graph: TestGraph; let options; @@ -161,11 +168,14 @@ const Actions = { }, }; -function deferred(value: { - +dependencies: $ReadOnlyArray, - +getSource: () => Buffer, - +output: $ReadOnlyArray, -}) { +function deferred( + value: $ReadOnly<{ + dependencies: $ReadOnlyArray, + getSource: () => Buffer, + output: $ReadOnlyArray, + unstable_transformResultKey?: ?string, + }>, +) { let resolve; const promise = new Promise(res => (resolve = res)); @@ -249,7 +259,7 @@ class TestGraph extends Graph<> { ): Promise> { // Get a snapshot of the graph before the traversal. const dependenciesBefore = new Set(this.dependencies.keys()); - const pathsBefore = new Set(paths); + const modifiedPaths = new Set(files); // Mutate the graph and calculate a delta. const delta = await super.traverseDependencies(paths, options); @@ -258,7 +268,7 @@ class TestGraph extends Graph<> { const expectedDelta = computeDelta( dependenciesBefore, this.dependencies, - pathsBefore, + modifiedPaths, ); expect(getPaths(delta)).toEqual(expectedDelta); @@ -294,6 +304,17 @@ beforeEach(async () => { Promise>, >() .mockImplementation(async (path: string, context: ?RequireContext) => { + const unstable_transformResultKey = + path + + (context + ? // For context modules, the real transformer will hash the + // generated template, which varies according to its dependencies. + // Approximate that by concatenating dependency paths. + (mockedDependencyTree.get(path) ?? []) + .map(d => d.path) + .sort() + .join('|') + : ` (revision ${files.count(path)})`); return { dependencies: (mockedDependencyTree.get(path) || []).map(dep => ({ name: dep.name, @@ -318,6 +339,7 @@ beforeEach(async () => { type: 'js/module', }, ], + unstable_transformResultKey, }; }); @@ -407,7 +429,7 @@ it('should return an empty result when there are no changes', async () => { getPaths(await graph.traverseDependencies(['/bundle'], options)), ).toEqual({ added: new Set(), - modified: new Set(['/bundle']), + modified: new Set([]), deleted: new Set(), }); }); @@ -1959,6 +1981,7 @@ describe('edge cases', () => { │ /baz │ └──────┘ */ + mockTransform.mockClear(); expect( getPaths(await graph.traverseDependencies([...files], localOptions)), ).toEqual({ @@ -1966,6 +1989,7 @@ describe('edge cases', () => { modified: new Set(['/bundle']), deleted: new Set([]), }); + expect(mockTransform).toHaveBeenCalledWith('/bundle', undefined); }); }); diff --git a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/Graph-test.js.snap b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/Graph-test.js.snap index bce2f05724..26b4313db5 100644 --- a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/Graph-test.js.snap +++ b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/Graph-test.js.snap @@ -30,6 +30,7 @@ TestGraph { }, ], "path": "/bundle", + "unstable_transformResultKey": "/bundle (revision 0)", }, "/foo" => Object { "dependencies": Map { @@ -71,6 +72,7 @@ TestGraph { }, ], "path": "/foo", + "unstable_transformResultKey": "/foo (revision 0)", }, "/bar" => Object { "dependencies": Map {}, @@ -89,6 +91,7 @@ TestGraph { }, ], "path": "/bar", + "unstable_transformResultKey": "/bar (revision 0)", }, "/baz" => Object { "dependencies": Map {}, @@ -107,6 +110,7 @@ TestGraph { }, ], "path": "/baz", + "unstable_transformResultKey": "/baz (revision 0)", }, }, "entryPoints": Set { @@ -153,6 +157,7 @@ TestGraph { }, ], "path": "/bundle", + "unstable_transformResultKey": "/bundle (revision 0)", }, }, "entryPoints": Set { diff --git a/packages/metro/src/DeltaBundler/types.flow.js b/packages/metro/src/DeltaBundler/types.flow.js index f45497fc52..7aea4dbd90 100644 --- a/packages/metro/src/DeltaBundler/types.flow.js +++ b/packages/metro/src/DeltaBundler/types.flow.js @@ -61,13 +61,14 @@ export type Dependency = { +data: TransformResultDependency, }; -export type Module = { - +dependencies: Map, - +inverseDependencies: CountingSet, - +output: $ReadOnlyArray, - +path: string, - +getSource: () => Buffer, -}; +export type Module = $ReadOnly<{ + dependencies: Map, + inverseDependencies: CountingSet, + output: $ReadOnlyArray, + path: string, + getSource: () => Buffer, + unstable_transformResultKey?: ?string, +}>; export type Dependencies = Map>; export type ReadOnlyDependencies = $ReadOnlyMap< @@ -102,6 +103,7 @@ export type {Graph}; export type TransformResult = $ReadOnly<{ dependencies: $ReadOnlyArray, output: $ReadOnlyArray, + unstable_transformResultKey?: ?string, }>; export type TransformResultWithSource = $ReadOnly<{