perf: improve performance of experimentalSourceRewriting

This change is in addition to benjamn/recast#1399. This commit
focuses on Cypress' use of recast, with the following optimizations:

1. Avoid printing the source file again if no change was made.
   Printing an AST using recast does reuse the original text, but identifying
   for which parts of the AST reuse is possible comes with noticeable overhead.
   By tracking changes within the visitor it becomes possible to skip this
   phase entirely if no changes were made.
2. Avoid a scope lookup (`path.scope.declares(node.name)`) for all identifiers in
   the program, doing it only for identifiers that may have to be rewritten.

With these changes, a 2.6MB bundle that does not need rewriting has improved
from 4.4s to 2.3s, provided that the above-mentioned recast PR has been applied.

cli/CHANGELOG.md

@@ -3,7 +3,11 @@
 
 _Released 5/21/2024 (PENDING)_
 
-**Bugfixes:**
+**Performance:**
+
+- Improved performance of `experimentalSourceRewriting` option. Fixed in [#29540](https://github.com/cypress-io/cypress/pull/29540).
+
+- **Bugfixes:**
 
 - Fixed an issue where orphaned Electron processes were inadvertently terminating the browser's CRI client. Fixes [#28397](https://github.com/cypress-io/cypress/issues/28397). Fixed in [#29515](https://github.com/cypress-io/cypress/pull/29515).
 - Fixed an issue where Cypress would use the wrong URL to upload Test Replay recordings when it wasn't able to determine the upload URL. It now displays an error when the upload URL cannot be determined, rather than a "Request Entity Too Large" error. Addressed in [#29512](https://github.com/cypress-io/cypress/pull/29512).

packages/rewriter/lib/js-rules.ts

@@ -62,6 +62,8 @@ function resolveLocationReference () {
   )
 }
 
+const replaceableProps = ['parent', 'top', 'location']
+
 /**
  * Given an Identifier or a Literal, return a property name that should use `resolveWindowReference`.
  * @param node
@@ -70,12 +72,12 @@ function getReplaceablePropOfMemberExpression (node: n.MemberExpression) {
   const { property } = node
 
   // something.(top|parent)
-  if (n.Identifier.check(property) && ['parent', 'top', 'location'].includes(property.name)) {
+  if (n.Identifier.check(property) && replaceableProps.includes(property.name)) {
     return property.name
   }
 
   // something['(top|parent)']
-  if (n.Literal.check(property) && ['parent', 'top', 'location'].includes(String(property.value))) {
+  if (n.Literal.check(property) && replaceableProps.includes(String(property.value))) {
     return String(property.value)
   }
 
@@ -104,6 +106,7 @@ export const jsRules: Visitor<{}> = {
     }
 
     path.replace(resolveWindowReference(path.get('object').node, prop))
+    this.reportChanged()
 
     return false
   },
@@ -120,6 +123,7 @@ export const jsRules: Visitor<{}> = {
       if (isAssignee && node.name === 'location') {
         // `location = 'something'`, rewrite to intercepted href setter since relative urls can break this
         path.replace(b.memberExpression(resolveLocationReference(), b.identifier('href')))
+        this.reportChanged()
 
         return false
       }
@@ -147,40 +151,38 @@ export const jsRules: Visitor<{}> = {
       }
     }
 
-    if (path.scope.declares(node.name)) {
-      // identifier has been declared in local scope, don't care about replacing
+    // identifier has been declared in local scope, don't care about replacing
+    if (!replaceableProps.includes(node.name) || path.scope.declares(node.name)) {
       return this.traverse(path)
     }
 
-    if (node.name === 'location') {
-      path.replace(resolveLocationReference())
-
-      return false
-    }
+    switch (node.name) {
+      case 'location':
+        path.replace(resolveLocationReference())
+        this.reportChanged()
 
-    if (['parent', 'top'].includes(node.name)) {
-      path.replace(resolveWindowReference(globalIdentifier, node.name))
+        return false
+      case 'parent':
+      case 'top':
+        path.replace(resolveWindowReference(globalIdentifier, node.name))
+        this.reportChanged()
 
-      return false
+        return false
+      default:
+        return this.traverse(path)
     }
-
-    this.traverse(path)
   },
   visitAssignmentExpression (path) {
     const { node } = path
 
-    const finish = () => {
-      this.traverse(path)
-    }
-
     if (!n.MemberExpression.check(node.left)) {
-      return finish()
+      return this.traverse(path)
     }
 
     const propBeingSet = getReplaceablePropOfMemberExpression(node.left)
 
     if (!propBeingSet) {
-      return finish()
+      return this.traverse(path)
     }
 
     if (node.operator !== '=') {
@@ -194,6 +196,7 @@ export const jsRules: Visitor<{}> = {
     const objBeingSetOn = node.left.object
 
     path.replace(resolveWindowReference(objBeingSetOn, propBeingSet, node.right))
+    this.reportChanged()
 
     return false
   },

packages/rewriter/lib/js.ts

@@ -32,7 +32,13 @@ export function rewriteJsSourceMap (url: string, js: string, inputSourceMap: any
 
     const ast = recast.parse(js, { sourceFileName })
 
-    astTypes.visit(ast, jsRules)
+    const visitor = astTypes.PathVisitor.fromMethodsObject(jsRules)
+
+    visitor.visit(ast)
+
+    if (!visitor.wasChangeReported() && inputSourceMap) {
+      return inputSourceMap
+    }
 
     return recast.print(ast, {
       inputSourceMap,
@@ -50,18 +56,32 @@ export function rewriteJsSourceMap (url: string, js: string, inputSourceMap: any
 export function _rewriteJsUnsafe (url: string, js: string, deferSourceMapRewrite?: DeferSourceMapRewriteFn): string {
   const ast = recast.parse(js)
 
+  let didRewrite: boolean
+
   try {
-    astTypes.visit(ast, jsRules)
+    const visitor = astTypes.PathVisitor.fromMethodsObject(jsRules)
+
+    visitor.visit(ast)
+
+    didRewrite = visitor.wasChangeReported()
   } catch (err: any) {
     // if visiting fails, it points to a bug in our rewriting logic, so raise the error to the driver
     return _generateDriverError(url, err)
   }
 
-  const { code } = recast.print(ast, defaultPrintOpts)
+  let rewritten: string
+
+  if (didRewrite) {
+    const { code } = recast.print(ast, defaultPrintOpts)
+
+    rewritten = code
+  } else {
+    rewritten = js
+  }
 
   if (!deferSourceMapRewrite) {
     // no sourcemaps
-    return sourceMaps.stripMappingUrl(code)
+    return sourceMaps.stripMappingUrl(rewritten)
   }
 
   // get an ID that can be used to lazy-generate the source map later
@@ -71,7 +91,7 @@ export function _rewriteJsUnsafe (url: string, js: string, deferSourceMapRewrite
     // using a relative URL ensures that required cookies + other headers are sent along
     // and can be reused if the user's sourcemap requires an HTTP request to be made
     `/__cypress/source-maps/${sourceMapId}.map`,
-    code,
+    rewritten,
   )
 }
 

packages/rewriter/test/unit/js-spec.ts

@@ -164,7 +164,7 @@ describe('js rewriter', function () {
 
         err.stack = 'stack'
 
-        sinon.stub(astTypes, 'visit').throws(err)
+        sinon.stub(astTypes.PathVisitor, 'fromMethodsObject').throws(err)
 
         const actual = _rewriteJsUnsafe(URL, 'console.log()')