-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
BigInteger constructor perf #91176
BigInteger constructor perf #91176
Conversation
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsUse BinaryPrimitives/MemoryMarshal in BigInteger constructor
public class Benchmark
{
[Params(5, 16, 64, 256)]
public int ByteCount;
[Params(true, false)]
public bool IsBigEndian;
private byte[] _bytes;
[GlobalSetup]
public void Setup()
{
Random r = new(123);
_bytes = new byte[ByteCount];
r.NextBytes(_bytes);
}
[Benchmark] public BigInteger Ctor() => new BigInteger(_bytes, isBigEndian: IsBigEndian);
}
|
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
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.
Overall it LGTM, but I would prefer another set of eyes that is more familiar with endianness conversion to confirm that.
@Rob-Hague thank you for your contribution!
src/libraries/System.Runtime.Numerics/src/System/Numerics/BigInteger.cs
Outdated
Show resolved
Hide resolved
/azp list |
/azp run runtime-community |
Azure Pipelines successfully started running 1 pipeline(s). |
|
||
// Copy all dwords, except don't do the last one if it's not a full four bytes | ||
int curDword, curByte; | ||
int wholeDwordCount = Math.DivRem(byteCount, 4, out int unalignedBytes); |
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.
Should we use:
(uint wholeUInt32Count, uint unalignedBytes) = Math.DivRem((uint)byteCount, 4);
instead, since we know here byteCount isn't negative?
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.
If we did, then we would have to cast the values back to int in each of the AsSpan
and Slice
calls. What do you suggest?
Commenting the runtime-community build link in case it disappears when I push https://dev.azure.com/dnceng-public/public/_build/results?buildId=456533&view=results |
/azp run runtime-community |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
LGTM, the s390x failures are unrelated (they are being addressed by #94333)
Use BinaryPrimitives/MemoryMarshal in BigInteger constructor