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

Replace zlib.js and pako with fflate #3054

Merged
merged 8 commits into from
Jan 14, 2021
Merged

Conversation

101arrowz
Copy link
Contributor

This PR replaces both zlib.js and pako with fflate, a faster and smaller compression library. The end result is a smaller bundle that produces PDFs more quickly. The UMD build has decreased in size from 409784 bytes to 363863 bytes with this change. The affected reference PDFs increase in size by 0.4%-2%, however the compression completes in 55% of the time compared to before on average, which significantly speeds up PDF generation. If the library wishes to make slightly smaller PDFs at the same speed as before, the default compression level can be increased.

Please let me know if you'd like me to clarify anything about fflate or the changes I made to the codebase. Thank you all so much maintaining an awesome open source project for years!

@101arrowz
Copy link
Contributor Author

The cause of the failing tests is the change from unstable QuickSort in Node 10 to stable TimSort in Node 11, which makes the compressed data change. I don't think that the tests should check for exact equality with reference PDFs for this reason, but I can make it work on Node 10 instead.

101arrowz and others added 2 commits January 1, 2021 13:22
- fix polyfills.umd deployment (core-js imports were not resolved)
- fix rollup preprocessor in karma configs
- update image compression reference files
- don't run image compressions tests in node (fail in node <= 10 due to instable sort)
Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR :) The fflate library sounds promising.

I think we can live with slightly larger PDFs if it's so much faster.

A couple of minor things:

  • Please revert the changes to the files in the dist folder
  • I updated the image compression reference files and skipped them on node. I agree that comparing compressed PDFs byte-wise is not optimal, but I think we have to live with that for now.
  • I also fixed the build
  • See the inline comments ;)

Apart from that I think the PR is fine and I will merge it as soon as the minor issues are resolved. Thanks again :)

src/libs/png.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
src/modules/filters.js Show resolved Hide resolved
test/deployment/esm/karma.conf.js Outdated Show resolved Hide resolved
@101arrowz
Copy link
Contributor Author

101arrowz commented Jan 12, 2021

I suggest running benchmarks manually to verify my claims for yourself, but from what I've tested, images take around 2/3 of the time for PDF generation. While on most data, fflate is faster by nearly a factor of 2, here it's closer to 33% faster (100ms instead of 150ms). Regardless, a 20% improvement in PDF generation for single-image PDFs is worthwhile IMO, especially since the effect multiplies for larger PDFs.

@HackbrettXXX
Copy link
Collaborator

I suggest running benchmarks manually to verify my claims for yourself, but from what I've tested, images take around 2/3 of the time for PDF generation. While on most data, fflate is faster by nearly a factor of 2, here it's closer to 33% faster (100ms instead of 150ms). Regardless, a 20% improvement in PDF generation for single-image PDFs is worthwhile IMO, especially since the effect multiplies for larger PDFs.

What we could do is introduce a compression level option to the jsPDF constructor similar to the addImage method. This way, the user can choose between slow and fast compression themselves. What I have in mind is to extend the type of the compress option:

export interface jsPDFOptions {
  compress?: boolean | Compression; // false is equal to "NONE" and true would be the default level
  // ...
}

export type Compression = "NONE" | "FAST" | "MEDIUM" | "SLOW";
// change ImageCompression type to:
export type ImageCompression = Compression

@101arrowz
Copy link
Contributor Author

What we could do is introduce a compression level option to the jsPDF constructor similar to the addImage method. This way, the user can choose between slow and fast compression themselves. What I have in mind is to extend the type of the compress option:

Should I add that to this PR?

@HackbrettXXX
Copy link
Collaborator

Should I add that to this PR?

I think a separate PR would be better :) I'll merge this one now. Thanks for this PR!

@HackbrettXXX HackbrettXXX merged commit 75e6ed7 into parallax:master Jan 14, 2021
@jekkos
Copy link

jekkos commented Jan 20, 2021

@here it seems the bower.json using fflate does not build currently. Their repository does not have any version tags that bower can use at this time. I'm getting following error

bower not-cached    https://github.com/101arrowz/fflate.git#^0.4.8

bower resolve       https://github.com/101arrowz/fflate.git#^0.4.8

bower resolved      https://github.com/thomaspark/bootswatch.git#3.4.1+1

bower ENORESTARGET  No tag found that was able to satisfy ^0.4.8

@101arrowz
Copy link
Contributor Author

101arrowz commented Jan 20, 2021

I was wondering how that worked. I'll add version tags to the repository ASAP, thanks for letting me know.

@jekkos I've added a release with the source code for that version. Could you verify the install succeeds now?

@jekkos
Copy link

jekkos commented Jan 20, 2021

Ok great it works again.. thanks for the swift reply!!

This was referenced Mar 12, 2021
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.

None yet

3 participants