Avoid traversing the whole tree to normalize pseudo globals

Summary:
The `normalizePseudoGlobals` renames the `__d` function params to shorter names, e.g. `$$__REQUIRE` to `r`.

The old implementation traversed the whole AST and renamed any existing usages / declarations, e.g. it renamed all existing references to `r`.

Traversing the full AST only to rename a potential conflicting declaration is rather expensive. This diff changes the plugin so that it generates a new unique name in case the name for a function argument, e.g. `r` is already used in the module. This is cheaper because it only requires to traverse the AST once to build up the scope.

This might arguably result in larger file size in case the function argument (e.g. `require`) is heavily used. However, renaming the existing declaration might also result in larger file sizes if that declaration is heavily used.

Overall: The current implementation optimizes for an edge case where one letter variables are used in the program. I would argue that it isn't worth spending many cpu-cycles on this edge case.

Win per Worker on an ondemand.

{F346854274}

Following the result for building MarketplaceHome.entrypoint on my ondemand using a local metronome build with `--reset-cache`.

|        | Run 1  | Run 2  | Run 3  |
|--------|--------|--------|--------|
| Before | 1m 14s | 1m 14s | 1m 13s |
| After  | 1m 11s | 1m 12s | 1m 12s |

Reviewed By: cpojer

Differential Revision: D25176777

fbshipit-source-id: 49245fa03987e2eb219a9d0dffb690fecb71093b

packages/metro-transform-plugins/src/__tests__/normalizePseudoGlobals-test.js

