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

Fix Bin/Hex parsing of BigInteger for powers of 2 #5

Merged

Conversation

kzrnm
Copy link

@kzrnm kzrnm commented Jan 25, 2024

dotnet#95543 (comment)

I fixed Bin/Hex parsing of BigInteger for powers of 2.

// -1 << 32
BigInteger.Parse("100000000000000000000000000000000", NumberStyles.BinaryNumber);
BigInteger.Parse("F00000000", NumberStyles.HexNumber);

// -1 << 64
BigInteger.Parse("10000000000000000000000000000000000000000000000000000000000000000", NumberStyles.BinaryNumber);
BigInteger.Parse("F0000000000000000", NumberStyles.HexNumber);

Comment on lines +489 to +490
// Small value that fits in Int32.
result = new BigInteger((int)leading);
Copy link
Author

Choose a reason for hiding this comment

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

I removed the (int)leading == int.MinValue condition and changed it to call new BigInteger(int).
It avoids allocation by using BigInteger.s_bnMinInt.

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, I didn't realize that BigInteger(int) constructor has an optimization to avoid the allocation for int.MinValue.
In previous commit I delegated to that constructor to handle MinValue. I'll update the comment.

@huoyaoyuan
Copy link
Owner

Sorry, I didn't mention this was in my fork.

Copy link
Owner

@huoyaoyuan huoyaoyuan left a comment

Choose a reason for hiding this comment

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

Thank you for pointing out my mistakes!

I'm going to think more about optimization and measure the performance of each case.

Comment on lines +489 to +490
// Small value that fits in Int32.
result = new BigInteger((int)leading);
Copy link
Owner

Choose a reason for hiding this comment

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

Interesting, I didn't realize that BigInteger(int) constructor has an optimization to avoid the allocation for int.MinValue.
In previous commit I delegated to that constructor to handle MinValue. I'll update the comment.

Comment on lines +473 to +484
if ((int)signBits < 0)
{
// Negative value
result = leading == 0
? new BigInteger(-1, [leading, 1u])
: new BigInteger(-1, [unchecked((uint)-(int)leading)]);
}
else
{
// Positive value
result = new BigInteger(1, [leading]);
}
Copy link
Owner

Choose a reason for hiding this comment

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

I missed that the bits of negative result needs negation here.

The non-zero case for negative and positive can be combined in bit hack: leading ^ signBits - signBits. The zero case needs [0, 1].

Comment on lines +519 to +524
Span<uint> trimmed = new Span<uint>(bits).TrimStart(0u);
if (trimmed.Length == 0)
{
bits = new uint[bits.Length + 1];
bits[^1] = 1;
}
Copy link
Owner

Choose a reason for hiding this comment

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

A new digit is required only if all the trailing digits are zero.
It's really a headache for allocation management.

@huoyaoyuan huoyaoyuan merged commit 07359c5 into huoyaoyuan:biginteger-hex-vectorize Feb 1, 2024
@kzrnm kzrnm deleted the fix-bigint-hex-bin branch February 1, 2024 10:17
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants