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 improvements #509

Merged
merged 32 commits into from
May 3, 2024
Merged

Performance improvements #509

merged 32 commits into from
May 3, 2024

Conversation

Shane32
Copy link
Contributor

@Shane32 Shane32 commented Apr 30, 2024

System

BenchmarkDotNet v0.13.12, Windows 10 (10.0.19045.4291/22H2/2022Update)
13th Gen Intel Core i9-13900K, 1 CPU, 32 logical and 24 physical cores
.NET SDK 8.0.202
  [Host]     : .NET 8.0.3 (8.0.324.11423), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.3 (8.0.324.11423), X64 RyuJIT AVX2

Before

Method Mean Error StdDev Gen0 Gen1 Allocated
CreateQRCode 245.4 us 4.88 us 5.80 us 17.0898 0.2441 317.99 KB
CreateQRCodeLong 7,531.1 us 34.13 us 31.92 us 265.6250 15.6250 4920.13 KB

After

Method Mean Error StdDev Gen0 Allocated
CreateQRCode 100.7 us 0.26 us 0.22 us 1.2207 23.18 KB
CreateQRCodeLong 1,842.1 us 3.19 us 2.83 us 23.4375 433.53 KB

Notes

Most of the performance improvements in this PR fall into a few categories:

  • Use BitArray instead of string
  • Eliminate concatenation and instead write/copy into buffers using offsets and counts
  • Preallocation of arrays/lists to proper length
  • Eliminate array allocation when unnecessary (e.g. don't call AddRange)
  • Eliminate LINQ use
  • Improve lookup tables / dictionaries

Progress

So far about half the code has been converted to use BitArray. There is still a notable amount of code that still uses strings that remains to be optimized. Also the Polynom class/constructor is heavily used and should be optimized.

QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Show resolved Hide resolved
@Shane32
Copy link
Contributor Author

Shane32 commented Apr 30, 2024

FYI, I have not yet gone back through all my changes to add comments and perform a self-review, etc.

@Shane32
Copy link
Contributor Author

Shane32 commented Apr 30, 2024

By the way, I plan to attempt using ArrayPool for further optimizations but not in this PR.

@Shane32
Copy link
Contributor Author

Shane32 commented May 1, 2024

@codebude I think this PR has enough changes. The GitHub comparison should be reasonable enough to follow. I would like to follow up with some other PRs:

  • Improve readability (code comments, rename variables, split into multiple files, etc)
  • Fix bug (?) with string handling
  • Utilize ArrayPool and other memory optimizations

@Shane32 Shane32 changed the title [WIP] Performance improvements Performance improvements May 1, 2024
@Shane32
Copy link
Contributor Author

Shane32 commented May 1, 2024

The benchmarks are updated in the PR description, showing 60-75% memory reduction and 100-400% speed increase. Notably, Gen1 collections have been reduced enough that BenchmarkDotNet has stopped listing them!

Most likely further PRs will further reduce memory requirements, but not increase speed.

@Shane32
Copy link
Contributor Author

Shane32 commented May 1, 2024

I'm not planning to re-engineer the mathematics, like to use vector math or anything. While I'm sure it's possible, that's not my forte.

QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
Shane32 and others added 5 commits May 2, 2024 08:55
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
Shane32 and others added 3 commits May 2, 2024 09:02
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
Co-authored-by: Günther Foidl <gue@korporal.at>
@Shane32
Copy link
Contributor Author

Shane32 commented May 2, 2024

@gfoidl Thanks for the review! I merged as many of them as I could via GitHub.com, and I'll need to spend a little more time integrating the rest of your comments.

@codebude
Copy link
Owner

codebude commented May 2, 2024

@gfoidl Thank you very much for your commitment. This is really a top-level review! Great!

@Shane32
Copy link
Contributor Author

Shane32 commented May 3, 2024

Benchmarks updated. New test results shaved 10us (10%) from short test and 20us (1%) from long test.

@Shane32
Copy link
Contributor Author

Shane32 commented May 3, 2024

Eliminating foreach across a BitArray and implementing IEquatable for Point cut down memory requirements for small QR codes by an additional 80%, and an additional 60% for large QR codes.

@codebude
Copy link
Owner

codebude commented May 3, 2024

Benchmarks updated. New test results shaved 10us (10%) from short test and 20us (1%) from long test.

But memory usage improved massively. :-)

@Shane32
Copy link
Contributor Author

Shane32 commented May 3, 2024

While all beneficial changes, the memory usage didn't change due to the arithmetic/ref/pointer changes; it measured as identical. The memory usage changes were due to foreach across a BitArray internally cast a bool to an object causing a heap allocation for each bit. Ditto for List<Point>.Contains due to Point not implementing IEquatable. These were accessed across some 'hot' code, and so many unnecessary heap allocations were made.

Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

A few nits, otherwise LGTM 👍🏻

QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
QRCoder/QRCodeGenerator.cs Outdated Show resolved Hide resolved
@Shane32 Shane32 requested review from codebude and gfoidl May 3, 2024 14:15
@codebude codebude self-requested a review May 3, 2024 14:37
@codebude codebude merged commit e36d34d into codebude:master May 3, 2024
3 checks passed
@codebude
Copy link
Owner

codebude commented May 3, 2024

Merged. Thanks for your great work @Shane32 & @gfoidl ! 👏

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.

3 participants