schoero / swissqrbill

Swiss QR Bill generation in Node.js and browsers
MIT License
155 stars 29 forks source link

Performance improvement for QR rendering #384

Closed danielpanero closed 1 year ago

danielpanero commented 1 year ago

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
danielpanero commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.