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

Enhance serialization speed to save 3s on diff writing. #88

Closed
wants to merge 2 commits into from
Closed

Conversation

ProTip
Copy link

@ProTip ProTip commented Jul 1, 2018

This may resolve #87. Although I'm not sure I addressed the specific regression this reduces the time spent when writing a diff image for me by slightly more than 3 seconds.

I now get ~2 seconds spent in the expect operation when a difference is detected and can get a further 600ms by reducing the threshold used with pixelmatch to 0.1 .

I suspect pixel match may also be suffering from the vm globals issue, however I have not checked verified this.

I have verified that placing a variant of const Math = Math at the top of every pngjs source file that uses math with foil the vm issue.. It may be worth asking politely if these changes can be introduced into pixelmatch and pngjs.

@CLAassistant
Copy link

CLAassistant commented Jul 1, 2018

CLA assistant check
All committers have signed the CLA.

@ProTip
Copy link
Author

ProTip commented Jul 1, 2018

I actually would like to refactor this to create the whole payload as a string literal when I get a chance.

@anescobar1991
Copy link
Member

I'll work on figuring out what caused the specific regression between 2.4.0 and 2.4.3 tomorrow and come up with a fix and will put in a temporary fix to avoid running into the VM issue for the diffing. In the meantime can you open up issues with pixelmatch and pngjs to see if they can resolve the issue on their end?

I will also see about adding some performance tests to the builds and a package-lock.json so that figuring out what caused the regression is not so difficult next time.

@ProTip Can you make sure you sign the CLA so that we can merge this? I think the build failure on node 10 is unrelated to your changes I just need to figure out how to re run the build. @tklever do you have permissions to re-run the build on travis for this PR?

@ProTip
Copy link
Author

ProTip commented Jul 1, 2018

I can do that.

How precious is pngjs to the project? Its sync.write(pack internally) appears intrinsically slow from my testing and I'm unable to descern why ATM. I tested using the library sharp for writing raw data out to png in the external process and was able to get sub-second total expect time on a diff. I suspect that nixing the external process and simply using sharp in-proc may be a great way to go.

The use case I have in mind, other than general speed up, is making a css or other far reaching change and wanting to grab diffs of an entire site to inspect the impact..

@anescobar1991
Copy link
Member

Hmm. I will play with that too. I assume they are not using Math or any other node globals then? I have no attachment to pngjs or any other library, the only difficulty was finding a library that was able to work synchronously as Jest required for custom marchers.

Nowadays Jest supports async custom marchers but it would be a breaking change to switch to them as the API for the matcher would change. Maybe that is the way to go now that it is supported?

@ProTip
Copy link
Author

ProTip commented Jul 2, 2018

I believe sharp would bypass most of the issues with pngjs due to utilizing compiled C++ code. I did discover a large source of the slowdown with sync.write though; the default filterType -1 utilizes a very expensive filter selection strategy. Hard setting this to Paeth via PNG.sync.write(image, {filterType: 4}) reduces the conversion time from raw bytes to PNG from ~260ms to ~65ms on my system.

The new async matchers look clutch: jestjs/jest@334161f . I wouldn't know where to start as far as compatibility. I personally run everything through TypeScript, but I suppose most recent node versions support async/await OOTB? Perhaps a new async/sharp based matcher could be added alongside the existing matcher for backwards compatibility?

@ProTip ProTip mentioned this pull request Jul 2, 2018
@anescobar1991
Copy link
Member

Declining in favor of #89

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.

Diff performance regression
3 participants