From 48cf902c7fae31228c55e6e1f0dfb4de6238a52e Mon Sep 17 00:00:00 2001 From: Joe Lencioni Date: Thu, 18 Jun 2020 11:21:19 -0500 Subject: [PATCH 1/4] Reuse JSDOM instances across targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/JSDOMDomProvider.js | 49 +++++++++++++++++++++++++-------- src/processSnapsInBundle.js | 15 ++++++---- test/integrations/error-test.js | 2 ++ test/integrations/react-test.js | 3 ++ 4 files changed, 53 insertions(+), 16 deletions(-) diff --git a/src/JSDOMDomProvider.js b/src/JSDOMDomProvider.js index d56bf4e..2a0d00a 100644 --- 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( ` @@ -37,14 +46,6 @@ 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; }, @@ -52,6 +53,21 @@ export default class JSDOMDomProvider { 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, configurable: true }, + availWidth: { value: width, configurable: true }, + height: { value: height, configurable: true }, + availHeight: { value: height, configurable: true }, + }); + } + next() { return this.dom.window.happoProcessor.next(); } diff --git a/src/processSnapsInBundle.js b/src/processSnapsInBundle.js index 8c3c9c4..2726d48 100644 --- 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 index 3994a2e..ed431de 100644 --- 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 index 7956594..230b911 100644 --- 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, { From 114c08f7add6ecf422707b3fc404761892f6d136 Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Thu, 18 Jun 2020 19:24:38 +0200 Subject: [PATCH 2/4] Resolve remaining issues with a reused jsdom A couple of issues are fixed in this commit: - we can only load the webpack bundle once, so a check for if the happoProcessor is initialized already will prevent infinitely waiting for the init promise. - we need to reset the happoProcessor in between runs, otherwise it won't start back at the top. - we can't close the jsdom window when we're done. Leaving this as a no-op for now, I think this is fine. - expecting the resize call to be async will help make it easier to update happo-plugin-puppeteer. - the init call needs to happen before the resize, now that we reset the happoProcessor as part of the resize. --- src/JSDOMDomProvider.js | 17 ++++++++++++----- src/browser/processor.js | 5 ++++- src/processSnapsInBundle.js | 6 +++--- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/src/JSDOMDomProvider.js b/src/JSDOMDomProvider.js index 2a0d00a..40051df 100644 --- a/src/JSDOMDomProvider.js +++ b/src/JSDOMDomProvider.js @@ -18,7 +18,11 @@ function getCachedDOM(jsdomOptions, webpackBundle) { const virtualConsole = new VirtualConsole(); virtualConsole.on('jsdomError', (e) => { const { stack, detail = '' } = e; - if (VERBOSE || typeof detail !== 'string' || detail.length < MAX_ERROR_DETAIL_LENGTH) { + if ( + VERBOSE || + typeof detail !== 'string' || + detail.length < MAX_ERROR_DETAIL_LENGTH + ) { console.error(stack, detail); } else { const newDetail = `${(detail || '').slice(0, MAX_ERROR_DETAIL_LENGTH)}... @@ -71,9 +75,11 @@ export default class JSDOMDomProvider { } async init({ targetName }) { - await new Promise((resolve) => { - this.dom.window.onBundleReady = resolve; - }); + if (!this.dom.window.happoProcessor) { + await new Promise((resolve) => { + this.dom.window.onBundleReady = resolve; + }); + } return this.dom.window.happoProcessor.init({ targetName }); } @@ -86,6 +92,7 @@ export default class JSDOMDomProvider { height: { value: height, configurable: true }, availHeight: { value: height, configurable: true }, }); + return this.dom.window.happoProcessor.reset(); } next() { @@ -101,6 +108,6 @@ export default class JSDOMDomProvider { } close() { - this.dom.window.close(); + // no-op } } diff --git a/src/browser/processor.js b/src/browser/processor.js index 71a5c31..34534b2 100644 --- a/src/browser/processor.js +++ b/src/browser/processor.js @@ -68,7 +68,6 @@ export default class Processor { // { fileName: '/bar/car.js', ... etc } // ] this.flattenedUnfilteredExamples = []; - this.cursor = -1; } init({ targetName } = {}) { @@ -81,6 +80,10 @@ export default class Processor { ); } + reset() { + this.cursor = -1; + } + addExamples(examples) { examples.forEach(({ fileName, component, variants }) => { Object.keys(variants).forEach((variant) => { diff --git a/src/processSnapsInBundle.js b/src/processSnapsInBundle.js index 2726d48..aab858b 100644 --- a/src/processSnapsInBundle.js +++ b/src/processSnapsInBundle.js @@ -14,14 +14,14 @@ export default async function processSnapsInBundle( snapPayloads: [], }; try { + await domProvider.init({ targetName }); + // 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.resize({ width, height }); } - await domProvider.init({ targetName }); - // Disabling eslint here because we actually want to run things serially. /* eslint-disable no-await-in-loop */ while (await domProvider.next()) { From 853e4fd52c0f0cfbce6fc1f48c71604fe30e9bdb Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Thu, 18 Jun 2020 20:13:58 +0200 Subject: [PATCH 3/4] Reset cursor in processor constructor I removed this in a previous commit, but forgot that happo-plugin-puppeteer isn't calling `happoProcessor.reset()`. Having it in the constructor as well ensures that the iteration works in the puppeteer case as well. --- src/browser/processor.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/browser/processor.js b/src/browser/processor.js index 34534b2..f04f53a 100644 --- a/src/browser/processor.js +++ b/src/browser/processor.js @@ -68,6 +68,7 @@ export default class Processor { // { fileName: '/bar/car.js', ... etc } // ] this.flattenedUnfilteredExamples = []; + this.cursor = -1; } init({ targetName } = {}) { From c3fa907bbe67f3ca4f9847715fd3c879e0237c4c Mon Sep 17 00:00:00 2001 From: Henric Trotzig Date: Thu, 18 Jun 2020 20:21:11 +0200 Subject: [PATCH 4/4] 5.7.0-rc.1 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index b98013b..3df6a13 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "happo.io", - "version": "5.6.1", + "version": "5.7.0-rc.1", "description": "Visual diffing for UI components", "main": "./build/index.js", "bin": {