Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reuse JSDOM instances across targets #164

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

lencioni
Copy link
Contributor

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.

@trotzig
Copy link
Contributor

trotzig commented Jun 18, 2020

This looks good! At first I had envisioned this to be handled outside of the JSDOMDomProvider (this would improve things for users of happo-plugin-puppeteer as well), but I think the local cache is good as well.

I'm going to play around with this locally to see if I can spot anything odd.

@@ -61,6 +77,17 @@ export default class JSDOMDomProvider {
return this.dom.window.happoProcessor.init({ targetName });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think inside the happoProcessor (which is src/browser/processor.js) we need to change some things so that the cursor is reset (and possibly other things as well).

@trotzig
Copy link
Contributor

trotzig commented Jun 18, 2020

Looking at the test failures I see this:

FAIL  test/integrations/react-test.js (19.096s)
  ● with multiple targets › produces the right number of snaps in each target
    TypeError: Cannot redefine property: width
        at Function.defineProperties (<anonymous>)
      81 |     this.dom.window.outerWidth = this.dom.window.innerWidth = width;
      82 |     this.dom.window.outerHeight = this.dom.window.innerHeight = height;
    > 83 |     Object.defineProperties(this.dom.window.screen, {
      84 |       width: { value: width },
      85 |       availWidth: { value: width },
      86 |       height: { value: height },
      
      at JSDOMDomProvider.resize (src/JSDOMDomProvider.js:83:12)
      at processSnapsInBundle (src/processSnapsInBundle.js:20:19)
      at generateScreenshots (src/domRunner.js:164:45)

I've run into this issue before, and I don't know of a workaround (yet).

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.
@trotzig
Copy link
Contributor

trotzig commented Jun 18, 2020

Btw, if you want to iterate quickly on this locally, you can run yarn test --watch react-test -t multiple (remember to run yarn build --watch as well in a different terminal).

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.
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.
@lencioni
Copy link
Contributor Author

After trying this out on our codebase, I'm nervous about merging it. We noticed a number of diffs where state from a different target had spilled over into other targets. For the most part this was okay, but I think it will make Happo more confusing to work with and may make rendering some things not possible. This was mostly problematic for code that read the viewport size and expected it to not change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants