Reuse JSDOM instances across targets

lencioni · lencioni · commit bd1628172878 · 2020-06-18T11:43:27.000-05:00
I've been investigating a performance problem we ran into at Airbnb
the last couple of days. We have some components that end up with
very very large dependency graphs, which makes the webpack bundle
quite large. Looking at the CI output, it seems to take about 2 min
to bundle, which isn't the most terrible thing. But, then I've
noticed that the timings for the targets are pretty crazy. Look at
these logs:

```
[2020-06-17T19:36:15Z] image_analytics was not bootstrapped
[2020-06-17T19:36:37Z]   - chrome-medium ✓ (22034.4ms)
[2020-06-17T19:38:38Z] No examples found for target chrome-mediumPlus, skipping
[2020-06-17T19:38:38Z]   - chrome-mediumPlus ✓ (4.0ms)
[2020-06-17T19:41:05Z] p3_defer_modals was not bootstrapped
```

Here chrome-medium had examples, so it took snapshots and that took
a while which makes sense. mediumPlus didn't have examples, so it
took no snapshots. Happo says that mediumPlus took 4ms, but if you
look at the timestamps in the log lines, you can see that it
actually took 2 minutes.

I believe that the time spent on empty targets is in initializing
the JSDOM and loading up the webpack bundle. Since we have 6
targets, this adds up to 12 minutes of overhead per job just for
loading the bundle, and another 12 if we have to check out previous
SHA. This is much too long.

I think we can improve performance by reusing the JSDOM instance for
all of the targets. In the example above, this should save us about
10 minutes on a run where the previous SHA report exists, and 20
minutes on a run where the previous SHA report does not exist.

One potential issue that could arise from this is if there is any
side-effecty code in the bundle that looks at the viewport size when
it is first executed, that will no longer work quite right since we
run the bundle and then resize the window later.

In order to preserve backwards compatibility with other DomProvider
plugins, like the puppeteer plugin, I left in the width/height in
the initialization call, and included a guard around the resize call
in case it does not exist yet. We should clean these up in the next
breaking change.
diff --git a/src/JSDOMDomProvider.js b/src/JSDOMDomProvider.js
@@ -1,11 +1,20 @@
 import { JSDOM, VirtualConsole } from 'jsdom';
 
+import Logger from './Logger';
+
 const { VERBOSE } = process.env;
 
 const MAX_ERROR_DETAIL_LENGTH = 200;
 
-export default class JSDOMDomProvider {
-  constructor(jsdomOptions, { width, height, webpackBundle }) {
+// Cache the JSDOM instance because re-loading the webpack bundle for every
+// target can be very expensive. This assumes that jsdomOptions and
+// webpackBundle do not change.
+let dom;
+function getCachedDOM(jsdomOptions, webpackBundle) {
+  if (!dom) {
+    const logger = new Logger();
+    logger.start('Initializing JSDOM with the bundle...');
+
     const virtualConsole = new VirtualConsole();
     virtualConsole.on('jsdomError', (e) => {
       const { stack, detail = '' } = e;
@@ -19,7 +28,7 @@ export default class JSDOMDomProvider {
     });
     virtualConsole.sendTo(console, { omitJSDOMErrors: true });
 
-    this.dom = new JSDOM(
+    dom = new JSDOM(
       `
         <!DOCTYPE html>
         <html>
@@ -37,21 +46,28 @@ export default class JSDOMDomProvider {
           url: 'http://localhost',
           virtualConsole,
           beforeParse(win) {
-            win.outerWidth = win.innerWidth = width;
-            win.outerHeight = win.innerHeight = height;
-            Object.defineProperties(win.screen, {
-              width: { value: width },
-              availWidth: { value: width },
-              height: { value: height },
-              availHeight: { value: height },
-            });
             win.requestAnimationFrame = (callback) => setTimeout(callback, 0);
             win.cancelAnimationFrame = clearTimeout;
           },
         },
         jsdomOptions,
       ),
     );
+
+    logger.success();
+  }
+
+  return dom;
+}
+
+// Useful for tests
+export function clearCachedDOM() {
+  dom = undefined;
+}
+
+export default class JSDOMDomProvider {
+  constructor(jsdomOptions, { webpackBundle }) {
+    this.dom = getCachedDOM(jsdomOptions, webpackBundle);
   }
 
   async init({ targetName }) {
@@ -61,6 +77,17 @@ export default class JSDOMDomProvider {
     return this.dom.window.happoProcessor.init({ targetName });
   }
 
+  resize({ width, height }) {
+    this.dom.window.outerWidth = this.dom.window.innerWidth = width;
+    this.dom.window.outerHeight = this.dom.window.innerHeight = height;
+    Object.defineProperties(this.dom.window.screen, {
+      width: { value: width },
+      availWidth: { value: width },
+      height: { value: height },
+      availHeight: { value: height },
+    });
+  }
+
   next() {
     return this.dom.window.happoProcessor.next();
   }
diff --git a/src/processSnapsInBundle.js b/src/processSnapsInBundle.js
@@ -5,16 +5,21 @@ export default async function processSnapsInBundle(
   { viewport, DomProvider, targetName },
 ) {
   const [width, height] = viewport.split('x').map((s) => parseInt(s, 10));
-  const domProvider = new DomProvider({
-    webpackBundle,
-    width,
-    height,
-  });
+
+  // TODO Remove width and height in next breaking change after puppeteer plugin
+  // has been updated.
+  const domProvider = new DomProvider({ webpackBundle, width, height });
 
   const result = {
     snapPayloads: [],
   };
   try {
+    // TODO remove resize guard in next breaking change and after puppeteer
+    // plugin has been updated.
+    if (typeof domProvider.resize !== 'undefined') {
+      domProvider.resize({ width, height });
+    }
+
     await domProvider.init({ targetName });
 
     // Disabling eslint here because we actually want to run things serially.
diff --git a/test/integrations/error-test.js b/test/integrations/error-test.js
@@ -4,6 +4,7 @@ import MockTarget from './MockTarget';
 import * as defaultConfig from '../../src/DEFAULTS';
 import makeRequest from '../../src/makeRequest';
 import runCommand from '../../src/commands/run';
+import { clearCachedDOM } from '../../src/JSDOMDomProvider';
 
 jest.mock('../../src/makeRequest');
 
@@ -15,6 +16,7 @@ let config;
 let sha;
 
 beforeEach(() => {
+  clearCachedDOM();
   console.warn = jest.fn(originalConsoleWarn);
   console.error = jest.fn(originalConsoleErr);
   console.error.mockReset();
diff --git a/test/integrations/react-test.js b/test/integrations/react-test.js
@@ -8,6 +8,7 @@ import MockTarget from './MockTarget';
 import * as defaultConfig from '../../src/DEFAULTS';
 import makeRequest from '../../src/makeRequest';
 import runCommand from '../../src/commands/run';
+import { clearCachedDOM } from '../../src/JSDOMDomProvider';
 
 jest.mock('../../src/makeRequest');
 
@@ -16,6 +17,8 @@ let config;
 let sha;
 
 beforeEach(() => {
+  clearCachedDOM();
+
   makeRequest.mockImplementation(() => Promise.resolve({}));
   sha = 'foobar';
   config = Object.assign({}, defaultConfig, {
