-
-
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?
Changes from all commits
804d4b9
29d9e0e
bde87ee
59b2690
58480c9
d694875
9baf6ce
38277b1
8bc3c30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
#endif | ||
using System.Collections; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Globalization; | ||
using System.Linq; | ||
using System.Runtime.CompilerServices; | ||
|
@@ -120,8 +121,14 @@ public static QRCodeData GenerateQrCode(string plainText, ECCLevel eccLevel, boo | |
//Version was passed as fixed version via parameter. Thus let's check if chosen version is valid. | ||
if (minVersion > version) | ||
{ | ||
var maxSizeByte = CapacityTables.GetVersionInfo(version).Details.First(x => x.ErrorCorrectionLevel == eccLevel).CapacityDict[encoding]; | ||
throw new QRCoder.Exceptions.DataTooLongException(eccLevel.ToString(), encoding.ToString(), version, maxSizeByte); | ||
// Use a throw-helper to avoid allocating a closure | ||
Throw(eccLevel, encoding, version); | ||
|
||
static void Throw(ECCLevel eccLevel, EncodingMode encoding, int version) | ||
{ | ||
var maxSizeByte = CapacityTables.GetVersionInfo(version).Details.First(x => x.ErrorCorrectionLevel == eccLevel).CapacityDict[encoding]; | ||
throw new Exceptions.DataTooLongException(eccLevel.ToString(), encoding.ToString(), version, maxSizeByte); | ||
} | ||
} | ||
} | ||
|
||
|
@@ -217,8 +224,9 @@ private static QRCodeData GenerateQrCode(BitArray bitArray, ECCLevel eccLevel, i | |
// Place interleaved data on module matrix | ||
var qrData = PlaceModules(); | ||
|
||
return qrData; | ||
CodewordBlock.ReturnList(codeWordWithECC); | ||
|
||
return qrData; | ||
|
||
// fills the bit array with a repeating pattern to reach the required length | ||
void PadData() | ||
|
@@ -254,7 +262,7 @@ List<CodewordBlock> CalculateECCBlocks() | |
using (var generatorPolynom = CalculateGeneratorPolynom(eccInfo.ECCPerBlock)) | ||
{ | ||
//Calculate error correction words | ||
codewordBlocks = new List<CodewordBlock>(eccInfo.BlocksInGroup1 + eccInfo.BlocksInGroup2); | ||
codewordBlocks = CodewordBlock.GetList(eccInfo.BlocksInGroup1 + eccInfo.BlocksInGroup2); | ||
AddCodeWordBlocks(1, eccInfo.BlocksInGroup1, eccInfo.CodewordsInGroup1, 0, bitArray.Length, generatorPolynom); | ||
int offset = eccInfo.BlocksInGroup1 * eccInfo.CodewordsInGroup1 * 8; | ||
AddCodeWordBlocks(2, eccInfo.BlocksInGroup2, eccInfo.CodewordsInGroup2, offset, bitArray.Length - offset, generatorPolynom); | ||
|
@@ -286,7 +294,7 @@ int CalculateInterleavedLength() | |
for (var i = 0; i < eccInfo.ECCPerBlock; i++) | ||
{ | ||
foreach (var codeBlock in codeWordWithECC) | ||
if (codeBlock.ECCWords.Length > i) | ||
if (codeBlock.ECCWords.Count > i) | ||
length += 8; | ||
} | ||
length += CapacityTables.GetRemainderBits(version); | ||
|
@@ -309,8 +317,8 @@ BitArray InterleaveData() | |
for (var i = 0; i < eccInfo.ECCPerBlock; i++) | ||
{ | ||
foreach (var codeBlock in codeWordWithECC) | ||
if (codeBlock.ECCWords.Length > i) | ||
pos = DecToBin(codeBlock.ECCWords[i], 8, data, pos); | ||
if (codeBlock.ECCWords.Count > i) | ||
pos = DecToBin(codeBlock.ECCWords.Array![i], 8, data, pos); | ||
} | ||
|
||
return data; | ||
|
@@ -484,7 +492,7 @@ private static void GetVersionString(BitArray vStr, int version) | |
/// This method applies polynomial division, using the message polynomial and a generator polynomial, | ||
/// to compute the remainder which forms the ECC codewords. | ||
/// </summary> | ||
private static byte[] CalculateECCWords(BitArray bitArray, int offset, int count, ECCInfo eccInfo, Polynom generatorPolynomBase) | ||
private static ArraySegment<byte> CalculateECCWords(BitArray bitArray, int offset, int count, ECCInfo eccInfo, Polynom generatorPolynomBase) | ||
{ | ||
var eccWords = eccInfo.ECCPerBlock; | ||
// Calculate the message polynomial from the bit array data. | ||
|
@@ -532,9 +540,16 @@ private static byte[] CalculateECCWords(BitArray bitArray, int offset, int count | |
generatorPolynom.Dispose(); | ||
|
||
// Convert the resulting polynomial into a byte array representing the ECC codewords. | ||
var ret = new byte[leadTermSource.Count]; | ||
#if NETCOREAPP | ||
var array = ArrayPool<byte>.Shared.Rent(leadTermSource.Count); | ||
var ret = new ArraySegment<byte>(array, 0, leadTermSource.Count); | ||
#else | ||
var ret = new ArraySegment<byte>(new byte[leadTermSource.Count]); | ||
var array = ret.Array!; | ||
#endif | ||
|
||
for (var i = 0; i < leadTermSource.Count; i++) | ||
ret[i] = (byte)leadTermSource[i].Coefficient; | ||
array[i] = (byte)leadTermSource[i].Coefficient; | ||
|
||
// Free memory used by the message polynomial. | ||
leadTermSource.Dispose(); | ||
|
@@ -1017,8 +1032,15 @@ private static Polynom MultiplyAlphaPolynoms(Polynom polynomBase, Polynom polyno | |
} | ||
|
||
// Identify and merge terms with the same exponent. | ||
#if NETCOREAPP | ||
var toGlue = GetNotUniqueExponents(resultPolynom, resultPolynom.Count <= 128 ? stackalloc int[128].Slice(0, resultPolynom.Count) : new int[resultPolynom.Count]); | ||
var gluedPolynoms = toGlue.Length <= 128 | ||
? stackalloc PolynomItem[128].Slice(0, toGlue.Length) | ||
: new PolynomItem[toGlue.Length]; | ||
#else | ||
var toGlue = GetNotUniqueExponents(resultPolynom); | ||
var gluedPolynoms = new PolynomItem[toGlue.Length]; | ||
#endif | ||
var gluedPolynomsIndex = 0; | ||
foreach (var exponent in toGlue) | ||
{ | ||
|
@@ -1036,7 +1058,11 @@ private static Polynom MultiplyAlphaPolynoms(Polynom polynomBase, Polynom polyno | |
|
||
// Remove duplicated exponents and add the corrected ones back. | ||
for (int i = resultPolynom.Count - 1; i >= 0; i--) | ||
#if NETCOREAPP | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. Before Linq was used w/ it's generic
Note: for .NET (Core) the span-based |
||
#endif | ||
resultPolynom.RemoveAt(i); | ||
foreach (var polynom in gluedPolynoms) | ||
resultPolynom.Add(polynom); | ||
|
@@ -1046,20 +1072,66 @@ private static Polynom MultiplyAlphaPolynoms(Polynom polynomBase, Polynom polyno | |
return resultPolynom; | ||
|
||
// Auxiliary function to identify exponents that appear more than once in the polynomial. | ||
int[] GetNotUniqueExponents(Polynom list) | ||
#if NETCOREAPP | ||
static ReadOnlySpan<int> GetNotUniqueExponents(Polynom list, Span<int> buffer) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implementation doesn't need a It works as follows:
For writing the result the same scratch buffer is used, as by definition the index to write the result is That way we avoid the need for a second scratch buffer. Should someting like this be added as comment or is it cleare enough how it works? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function isn't long; so long as the method comment is descriptive enough, I think it's fine. |
||
{ | ||
var dic = new Dictionary<int, bool>(list.Count); | ||
// It works as follows: | ||
// 1. a scratch buffer of the same size as the list is passed in | ||
// 2. exponents are written / copied to that scratch buffer | ||
// 3. scratch buffer is sorted, thus the exponents are in order | ||
// 4. for each item in the scratch buffer (= ordered exponents) it's compared w/ the previous one | ||
// * if equal, then increment a counter | ||
// * else check if the counter is $>0$ and if so write the exponent to the result | ||
// | ||
// For writing the result the same scratch buffer is used, as by definition the index to write the result | ||
// is `<=` the iteration index, so no overlap, etc. can occur. | ||
|
||
Debug.Assert(list.Count == buffer.Length); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you plan to leave the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like to have some
Especially here the So I'd leave them in the code. As you said: for !DEBUG these asserts won't have any effect. |
||
|
||
int idx = 0; | ||
foreach (var row in list) | ||
{ | ||
#if NETCOREAPP | ||
if (dic.TryAdd(row.Exponent, false)) | ||
dic[row.Exponent] = true; | ||
buffer[idx++] = row.Exponent; | ||
} | ||
|
||
buffer.Sort(); | ||
|
||
idx = 0; | ||
int expCount = 0; | ||
int last = buffer[0]; | ||
|
||
for (int i = 1; i < buffer.Length; ++i) | ||
{ | ||
if (buffer[i] == last) | ||
{ | ||
expCount++; | ||
} | ||
else | ||
{ | ||
if (expCount > 0) | ||
{ | ||
Debug.Assert(idx <= i - 1); | ||
|
||
buffer[idx++] = last; | ||
expCount = 0; | ||
} | ||
} | ||
|
||
last = buffer[i]; | ||
} | ||
|
||
return buffer.Slice(0, idx); | ||
} | ||
#else | ||
static int[] GetNotUniqueExponents(Polynom list) | ||
{ | ||
var dic = new Dictionary<int, bool>(list.Count); | ||
foreach (var row in list) | ||
{ | ||
if (!dic.ContainsKey(row.Exponent)) | ||
dic.Add(row.Exponent, false); | ||
else | ||
dic[row.Exponent] = true; | ||
#endif | ||
} | ||
|
||
// Collect all exponents that appeared more than once. | ||
|
@@ -1080,6 +1152,7 @@ int[] GetNotUniqueExponents(Polynom list) | |
|
||
return result; | ||
} | ||
#endif | ||
} | ||
|
||
/// <inheritdoc cref="IDisposable.Dispose"/> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
|
@@ -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 commentThe 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. |
||
{ | ||
foreach (var item in _capacityECCTable) | ||
{ | ||
if (item.Version == version && item.ErrorCorrectionLevel == eccLevel) | ||
{ | ||
return item; | ||
} | ||
} | ||
|
||
throw new InvalidOperationException("No item found"); // same exception type as Linq would throw | ||
} | ||
|
||
/// <summary> | ||
/// Retrieves the capacity information for a specific QR code version. | ||
|
@@ -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 commentThe 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. |
||
y => (y.ErrorCorrectionLevel == eccLevel)) | ||
).Max(x => x.Details.Single(y => y.ErrorCorrectionLevel == eccLevel).CapacityDict[encMode]); | ||
throw new QRCoder.Exceptions.DataTooLongException(eccLevel.ToString(), encMode.ToString(), maxSizeByte); | ||
// In order to get the maxSizeByte we use a throw-helper method to avoid the allocation of a closure | ||
Throw(encMode, eccLevel); | ||
throw null!; // this is needed to make the compiler happy | ||
|
||
static void Throw(EncodingMode encMode, ECCLevel eccLevel) | ||
{ | ||
var maxSizeByte = _capacityTable.Where( | ||
x => x.Details.Any( | ||
y => (y.ErrorCorrectionLevel == eccLevel)) | ||
).Max(x => x.Details.Single(y => y.ErrorCorrectionLevel == eccLevel).CapacityDict[encMode]); | ||
|
||
throw new Exceptions.DataTooLongException(eccLevel.ToString(), encMode.ToString(), maxSizeByte); | ||
} | ||
} | ||
|
||
/// <summary> | ||
|
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.
Unfortunately I don't know too little about the QR-specs, but the
resultPolynom.Count <= 128
could be avoided if by spec the count can't be as high. Or we can change the threshold to a higher value.If the count can't be as high, then the fallback to the array allocation could also be removed.
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.
When running the tests, it doesn't go over 64. Seems fine to me.
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.
My tests showed the same (what a wonder 😉).
But in that regard I'm a bit paranoid...in the sense wenn the fallback is removed what will happen when a bigger buffer is needed (maybe now by some special inputs or anytime in the future when new additions may be done)? This would result in an exception. W/ the fallback there's a safety net.
Except it can be proven via the QR-spec that the count never will exceed a certain threshold (but does that hold in the future too?)
If the non-stackalloc path would be very frequent, then renting from the array-pool would be an option, but here it's a assumed to be rare / never taken path.
Would you still remove the fallback or let's just leave it as is?
Uh oh!
There was an error while loading. Please reload this page.
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.
Oh I’d definitely leave the fallback. There’s just no reason to remove it. The JIT will realize it’s not a common pathway and optimize it accordingly. But as you say, we neither of us know enough about how these polynomials work to know if it will always be < 128.