Format/Parse binary from/to BigInteger #85392

lateapexearlyspeed · 2023-04-26T11:48:54Z

Fix #83619
Hi, currently PR is just in draft stage and in developing, will mark it ready for review when ready, thanks.
Would need final binary format definition firstly.

This is marked as ready for review now, thanks.

ghost · 2023-04-26T11:49:06Z

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #83619
Hi, currently PR is just in draft stage and in developing, will mark it ready for review when ready, thanks.

Author:	lateapexearlyspeed
Assignees:	-
Labels:	`area-System.Numerics`
Milestone:	-

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs

IDisposable · 2023-04-26T22:45:07Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs

+            }
+
+            // each byte is typically eight chars
+            var sb = new ValueStringBuilder(stackalloc char[Math.Min(charCount, 512)]);


maybe

var sb = (charCount <= 512) ? new ValueStringBuilder(stackalloc char[charCount]) : new ValueStringBuilder(charCount); }

Fixed, didn't know this pattern can mark sb as 'unsafe to return' for stackalloc before, thanks !

…t 'unsafe to return' so that can assign stackalloced.

…ion span, so to save extra buffer allocation and copy in ValueStringBuilder.

lateapexearlyspeed · 2023-06-01T14:41:10Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs

@@ -1002,6 +1096,103 @@ internal static char ParseFormatSpecifier(ReadOnlySpan<char> format, out int dig
            }
        }

+        private static string? FormatBigIntegerToBin(bool targetSpan, BigInteger value, int digits, Span<char> destination, out int charsWritten, out bool spanSuccess)


Unlike existing FormatBigIntegerToHex(), this FormatBigIntegerToBin() is implemented by calculating required char length before format, this can:

avoids from extending ValueStringBuilder's capacity during append char;

for targetSpan flow, formatted chars can write to wanted destination span directly rather than allocate buffer in ValueStringBuilder and copy to destination at the end

Please give advice if not proper, thanks !

IDisposable · 2023-06-01T18:17:47Z

src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs

