Optimise the Inline Requires plugin (#9664)

* Improve performance of InlineRequires visitor * Change side-effect map to set * Removed unused type / dependency
parcel-bundler · Apr 23, 2024 · 71a8493 · 71a8493
1 parent 82ed636
commit 71a8493
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 75 deletions.
diff --git a/packages/optimizers/inline-requires/src/InlineRequires.js b/packages/optimizers/inline-requires/src/InlineRequires.js
@@ -2,27 +2,26 @@
 import {Optimizer} from '@parcel/plugin';
 import {parse, print} from '@swc/core';
 import {RequireInliningVisitor} from './RequireInliningVisitor';
-import type {SideEffectsMap} from './types';
 import nullthrows from 'nullthrows';
 import SourceMap from '@parcel/source-map';
 
-let publicIdToAssetSideEffects = null;
+let assetPublicIdsWithSideEffects = null;
 
 type BundleConfig = {|
-  publicIdToAssetSideEffects: Map<string, SideEffectsMap>,
+  assetPublicIdsWithSideEffects: Set<string>,
 |};
 
 // $FlowFixMe not sure how to anotate the export here to make it work...
 module.exports = new Optimizer<empty, BundleConfig>({
   loadBundleConfig({bundle, bundleGraph, tracer}): BundleConfig {
-    if (publicIdToAssetSideEffects !== null) {
-      return {publicIdToAssetSideEffects};
+    if (assetPublicIdsWithSideEffects !== null) {
+      return {assetPublicIdsWithSideEffects};
     }
 
-    publicIdToAssetSideEffects = new Map<string, SideEffectsMap>();
+    assetPublicIdsWithSideEffects = new Set<string>();
 
     if (!bundle.env.shouldOptimize) {
-      return {publicIdToAssetSideEffects};
+      return {assetPublicIdsWithSideEffects};
     }
 
     const measurement = tracer.createMeasurement(
@@ -32,19 +31,16 @@ module.exports = new Optimizer<empty, BundleConfig>({
     );
 
     bundleGraph.traverse(node => {
-      if (node.type === 'asset') {
+      if (node.type === 'asset' && node.value.sideEffects) {
         const publicId = bundleGraph.getAssetPublicId(node.value);
-        let sideEffectsMap = nullthrows(publicIdToAssetSideEffects);
-        sideEffectsMap.set(publicId, {
-          sideEffects: node.value.sideEffects,
-          filePath: node.value.filePath,
-        });
+        let sideEffectsMap = nullthrows(assetPublicIdsWithSideEffects);
+        sideEffectsMap.add(publicId);
       }
     });
 
     measurement && measurement.end();
 
-    return {publicIdToAssetSideEffects};
+    return {assetPublicIdsWithSideEffects};
   },
 
   async optimize({
@@ -61,7 +57,7 @@ module.exports = new Optimizer<empty, BundleConfig>({
     }
 
     try {
-      const measurement = tracer.createMeasurement(
+      let measurement = tracer.createMeasurement(
         '@parcel/optimizer-inline-requires',
         'parse',
         bundle.name,
@@ -72,12 +68,26 @@ module.exports = new Optimizer<empty, BundleConfig>({
       const visitor = new RequireInliningVisitor({
         bundle,
         logger,
-        publicIdToAssetSideEffects: bundleConfig.publicIdToAssetSideEffects,
+        assetPublicIdsWithSideEffects:
+          bundleConfig.assetPublicIdsWithSideEffects,
       });
+
+      measurement = tracer.createMeasurement(
+        '@parcel/optimizer-inline-requires',
+        'visit',
+        bundle.name,
+      );
       visitor.visitProgram(ast);
+      measurement && measurement.end();
 
       if (visitor.dirty) {
+        const measurement = tracer.createMeasurement(
+          '@parcel/optimizer-inline-requires',
+          'print',
+          bundle.name,
+        );
         const result = await print(ast, {sourceMaps: !!bundle.env.sourceMap});
+        measurement && measurement.end();
 
         let sourceMap = null;
         let resultMap = result.map;

diff --git a/packages/optimizers/inline-requires/src/RequireInliningVisitor.js b/packages/optimizers/inline-requires/src/RequireInliningVisitor.js
@@ -10,13 +10,12 @@ import type {
   Identifier,
 } from '@swc/core';
 import {Visitor} from '@swc/core/Visitor';
-import type {SideEffectsMap} from './types';
 import nullthrows from 'nullthrows';
 
 type VisitorOpts = {|
   bundle: NamedBundle,
   logger: PluginLogger,
-  publicIdToAssetSideEffects: Map<string, SideEffectsMap>,
+  assetPublicIdsWithSideEffects: Set<string>,
 |};
 
 export class RequireInliningVisitor extends Visitor {
@@ -26,17 +25,17 @@ export class RequireInliningVisitor extends Visitor {
   dirty: boolean;
   logger: PluginLogger;
   bundle: NamedBundle;
-  publicIdToAssetSideEffects: Map<string, SideEffectsMap>;
+  assetPublicIdsWithSideEffects: Set<string>;
 
-  constructor({bundle, logger, publicIdToAssetSideEffects}: VisitorOpts) {
+  constructor({bundle, logger, assetPublicIdsWithSideEffects}: VisitorOpts) {
     super();
     this.currentModuleNode = null;
     this.moduleVariables = new Set();
     this.moduleVariableMap = new Map();
     this.dirty = false;
     this.logger = logger;
     this.bundle = bundle;
-    this.publicIdToAssetSideEffects = publicIdToAssetSideEffects;
+    this.assetPublicIdsWithSideEffects = assetPublicIdsWithSideEffects;
   }
 
   visitFunctionExpression(n: FunctionExpression): FunctionExpression {
@@ -48,19 +47,14 @@ export class RequireInliningVisitor extends Visitor {
     // and also reset the module variable tracking data structures.
     //
     // (TODO: Support arrow functions if we modify the runtime to output arrow functions)
-    // For ease of comparison, map the arg identifiers to an array of strings.. this will skip any
-    // functions with non-identifier args (e.g. spreads etc..)
-    const args = n.params.map(param => {
-      if (param.pat.type === 'Identifier') {
-        return param.pat.value;
-      }
-      return null;
-    });
-
     if (
-      args[0] === 'require' &&
-      args[1] === 'module' &&
-      args[2] === 'exports'
+      n.params.length === 3 &&
+      n.params[0].pat.type === 'Identifier' &&
+      n.params[0].pat.value === 'require' &&
+      n.params[1].pat.type === 'Identifier' &&
+      n.params[1].pat.value === 'module' &&
+      n.params[2].pat.type === 'Identifier' &&
+      n.params[2].pat.value === 'exports'
     ) {
       // `inModuleDefinition` is either null, or the module definition node
       this.currentModuleNode = n;
@@ -82,7 +76,7 @@ export class RequireInliningVisitor extends Visitor {
     // We're looking for variable declarations that look like this:
     //
     // `var $acw62 = require("acw62");`
-    let unusedDeclIndexes = [];
+    let needsReplacement = false;
     for (let i = 0; i < n.declarations.length; i++) {
       let decl = n.declarations[i];
       const init = decl.init;
@@ -111,41 +105,24 @@ export class RequireInliningVisitor extends Visitor {
         //
         // This won't work in dev mode, because the id used to require the asset isn't the public id
         if (
-          !this.publicIdToAssetSideEffects ||
-          !this.publicIdToAssetSideEffects.has(assetPublicId)
+          this.assetPublicIdsWithSideEffects &&
+          this.assetPublicIdsWithSideEffects.has(assetPublicId)
         ) {
-          this.logger.warn({
-            message: `${this.bundle.name}: Unable to resolve ${assetPublicId} to an asset! Assuming sideEffects are present.`,
-          });
-        } else {
-          const asset = nullthrows(
-            this.publicIdToAssetSideEffects.get(assetPublicId),
-          );
-          if (asset.sideEffects) {
-            this.logger.verbose({
-              message: `Skipping optimisation of ${assetPublicId} (${asset.filePath}) as it declares sideEffects`,
-            });
-            // eslint-disable-next-line no-continue
-            continue;
-          }
+          continue;
         }
 
         // The moduleVariableMap contains a mapping from (e.g. $acw62 -> the AST node `require("acw62")`)
         this.moduleVariableMap.set(variable, init);
         // The moduleVariables set is just the used set of modules (e.g. `$acw62`)
         this.moduleVariables.add(variable);
 
-        this.logger.verbose({
-          message: `${this.bundle.name}: Found require of ${variable} for replacement`,
-        });
-
         // Replace this with a null declarator, we'll use the `init` where it's declared.
         //
         // This mutates `var $acw62 = require("acw62")` -> `var $acw62 = null`
         //
         // The variable will be unused and removed by optimisation
         decl.init = undefined;
-        unusedDeclIndexes.push(i);
+        needsReplacement = true;
       } else if (
         decl.id.type === 'Identifier' &&
         typeof decl.id.value === 'string' &&
@@ -183,10 +160,10 @@ export class RequireInliningVisitor extends Visitor {
         this.moduleVariables.add(variable);
 
         decl.init = undefined;
-        unusedDeclIndexes.push(i);
+        needsReplacement = true;
       }
     }
-    if (unusedDeclIndexes.length === 0) {
+    if (!needsReplacement) {
       return super.visitVariableDeclaration(n);
     } else {
       this.dirty = true;

diff --git a/packages/optimizers/inline-requires/src/types.js b/packages/optimizers/inline-requires/src/types.js
diff --git a/packages/optimizers/inline-requires/test/RequireInliningVisitor.test.js b/packages/optimizers/inline-requires/test/RequireInliningVisitor.test.js
@@ -3,18 +3,15 @@ import {RequireInliningVisitor} from '../src/RequireInliningVisitor';
 import assert from 'assert';
 import logger from '@parcel/logger';
 
-async function testRequireInliningVisitor(src, sideEffectsMap) {
+async function testRequireInliningVisitor(src, sideEffects) {
   const ast = await parse(src, {});
-  const publicIdToAssetSideEffects = new Map();
-  for (const [id, hasSideEffects] of Object.entries(sideEffectsMap)) {
-    publicIdToAssetSideEffects.set(id, {sideEffects: hasSideEffects});
-  }
+  const assetPublicIdsWithSideEffects = new Set(sideEffects);
 
   const visitor = new RequireInliningVisitor({
     bundle: {
       name: 'test-bundle',
     },
-    publicIdToAssetSideEffects,
+    assetPublicIdsWithSideEffects,
     logger,
   });
   visitor.visitProgram(ast);
@@ -46,9 +43,7 @@ describe('InliningVisitor', () => {
     const src = getModule(`
         var $abc123 = require('abc123');
         console.log($abc123);`);
-    const result = await testRequireInliningVisitor(src, {
-      abc123: false,
-    });
+    const result = await testRequireInliningVisitor(src, []);
     assertEqualCode(
       result,
       getModule(`var $abc123;
@@ -67,9 +62,7 @@ describe('InliningVisitor', () => {
         var $abc123Default;
         console.log((0, parcelHelpers.interopDefault(require('abc123'))).foo());`,
     );
-    const result = await testRequireInliningVisitor(src, {
-      abc123: false,
-    });
+    const result = await testRequireInliningVisitor(src, []);
     assertEqualCode(result, expected);
   });
 
@@ -82,10 +75,7 @@ describe('InliningVisitor', () => {
         var $abc456;
         console.log($abc123);
         console.log((0, require('abc456')));`);
-    const result = await testRequireInliningVisitor(src, {
-      abc123: true,
-      abc456: false,
-    });
+    const result = await testRequireInliningVisitor(src, ['abc123']);
     assertEqualCode(result, expected);
   });
 });