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

How long should the image comparison typically take? #28

Closed
theblang opened this issue Nov 27, 2017 · 16 comments
Closed

How long should the image comparison typically take? #28

theblang opened this issue Nov 27, 2017 · 16 comments

Comments

@theblang
Copy link

I have a really basic Jest test that navigates to a screen in our app using puppeteer, takes a screenshot, then uses toMatchImageSnapshot. Unfortunately, the test takes about thirty seconds after adding the toMatchImageSnapshot matcher. I have another script where I use puppeteer to take two screenshots to compare them with Resemble outside of a Jest environment and it only takes about four seconds. Any idea as to what might be causing this drastic increase in run time?

@anescobar1991
Copy link
Member

I have not seen them take that long. For an 800x600 image typically the comparison takes about 3-4 seconds max when there is a large difference between the 2 images.

What version are you using? There are significant performance improvements between versions 1.x.x and 2.x.x.

And I have noticed as well (but haven't had time to look into) that simple image assertions using both pixelmatch and blink-diff within jest (even without the toMatchImageSnapshot() matcher take much much longer than outside of the Jest environment. Can you try using Resemble within the Jest environment to see if performance degrades at all?

@theblang
Copy link
Author

@anescobar1991 Great suggestion, I have actually already done a test with Resemble inside of Jest. It absolutely does degrade as well, taking about ten seconds instead of the 3-4 seconds that it takes outside of Jest. The image being used for comparison is 1100 x 2400. The Jest version is 21.2.1, and jest-image-snapshot is 2.2.0.

@anescobar1991
Copy link
Member

Then I believe an issue should be opened against Jest to figure out where the performance degradation may be coming from on their end. Closing this issue as this is an upstream Jest issue.

@theblang
Copy link
Author

theblang commented Dec 5, 2017

@anescobar1991 See the referenced ticket that I made on Jest's repo. I put together a simple test here that shows writing a file in Jest is painfully slow, which I suspect is causing the jest-image-snapshot matcher to be painfully slow.

Edit: More specifically, writing a PNG file seems to be extremely slow. See the pngjs calls in this CPU profile.

@theblang
Copy link
Author

theblang commented Dec 7, 2017

@anescobar1991 If you get a chance could you please run the matcher.spec.js spec in this repo that I set up to reproduce the issue and tell me if you experience it as well? I've pinpointed the slowdown to pngjs writing the diff file to disk. I've tested across two platforms now, both Windows and macOS, and both times I go from four seconds to thirty seconds as soon as I uncomment the line that writes the diff PNG to disk. This seems catastrophically bad.

@anescobar1991
Copy link
Member

Yes I will play with it tomorrow. I do know there is definite slowness with both pixelmatch and blink-diff that only occurs for me when running within jest but I will also test out your repo and see if pngjs is the culprit for what I've noticed in the past.

I don't have access to a Windows machine but I can confirm with your repo on macOS.

@anescobar1991
Copy link
Member

anescobar1991 commented Dec 8, 2017

Alright so I was able to confirm that the slowness only occurs within Jest and that writing to a file is what is hanging when running in Jest as you asserted in jestjs/jest#4972 (comment)

In fact I modified jest-image-snapshot to not write diffs to a file and the performance issues were 100% gone. I will play around more and make changes to your jest-image-comparison-perf-repro so others can see.

Correction: writing to a file is not what is slower in Jest, creating the PNG buffer that is fed into the file is. See theblang/jest-image-comparison-perf-repro#2

@anescobar1991
Copy link
Member

anescobar1991 commented Jan 11, 2018

@mattblang thanks to the repro and the time you spent on following up with this issue I am working on a PR that resolves the performance issues! Should have it open in the next few days.

@theblang
Copy link
Author

@anescobar1991 Hey! I was curious if you ever made any progress on a potential solution after the feedback in jestjs/jest#5163?

@anescobar1991
Copy link
Member

Hey! So after that feedback we tried a few different ways to resolve the issue from our side (or from the jest side) but couldn’t figure anything that was reasonable out...

Instead what I resorted to was performing the diff image creation in a different process (one that does not run within the jest sandbox) which did help performance trememdously. This was done in #40

Are you still seeing perf issues after that fix?

bfirsh added a commit to arxiv-vanity/engrafo that referenced this issue Jun 14, 2018
The tests were running incredinly slow, due to
americanexpress/jest-image-snapshot#28 .

This also gives the advantage of not producing a new huge image
in our Git repo every time there is a minor change.
@bfirsh
Copy link

bfirsh commented Jun 14, 2018

I am seeing very slow performance on the image comparison, not just producing the diff.

An 800x3125 snapshot runs very fast with jest -u, but when comparing, it takes more than 10 seconds. An 800x12500 snapshot similarly updates very fast, but is so slow comparing that it just locks up the Node process. (Perhaps there is something quadratic going on with respect to number of pixels?)

I haven't got time to put together a reproducible test case right now, but in the meantime here is a profile: https://gist.github.com/bfirsh/8f6dc59ac87d5d60f547519e27633b33

I'm running Debian inside a Docker container with headless Puppeteer.

bfirsh added a commit to arxiv-vanity/engrafo that referenced this issue Jun 14, 2018
The tests were running incredinly slow, due to
americanexpress/jest-image-snapshot#28 .

This also gives the advantage of not producing a new huge image
in our Git repo every time there is a minor change.
@anescobar1991
Copy link
Member

@bfirsh I'd be interested in seeing a comparison with running a diff using jest-image-snapshot and with just using pixelmatch by itself outside of jest and see if the performance issue is introduced by us or is there with pixelmatch.

Do you think you could do so and report back what you see with your large images?

@bfirsh
Copy link

bfirsh commented Jun 15, 2018

Sure! I haven't got time right now, but if I have some spare time I'll give it a go. :)

@theblang
Copy link
Author

theblang commented Jul 9, 2018

@anescobar1991 My apologies, I didn't back to you when you asked

Are you still seeing perf issues after that fix?

I recently got back to the task of migrating our current screenshot specs to Puppeteer. I'm noticing that screenshot comparison with the matcher still takes around 15 seconds on an 1100 x 2400 image. I'm curious if there is something I need to do on my end to take advantage of the performance tweak you made involving the separate process that runs the diffing.

@anescobar1991
Copy link
Member

Yeah what I fixed was moving the image diff creation into its own child process outside of Jest. After that there is still some slowdown caused by Jest and to avoid that #89 finishes the job.

Can you clone that branch and validate speed with it?

@theblang
Copy link
Author

theblang commented Jul 9, 2018

@anescobar1991 Oh wow! So here is the before and after, master vs #89

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   1 failed, 1 total
Time:        15.593s, estimated 16s

vs

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   1 failed, 1 total
Time:        2.275s, estimated 16s

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

No branches or pull requests

3 participants