@@ -440,11 +440,11 @@ private static unsafe bool ParseNumber(ref char* str, char* strEnd, NumberStyles
                int digEnd = 0;
                while (true)
                {
-                    if (char.IsAsciiDigit(ch) || (((options & NumberStyles.AllowHexSpecifier) != 0) && char.IsBetween((char)(ch | 0x20), 'a', 'f')))
+                    if ((options & NumberStyles.AllowBinarySpecifier) == 0 ? char.IsAsciiDigit(ch) || ((options & NumberStyles.AllowHexSpecifier) != 0 && char.IsBetween((char)(ch | 0x20), 'a', 'f')) : char.IsBetween(ch, '0', '1'))


would that last bit of the ternary be better as : (char == '0' || char == '1) instead of the char.IsBetween(ch, '0', '1')? Seems we're really driving toward optimal performance :)

I also struggled on these 2, but selected to use char.IsBetween() based on its implementation because it can always just use one check but need 2 arithmetic operation..

Np, changed to (char == '0' || char == '1) now, thanks.

IDisposable · 2023-06-01T18:29:15Z

src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs

                    {
                        state |= StateDigits;

-                        if (ch != '0' || (state & StateNonZero) != 0 || (bigNumber && ((options & NumberStyles.AllowHexSpecifier) != 0)))
+                        if (ch != '0' || (state & StateNonZero) != 0 || (bigNumber && (options & (NumberStyles.AllowHexSpecifier | NumberStyles.AllowBinarySpecifier)) != 0))


Should we hoist this options test and the ones on line 443 to outside the loop so they are only bool flags by the time we're inside the loop?

Perhaps insert around line 440...

const bool allowBinary = (options & NumberStyles.AllowBinarySpecifier) != 0 ; const bool allowHex = (options & NumberStyles.AllowHexSpecifier) != 0 ; const bool allowLeadingZero = allowBinary || allowHex;

Then inside the loop, at line 443 looks like

if (allowBinary ? (char == '0' || char == '1) : char.IsAsciiDigit(ch) || (allowHex && char.IsBetween((char)(ch | 0x20), 'a', 'f')))

and line 447 becomes

if (ch != '0' || (state & StateNonZero) != 0 || (bigNumber && allowLeadingZero))

The same thing could be done for options tested elsewhere inside the loop if performance testing makes that wise...

I am also not sure what's going on with the bigNumber && here. This flag is set to indicate that we're backed by a StringBuilder, not that we're parsing a particular kind of number...

love all the new tests :)

Should we hoist this options test

Got your point. As you found all the existing options tests code were inside whole loop, and during run time some option tests may happen only when data is met, so not sure whether we should hoist 'options' test which will turn to be called once but always happen. That is why I did not change this convention :)

I am also not sure what's going on with the bigNumber && here.

I am also not sure, so that I just added Binary stuff behind it like Hex :)

Splitting it apart for readability is generally helpful. The logic is getting quite verbose to have on a single line IMO.

For the primitive types, we have it split apart into some early checks that go to TryParseBinaryInteger*Style: https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/Number.Parsing.cs,437

The check for "IsValidChar" can then be specialized by use of the generics and made a lot cheaper.

@tannergooding Fixed. After introducing Hex/Bin parser types, here simplifies not only prev line 447, but also prev line 443 which involved hex/binary checks previously. Please check, thanks.

…ions for previous Hex and new Binary.

tannergooding · 2023-08-30T15:56:47Z

src/libraries/Common/src/System/Globalization/FormatProvider.Number.cs

+            {
+                public static bool IsValidChar(char c) => char.IsAsciiDigit(c);
+
+                public static bool IsHexBinary() => false;


nit: IsHexOrBinary

-or- maybe IsHexOrBinaryParser and rename IDigitValidator to IDigitParser, to more closely match the other.

Longer term I'd like to see this move even closer to the IHexOrBinaryParser logic we have in corelib, but that can be done in a future PR and isn't necessary as part of this.

Having the names and general logic mostly start to line up helps with that, however.

Fixed, thanks.

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs

…ern similar as HexNumberToBigInteger's

lateapexearlyspeed · 2023-09-05T10:53:51Z

@tannergooding all comments are fixed, please check.

tannergooding

LGTM.

It would be good to get a secondary review from someone. CC. @stephentoub since you've helped on similar bits for the corelib side (let me know if you're busy and I can find someone else).

stephentoub · 2023-10-03T14:40:36Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs

@@ -511,6 +515,112 @@ private static ParsingStatus HexNumberToBigInteger(ref BigNumberBuffer number, o
            }
        }

+        private static ParsingStatus BinNumberToBigInteger(ref BigNumberBuffer number, out BigInteger result)


Nit: can we expand "Bin" to "Binary" in the name? I realize this is likely an attempt to match the conciseness of "Hex", but "Bin" and "Big" are so close that this keeps making me do a double-take to know which it was.

Renamed method names, also for other 'Bin' related ones.

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs

stephentoub · 2023-10-03T14:48:09Z

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs

+                    // each byte is typically eight chars
+                    sb = charsIncludeDigits > 512
+                        ? new ValueStringBuilder(charsIncludeDigits)
+                        : new ValueStringBuilder(stackalloc char[512].Slice(0, charsIncludeDigits));


Why slice it?

Yes it is unnecessary, removed.

stephentoub

Thanks.

…essary Slice().

lateapexearlyspeed · 2023-10-05T08:07:19Z

Hi @tannergooding I have also addressed @stephentoub 's comments.

lateapexearlyspeed · 2023-10-12T16:50:44Z

Hi @tannergooding any chance to have a look at status of this approved PR ? thanks !

Initial draft commit: add FormatBigIntegerToBin().

3c127ea

dotnet-issue-labeler bot added the area-System.Numerics label Apr 26, 2023

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 26, 2023

IDisposable reviewed Apr 26, 2023

View reviewed changes

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs Show resolved Hide resolved

IDisposable reviewed Apr 26, 2023

View reviewed changes

lateapexearlyspeed added 12 commits April 28, 2023 17:37

Fix comment: use '?:' to assign ValueStringBuilder variable to make i…

64a0ead

…t 'unsafe to return' so that can assign stackalloced.

Refine FormatBigIntegerToBin(); and consider chars overflow scenario.

4216498

Update Format code for final binary format definition.

1bcf7f8

Refine FormatBigIntegerToBin().

63923da

consider case where output is span

19c701d

Turn to use try..finally to return array pool.

56e701f

Initial add method BinNumberToBigInteger().

817c58c

Update FormatProvider.Number.cs to support AllowBinarySpecifier.

24d88c7

Use BinNumberToBigInteger().

5a90e15

Add tests of Format.

3987f71

Add tests of Parse().

8b58eb7

Improve Format(): use ValueStringBuilder just as wrapper for destinat…

6cea91a

…ion span, so to save extra buffer allocation and copy in ValueStringBuilder.

build-analysis bot mentioned this pull request Jun 1, 2023

Tracking issue for CI build timeouts #76454

Closed

lateapexearlyspeed commented Jun 1, 2023

View reviewed changes

lateapexearlyspeed marked this pull request as ready for review June 1, 2023 14:44

lateapexearlyspeed mentioned this pull request Jun 1, 2023

[API Proposal]: Support binary format specifier 'b' and 'B' (standard numeric format strings) #83619

Closed

IDisposable reviewed Jun 1, 2023

View reviewed changes

Fix comment: use ch == '0' || ch == '1'

3007b48

This was referenced Jun 2, 2023

Failing test System.Net.Quic.Tests.MsQuicPlatformDetectionTests.SupportedLinuxPlatforms #87038

Closed

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

tannergooding self-assigned this Jun 2, 2023

IDisposable approved these changes Jun 19, 2023

View reviewed changes

huoyaoyuan mentioned this pull request Jul 5, 2023

Unify parsing part of BigInteger with CoreLib #85978

Merged

Fix comment: refactor ParseNumber() to extract common abstract operat…

cd0a03d

…ions for previous Hex and new Binary.

build-analysis bot mentioned this pull request Aug 25, 2023

Http2_MultipleConnectionsEnabled test failures #91075

Closed

tannergooding reviewed Aug 30, 2023

View reviewed changes

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs Outdated Show resolved Hide resolved

tannergooding reviewed Aug 30, 2023

View reviewed changes

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs Outdated Show resolved Hide resolved

tannergooding reviewed Aug 30, 2023

View reviewed changes

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs Outdated Show resolved Hide resolved

tannergooding reviewed Aug 30, 2023

View reviewed changes

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs Outdated Show resolved Hide resolved

Fix comment: refine naming; make BinNumberToBigInteger() general patt…

f22d2e9

…ern similar as HexNumberToBigInteger's

build-analysis bot mentioned this pull request Sep 1, 2023

Microsoft.NET.HostModel.Tests failing with "No space left on device" #91039

Closed

lateapexearlyspeed requested a review from tannergooding September 14, 2023 14:08

tannergooding approved these changes Sep 14, 2023

View reviewed changes

Fix comment: use internal 'kcbitUint'.

17434aa

build-analysis bot mentioned this pull request Sep 15, 2023

LibraryImportGenerator unit test segfault illegal memory access #89054

Closed

stephentoub reviewed Oct 3, 2023

View reviewed changes

src/libraries/System.Runtime.Numerics/src/System/Numerics/BigNumber.cs Show resolved Hide resolved

stephentoub reviewed Oct 3, 2023

View reviewed changes

stephentoub approved these changes Oct 3, 2023

View reviewed changes

Fix comment: rename 'Bin' method names to 'Binary' ones; remove unnec…

bdfddf5

…essary Slice().

tannergooding merged commit 71d6476 into dotnet:main Oct 12, 2023
109 checks passed

ghost locked as resolved and limited conversation to collaborators Nov 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Format/Parse binary from/to BigInteger #85392

Format/Parse binary from/to BigInteger #85392

lateapexearlyspeed commented Apr 26, 2023 •

edited

Loading

ghost commented Apr 26, 2023

IDisposable Apr 26, 2023

lateapexearlyspeed Apr 29, 2023

lateapexearlyspeed Jun 1, 2023

IDisposable Jun 1, 2023

lateapexearlyspeed Jun 2, 2023 •

edited

Loading

IDisposable Jun 1, 2023

IDisposable Jun 1, 2023

IDisposable Jun 1, 2023

lateapexearlyspeed Jun 2, 2023

tannergooding Aug 23, 2023

lateapexearlyspeed Aug 28, 2023

tannergooding Aug 30, 2023

tannergooding Aug 30, 2023

lateapexearlyspeed Sep 1, 2023

lateapexearlyspeed commented Sep 5, 2023

tannergooding left a comment

stephentoub Oct 3, 2023

lateapexearlyspeed Oct 5, 2023

stephentoub Oct 3, 2023

lateapexearlyspeed Oct 5, 2023

stephentoub left a comment

lateapexearlyspeed commented Oct 5, 2023

lateapexearlyspeed commented Oct 12, 2023

Format/Parse binary from/to BigInteger #85392

Format/Parse binary from/to BigInteger #85392

Conversation

lateapexearlyspeed commented Apr 26, 2023 • edited Loading

ghost commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lateapexearlyspeed Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lateapexearlyspeed commented Sep 5, 2023

tannergooding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

lateapexearlyspeed commented Oct 5, 2023

lateapexearlyspeed commented Oct 12, 2023

lateapexearlyspeed commented Apr 26, 2023 •

edited

Loading

lateapexearlyspeed Jun 2, 2023 •

edited

Loading