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

Fix performance issue with setImmediate #619

Closed
wants to merge 2 commits into from

Conversation

maximerety
Copy link

@maximerety maximerety commented Sep 11, 2019

Hi there,

As explained in issue #617, there's a serious performance issue in generateAsync since v3.2.0.

After having run a comparison test (https://jsbin.com/kuxutodini/edit?html,console) and monitored the performance with Chrome's Javascript Profiler, I noticed repeated pauses during which the browser does nothing.

I found the culprit to be a change introduced in 5566c95 from MR #532.

The set-immediate-shim introduced is a very naive implementation using the setTimeout(..., 0) trick, whereas the core-js ponyfill used before is more subtle, e.g. using MessageChannel in browsers (as explained on MDN).

setImmediate is used extensively in the code base, and this change alone made the performance drop significantly in browsers.

This MR is a partial revert of 5566c95, but with a recent version of core-js-pure instead of the older core-js used before. With, core-js-pure we're guaranteed to use a ponyfill (not a polyfill polluting the global environment) as explained here.

The resulting file sizes in dist/ folder are a little bigger, but I think this is totally acceptable:

dist  (before)
├── [358K]  jszip.js
└── [ 94K]  jszip.min.js

dist  (after)
├── [376K]  jszip.js
└── [102K]  jszip.min.js

The naive implementation from the 'set-immediate-shim' package, based
on setTimeout(..., 0) is order of magnitude slower than the clever
ponyfill from core-js.

See Stuk#617
@maximerety maximerety mentioned this pull request Sep 11, 2019
@maximerety maximerety changed the title Fix performance issue with setImmediate - #617 Fix performance issue with setImmediate Sep 11, 2019
@maximerety
Copy link
Author

For some reason, SAUCE_USERNAME and SAUCE_ACCESS_KEY don't seem to be defined and the Travis build runs "test-local" which timeouts: https://travis-ci.org/Stuk/jszip/jobs/583580831

Maybe they are not defined for merge requests, but defined in builds on master? (e.g. with the last two commits on master: https://travis-ci.org/Stuk/jszip/builds/573829595 didn't pass, but https://travis-ci.org/Stuk/jszip/builds/577899994 passed).

Running test/index.html in a browser locally works just fine.

@TestPolygon
Copy link

What about it https://github.com/YuzuJS/setImmediate?

@maximerety
Copy link
Author

What about it https://github.com/YuzuJS/setImmediate?

This library works fine and can be used in a webapp as a workaround / temporary fix for people using current versions of jszip (3.2.x) and having performance issues.

I wouldn't recommend adding it as a dependency to the jszip repository because it's a polyfill which "pollutes" the global environment, i.e. it sets window.setImmediate. It could trigger side-effects or incompatibilities with other libraries.

IMHO, sticking to core-js-pure is a better choice as it is a ponyfill (explanations avaiable here), i.e. it does not modify window. No side-effect.

Moreover, core-js and core-js-pure are widely used (e.g. by Babel) so if other dependencies of projects using jszip also need a setImmediate implementation, there is a good chance that core-js is also used there and code duplication can be avoided.

@maximerety
Copy link
Author

Closing this as #829 has been merged and released.

As stated above, the solution chosen in #829 is:

[...] a polyfill which "pollutes" the global environment, i.e. it sets window.setImmediate. It could trigger side-effects or incompatibilities with other libraries.

Keep this in mind if it could be a problem for you.

@maximerety maximerety closed this May 23, 2022
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