@@ -45,7 +45,7 @@ it('minimizes arguments given', () => {
 
       (function() {
         {
-          const r = 1; // _$$_REQUIRE will be renamed to "r".
+          const r = 1; // _$$_REQUIRE will be renamed to "_r".
           return r++;
         }
       })();
@@ -57,16 +57,16 @@ it('minimizes arguments given', () => {
     })
   `);
 
-  expect(result.reserved).toEqual(['g', 'r', 'm', 'e', 'd']);
+  expect(result.reserved).toEqual(['g', '_r', 'm', 'e', 'd']);
   expect(result.code).toMatchInlineSnapshot(`
-    "__d(function (g, r, m, e, d) {
-      r(27).foo();
+    "__d(function (g, _r, m, e, d) {
+      _r(27).foo();
 
       (function () {
         {
-          const _r4 = 1; // _$$_REQUIRE will be renamed to \\"r\\".
+          const r = 1; // _$$_REQUIRE will be renamed to \\"_r\\".
 
-          return _r4++;
+          return r++;
         }
       })();
 

packages/metro-transform-plugins/src/normalizePseudoGlobals.js

@@ -11,87 +11,94 @@
 'use strict';
 
 const traverse = require('@babel/traverse').default;
+const nullthrows = require('nullthrows');
 
-import type {NodePath} from '@babel/traverse';
+import type {NodePath, Scope} from '@babel/traverse';
 import type {Program} from '@babel/types';
 
 function normalizePseudoglobals(ast: BabelNode): $ReadOnlyArray<string> {
-  let pseudoglobals: Array<string> = [];
-  const reserved = [];
-  let params = null;
-  let body: ?NodePath<> = null;
-
+  const renamedParamNames = [];
   traverse(ast, {
-    Program: {
-      enter(path: NodePath<Program>): void {
-        params = path.get('body.0.expression.arguments.0.params');
-        const bodyPath = path.get('body.0.expression.arguments.0.body');
-
-        if (!bodyPath || Array.isArray(bodyPath) || !Array.isArray(params)) {
-          params = null;
-          body = null;
-
-          return;
-        } else {
-          body = bodyPath;
-        }
-
-        // $FlowFixMe Flow error uncovered by typing Babel more strictly
-        pseudoglobals = params.map(path => path.node.name);
-
-        for (let i = 0; i < pseudoglobals.length; i++) {
-          // Try finding letters that are semantically relatable to the name
-          // of the variable given. For instance, in XMLHttpRequest, it will
-          // first match "X", then "H", then "R".
-          const regexp = /^[^A-Za-z]*([A-Za-z])|([A-Z])[a-z]|([A-Z])[A-Z]+$/g;
-          let match;
-
-          while ((match = regexp.exec(pseudoglobals[i]))) {
-            const name = (match[1] || match[2] || match[3] || '').toLowerCase();
-
-            if (!name) {
-              throw new ReferenceError(
-                'Could not identify any valid name for ' + pseudoglobals[i],
-              );
-            }
-
-            if (reserved.indexOf(name) === -1) {
-              reserved[i] = name;
-              break;
-            }
-          }
-        }
-
-        if (new Set(reserved).size !== pseudoglobals.length) {
-          throw new ReferenceError(
-            'Shortened variables are not unique: ' + reserved.join(', '),
-          );
-        }
-      },
-
-      exit(path: NodePath<>): void {
-        reserved.forEach((shortName: string, i: number) => {
-          if (pseudoglobals[i] && shortName && body && params) {
-            body.scope.rename(pseudoglobals[i], shortName);
-          }
-        });
-      },
-    },
+    Program(path: NodePath<Program>): void {
+      const params = path.get('body.0.expression.arguments.0.params');
+      const body = path.get('body.0.expression.arguments.0.body');
+
+      if (!body || Array.isArray(body) || !Array.isArray(params)) {
+        path.stop();
+        return;
+      }
 
-    Scope(path: NodePath<>): void {
-      path.scope.crawl();
+      // $FlowFixMe Flow error uncovered by typing Babel more strictly
+      const pseudoglobals: Array<string> = params.map(path => path.node.name);
 
-      if (body && params && path.node !== body.node) {
-        reserved.forEach((shortName: string, i: number) => {
-          if (pseudoglobals[i] && shortName) {
-            path.scope.rename(shortName, path.scope.generateUid(shortName));
-          }
-        });
+      const usedShortNames = new Set<string>();
+      const namePairs: Array<[string, string]> = pseudoglobals.map(fullName => [
+        fullName,
+        getShortName(fullName, usedShortNames),
+      ]);
+
+      for (const [fullName, shortName] of namePairs) {
+        renamedParamNames.push(rename(fullName, shortName, body.scope));
       }
+
+      path.stop();
     },
   });
 
-  return reserved;
+  return renamedParamNames;
+}
+
+function getShortName(fullName: string, usedNames: Set<string>): string {
+  // Try finding letters that are semantically relatable to the name
+  // of the variable given. For instance, in XMLHttpRequest, it will
+  // first match "X", then "H", then "R".
+  const regexp = /^[^A-Za-z]*([A-Za-z])|([A-Z])[a-z]|([A-Z])[A-Z]+$/g;
+  let match;
+
+  while ((match = regexp.exec(fullName))) {
+    const name = (match[1] || match[2] || match[3] || '').toLowerCase();
+
+    if (!name) {
+      throw new ReferenceError(
+        'Could not identify any valid name for ' + fullName,
+      );
+    }
+
+    if (!usedNames.has(name)) {
+      usedNames.add(name);
+      return name;
+    }
+  }
+
+  throw new ReferenceError(
+    `Unable to determine short name for ${fullName}. The variables are not unique: ${Array.from(
+      usedNames,
+    ).join(', ')}`,
+  );
+}
+
+function rename(fullName: string, shortName: string, scope: Scope): string {
+  let unusedName = shortName;
+
+  // `generateUid` generates a name of the form name_ even if there was no conflict which we don't want.
+  // Check if the desired name was never used and in that case proceed and only use `generateUid` if there's a
+  // name clash.
+  if (
+    scope.hasLabel(shortName) ||
+    scope.hasBinding(shortName) ||
+    scope.hasGlobal(shortName) ||
+    scope.hasReference(shortName)
+  ) {
+    unusedName = scope.generateUid(shortName);
+
+    const programScope = scope.getProgramParent();
+    nullthrows(programScope.references)[shortName] = true;
+    nullthrows(programScope.uids)[shortName] = true;
+  }
+
+  scope.rename(fullName, unusedName);
+
+  return unusedName;
 }
 
 module.exports = normalizePseudoglobals;