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

Performance improvement for QR rendering #384

Merged
merged 3 commits into from
Aug 31, 2022

Conversation

danielpanero
Copy link
Contributor

It takes a lot of time to render a lot of QR Bills since addPath is an expensive function as we need to convert from QR segments to SVG and then to PDF.

We can bypass the conversion from QR to SVG, and directly convert QR segments to PDF.

This leads to huge performance benefits (a4.js):

Number of bills 10 100 1000 10000
Normal 754.651ms 5.426s 50.278s Node crashed :(
Modified 278.702ms 969.642ms 6.985s 1 min 7.421s

Copy link
Owner

@schoero schoero 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 for your pull request. I made a quick review and found some things that should be improved before merging.

Apart from that, I could also reproduce the performance gains. Good job 👍

src/pdf/pdf.ts Show resolved Hide resolved
src/shared/qr-code.ts Outdated Show resolved Hide resolved
src/pdf/pdf.ts Outdated Show resolved Hide resolved
@danielpanero
Copy link
Contributor Author

Furthermore, the second most expensive function is the getPenaltyScore() found in qr-code-generator.ts, that checks which of the 8 mask patterns is the most effective one...

const qrCode = qrcodegen.QrCode.encodeSegments([eci, ...segments], qrcodegen.QrCode.Ecc.MEDIUM, 1, 40, 0);

If we impose mask 0, this leads to again to huge performance gains:

Number of bills 10 100 1000 10000
Normal 754.651ms 5.426s 50.278s Node crashed :(
Modified 278.702ms 969.642ms 6.985s 1 min 7.421s
Modified V2 157.408ms 586.346ms 3.679s 34.487s

@schoero
Copy link
Owner

schoero commented Aug 29, 2022

Thank you for further researching and testing performance bottlenecks. It sounds good, but I'm not sure if it's the best idea to hardcode the masking pattern. After all, the readability of the QR code will suffer in most cases. Do you have a more in-depth knowledge about QR codes and do you know if this is safe to do?

I don't really want to trade readability for performance.

@danielpanero
Copy link
Contributor Author

Yes, that's the problem... for 10/100 bills the performance gains are not justifiable as I think it's better to lose some time improving the readability of the code. After 1000 I don't know...

@schoero
Copy link
Owner

schoero commented Aug 31, 2022

I checked the specifications again. There is no mention about the masking patterns. But there are rules to minimum module size and error correction etc, so I would like to play it safe and stick to the expensive calculation for now.

Im going to merge, feel free to open an issue if you want to further discuss this topic.

@schoero schoero merged commit a49e7c1 into schoero:master Aug 31, 2022
@danielpanero danielpanero deleted the performance-qr branch September 1, 2022 06:42
@schoero schoero mentioned this pull request Sep 26, 2023
29 tasks
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

2 participants