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

Author: robhogan

packages/metro/src/DeltaBundler/DeltaCalculator.js

@@ -323,10 +323,17 @@ class DeltaCalculator<T> 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,

packages/metro/src/DeltaBundler/Graph.js

@@ -176,17 +176,24 @@ export class Graph<T = MixedOutput> {
 
     // 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<string, Module<T>>();
+    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<T = MixedOutput> {
     // 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<T = MixedOutput> {
   ): Promise<Module<T>> {
     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<T = MixedOutput> {
       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<T = MixedOutput> {
 
     // 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<T = MixedOutput> {
   /**
    * Collect a list of context modules which include a given file.
    */
-  markModifiedContextModules(filePath: string, modifiedPaths: Set<string>) {
+  markModifiedContextModules(
+    filePath: string,
+    modifiedPaths: Set<string> | CountingSet<string>,
+  ) {
     for (const [absolutePath, context] of this.#resolvedContexts) {
       if (
         !modifiedPaths.has(absolutePath) &&
@@ -814,6 +857,23 @@ function dependenciesEqual(
   );
 }
 
+function allDependenciesEqual<T>(
+  a: Module<T>,
+  b: Module<T>,
+  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<T>(a: Module<T>, b: Module<T>): boolean {
+  return (
+    a.unstable_transformResultKey == null ||
+    a.unstable_transformResultKey !== b.unstable_transformResultKey
+  );
+}

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;

packages/metro/src/DeltaBundler/__tests__/Graph-test.js

@@ -43,7 +43,14 @@ let mockedDependencyTree: Map<
     }>,
   >,
 > = new Map();
-const files = new Set<string>();
+
+/* `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<string>();
 let graph: TestGraph;
 let options;
 
@@ -161,11 +168,14 @@ const Actions = {
   },
 };
 
-function deferred(value: {
-  +dependencies: $ReadOnlyArray<TransformResultDependency>,
-  +getSource: () => Buffer,
-  +output: $ReadOnlyArray<MixedOutput>,
-}) {
+function deferred(
+  value: $ReadOnly<{
+    dependencies: $ReadOnlyArray<TransformResultDependency>,
+    getSource: () => Buffer,
+    output: $ReadOnlyArray<MixedOutput>,
+    unstable_transformResultKey?: ?string,
+  }>,
+) {
   let resolve;
   const promise = new Promise(res => (resolve = res));
 
@@ -249,7 +259,7 @@ class TestGraph extends Graph<> {
   ): Promise<Result<MixedOutput>> {
     // 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<TransformResultWithSource<MixedOutput>>,
     >()
     .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,13 +1981,15 @@ describe('edge cases', () => {
                           │ /baz │
                           └──────┘
       */
+      mockTransform.mockClear();
       expect(
         getPaths(await graph.traverseDependencies([...files], localOptions)),
       ).toEqual({
         added: new Set([]),
         modified: new Set(['/bundle']),
         deleted: new Set([]),
       });
+      expect(mockTransform).toHaveBeenCalledWith('/bundle', undefined);
     });
   });
 

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 {