codebude / QRCoder

A pure C# Open Source QR Code implementation
MIT License
4.58k stars 1.09k forks source link

Low performance. #319

Closed AndersMalmgren closed 2 years ago

AndersMalmgren commented 3 years ago

We render alot of invoices and we want to add a QRCode so that users can scan the invoice with their phone. Since we render several thousand invoices performance is important. I noticed it took 25ms to render the image when warm. I used the code from readme

QRCodeGenerator qrGenerator = new QRCodeGenerator();
QRCodeData qrCodeData = qrGenerator.CreateQrCode("The text which should be encoded.", QRCodeGenerator.ECCLevel.Q);
QRCode qrCode = new QRCode(qrCodeData);
Bitmap qrCodeImage = qrCode.GetGraphic(20);

Included in the 25 Ms is saving the bitmap to a memory stream and convert it to a byte array.

We are using full framework 4.7 and the computer i tested on is a AMD 5950x. Latest nuget version

codebude commented 3 years ago

In general it's hard to say what low or high performance is. Do you have any comparison? (Other libs that are faster? Or what is your code/second expectation?)

Two things which may improve performance:

  1. Instantiate the QRCodeGenerator class only once and re-use it when creating the individual codes. (Means: Define the QRCodeGenerator object out of the QR code creation loop logic)
  2. If you need the QR codes as bytearray, use either the BitmapByteQrCode or the PngByteQrCode rendering class. Those classes output byte arrays themselves, so that you can remove your extra step for byte arrays conversion. You can read more about the rendering classes in the wiki: https://github.com/codebude/QRCoder/wiki/Advanced-usage---QR-Code-renderers#24-bitmapbyteqrcode-renderer-in-detail
AndersMalmgren commented 3 years ago

I clocked only the QRCode part now. It takes 21ms. I lowered pixelres from 20 per module to 10. Down to 19-20ms. I think its slow. A modern computer game takes 5-8ms to render a single scene frame which is way more complex. It should be possible to get better performance. I think the problem is the old GDI+ framework. Its very slow

AndersMalmgren commented 3 years ago

It looks like the PngByteQRCode doesnt use GDI+ will try. I need it without quiet zone because I render it in a report with its own borders.

AndersMalmgren commented 3 years ago

I tried the byte version. It takes 13ms to render. So better. But it seems there is no way to disable quiet zone on it.

AndersMalmgren commented 3 years ago

I missed point 1) (Cache QRCodeGenerator) That shaved of some time. Down to 15ms warm on GDI+ Its still alot of time for a singel image render. But way better

codebude commented 3 years ago

Another idea is using ParallelForeach: https://www.c-sharpcorner.com/UploadFile/efa3cf/parallel-foreach-vs-foreach-loop-in-C-Sharp/ By using it, you could make better use of your multicore processor. If it works with your application at the end, depends on your use case. If you want to generate codes on request, than you can't multithread with the loop, because there's nothing to loop. But if you create multiple QR codes in batches/at the same time, using a ParallelForeach could increase the overall throughput. (And at least for your benchmark, it should show an increased performance.)

That said - feel free to fork QRCoder and send in your performance optimizations via Pull Request, if you see any points that should/could be optimized.

AndersMalmgren commented 3 years ago

We are already running the report generation task in parallel. It takes longer to generate the QR Code then it takes for the database to request and fetch the data for each invoice :D

codebude commented 2 years ago

Hi @AndersMalmgren ,

If I read your comments correctly, you had the best performance when using the PngByteQRCode, but couldn't use this renderer because you didn't want the whitespace (=quiet zone). Correct?

In the latest CI build (and soon officially when I push version 1.4.2 to NuGet) there is now a new/additional parameter "drawQuietZones" available for the PngByteQRCode. If you set this to "false", the codes will be rendered without quiet zones. With this you could take advantage of the performance benefits of the PngByteQR code.

FawzyMokhtar commented 2 years ago

I'm facing another performance issue but not a time complexity it's a Memory Leak as follow:

I'm using this code to convert a base64 string to a QR in the form of byte[] so I can display it in a Microsoft LocalReport.

/// <summary>
/// Converts the given invoice information into a (tag/type length value) TLV representation then convert it into a <see cref="Bitmap"/> QR code in the form of <see cref="byte"/>[].
/// </summary>
/// <param name="sellerName">The name of the seller.</param>
/// <param name="vatRegNum">The VAT registration number.</param>
/// <param name="timestamp">The timestamp in which the transaction was created.</param>
/// <param name="totalWithVAT">The transaction total value with VAT.</param>
/// <param name="vatTotal">The total VAT value of the transaction.</param>
/// <returns>A <see cref="byte"/>[] for the <see cref="Bitmap"/> QR code.</returns>
public static byte[] ToBitmapBytesQRCode(string sellerName, string vatRegNum, string timestamp, string totalWithVAT, string vatTotal)
{
    var tlv = ToTLV(sellerName, vatRegNum, timestamp, totalWithVAT, vatTotal);

    byte[] qrCodeAsBitmapByteArr;

    using (var qrGenerator = new QRCodeGenerator())
    {
        // 2mb added to memory and never got released here.
        using (var qrCodeData = qrGenerator.CreateQrCode(tlv, QRCodeGenerator.ECCLevel.Q)) 
        {
            using (var qrCode = new BitmapByteQRCode(qrCodeData))
            {
                // 19mb added to memory and never got released here.
                qrCodeAsBitmapByteArr = qrCode.GetGraphic(20);
            }
        }
    }

    return qrCodeAsBitmapByteArr;
}

Each call to this methods puts about 21mb to the memory and never got released even with the using that should dispose any resources related by the created QR instances.

Am I abusing something in the library or it is an issue with it?

AndersMalmgren commented 2 years ago

@FawzyMokhtar

You are sure its the QRCode? LocalReport will leak memory if you dont call

lr.ReleaseSandboxAppDomain();

I remember we had to work alot to get LocalReport to play nice,

FawzyMokhtar commented 2 years ago

Actually I managed to reduce the amount of memory used by forcing the application to run on x64 bit platform instead of 32 bit, things started to work correctly and some memory (not all of course) are released.

I went through the code - while debugging - line by line and monitoring the Resource Monitor, after that it is obvious it's caused by QRCode.

BTW, the LocalReport.render() in my case is getting called after the QR is successfully generated and not before that.

FawzyMokhtar commented 2 years ago

It's good to say that using the PngByteQRCode instead of the BitmapByteQRCode reduced the amount of memory used - in that step - from 19mb to actually NOTHING which is WooW.

I think the problem maybe with the BitmapByteQRCode itself.