-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Reduced some allocations in QRCodeGenerator
(NETCORE_APP only)
#595
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
base: master
Are you sure you want to change the base?
Conversation
if (toGlue.Contains(resultPolynom[i].Exponent)) | ||
#else | ||
if (Array.IndexOf(toGlue, resultPolynom[i].Exponent) >= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before Linq was used w/ it's generic Contains
method.
- for AOT Linq requires way more code
- it's an interface dispatch that isn't needed
Note: for .NET (Core) the span-based Contains
is used.
{ | ||
var dic = new Dictionary<int, bool>(list.Count); | ||
Debug.Assert(list.Count == buffer.Length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to leave the Debug.Assert
code in here? Just wondering; it will get removed from release builds anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to have some Debug.Assert
s
- they check some invariants in debug builds and while running tests
- are a bit of self-documenting the intention (so why write a comment, when the assert can also be done?)
Especially here the buffer
is passed in as argument and must have the correct size.
If the size doesn't match, then the tests (under debug) will fail and one knows why. Otherwise it may be hard to track down the bug.
So I'd leave them in the code.
As you said: for !DEBUG these asserts won't have any effect.
I wouldn't. There has been so many performance enhancements in .NET Core since .NET Framework, that if anyone wants better performance, they should use .NET Core. And I'm sure the fallback performance is plenty good enough anyway. |
The reduction in allocations is very impressive! |
I fixed the CI scripts in #592 btw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! (subject to tests passing, of course)
Honestly yes… |
Found some more allocation that can be quite easily avoided.
(note: time column isn't really comparable to results in top post (different machine), so only allocations count here) |
@@ -36,7 +37,17 @@ private static class CapacityTables | |||
/// block group details, and other parameters required for encoding error correction data. | |||
/// </returns> | |||
public static ECCInfo GetEccInfo(int version, ECCLevel eccLevel) | |||
=> _capacityECCTable.Single(x => x.Version == version && x.ErrorCorrectionLevel == eccLevel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside of Linq is that no (generic) state can be passed in, thus a closure is needed, and this one showed up in a memory-profile.
@@ -92,11 +103,19 @@ public static int CalculateMinimumVersion(int length, EncodingMode encMode, ECCL | |||
} | |||
|
|||
// if no version was found, throw an exception | |||
var maxSizeByte = _capacityTable.Where( | |||
x => x.Details.Any( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This closure also showed up, and unfortunately the closure (display-class) gets created at method entry, and not when actually needed, so we have to work around this.
Profiling showed that there are some allocations in
QRCodeGenerator
that can be quite easily avoided.simple console app for profiling
Allocations are removed / avoided for:
Dictionary<,>.Entry[]
QRCodeGenerator.PolynomItem[]
Dictionary<,>
The change is done only for .NET (Core) targets, as
Span<T>
is used.By adding a reference to System.Memory package this change could also be done for .NET Desktop.
Profile
Before
After
Benchmarks
Before
After