Double to Decimal Conversion Refactor (WIP) #70602

dakersnar · 2022-06-11T01:59:01Z

This is unfinished, but I'm leaving for vacation, so I wanted to share the current state of this effort.

Problems with previous code

The previous conversion code is buggy.

It relies on false assumptions
- "Round the input to a 15-digit integer. The R8 format has only 15 digits of precision, and we want to keep garbage digits out of the Decimal were making." This is an incorrect assumption, and the cause of the linked bug.
It seems to have solved an off-by-one error in a hacky way
- const uint DBLBIAS = 1022; is conceptually incorrect; the exponent bias for double is 1023
It generally doesn't take advantage of a lot of functionality we now expose in double and decimal

Summary of approach

This PR is a general refactor of the conversion code. The primary goal is to solve the reported bug, but secondarily make this process more efficient by reusing fast code we have written elsewhere. To achieve this, @tannergooding and I brainstormed reusing a section of the Dragon4 algorithm for double to string conversion, as printing a double already involves converting it to base 10.

To do this, I split up the code for the Dragon4 algorithm into two parts and added an additional API that allows Decimal.DecCalc.cs access to the first half of Dragon4. This API is structured to return everything Decimal.DecCalc.cs needs to construct a decimal representation of the original double.

Status of the work

The rough draft of the approach is complete, and the original bug reported in #68042 is now fixed. Unfortunately, the logic does not seem to be holding up for all code paths, as there are a number of failing test cases in DecimalTests.cs. For example:

I haven't had enough time to dive deep into what is causing these issues, but I'm adding GitHub comments to the logic that I think is shaky.

What is left to do

Fix the logic to handle all the failing edge cases
Ensure we have proper test coverage
(Possibly) perf analysis to see if borrowing from Dragon4 is giving the performance we are expecting
If we determine this Dragon4 method is barking up the wrong tree, use another method @tannergooding and I have brainstormed. In my opinion, correctness and maintainability is more important than performance here.
Backport to previous versions of .NET?

… fix-68042

…ction

ghost · 2022-06-11T01:59:21Z

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

Issue Details

Fixes #68042

This is unfinished, but I'm leaving for vacation, so I wanted to share the current state of this effort.

Problems with previous code

The previous conversion code is buggy.

It relies on false assumptions
- "Round the input to a 15-digit integer. The R8 format has only 15 digits of precision, and we want to keep garbage digits out of the Decimal were making." This is an incorrect assumption, and the cause of the linked bug.
It seems to have solved an off-by-one error in a hacky way
- const uint DBLBIAS = 1022; is conceptually incorrect; the exponent bias for double is 1023
It generally doesn't take advantage of a lot of functionality we now expose in double and decimal

Summary of approach

This PR is a general refactor of the conversion code. The primary goal is to solve the reported bug, but secondarily make this process more efficient by reusing fast code we have written elsewhere. To achieve this, @tannergooding and I brainstormed reusing a section of the Dragon4 algorithm for double to string conversion, as printing a double already involves converting it to base 10.

To do this, I split up the code for the Dragon4 algorithm into two parts and added an additional API that allows Decimal.DecCalc.cs access to the first half of Dragon4. This API is structured to return everything Decimal.DecCalc.cs needs to construct a decimal representation of the original double.

Status of the work

The rough draft of the approach is complete, and the original bug reported in #68042 is now fixed. Unfortunately, the logic does not seem to be holding up for all code paths, as there are a number of failing test cases in DecimalTests.cs. For example:

I haven't had enough time to dive deep into what is causing these issues, but I'm adding GitHub comments to the logic that I think is shaky.

What is left to do

Fix the logic to handle all the failing edge cases
Ensure we have proper test coverage
(Possibly) perf analysis to see if borrowing from Dragon4 is giving the performance we are expecting
If we determine this Dragon4 method is barking up the wrong tree, use another method @tannergooding and I have brainstormed. In my opinion, correctness and maintainability is more important than performance here.
Backport to previous versions of .NET?

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

dakersnar · 2022-06-11T02:01:44Z

src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs

                //
-                const uint DBLBIAS = 1022;


As mentioned, this is conceptually incorrect, as possibly even logically incorrect

dakersnar · 2022-06-11T02:02:13Z

src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs

-                // Round the input to a 15-digit integer.  The R8 format has
-                // only 15 digits of precision, and we want to keep garbage digits
-                // out of the Decimal were making.


As mentioned, this is incorrect, and the cause of the original bug

dakersnar · 2022-06-11T02:04:47Z

src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs

-                // power is between -14 and 43
-
-                if (power >= 0)
+                if (input.Exponent < -94)


I'm using the Exponent exposed on double now. I think this is more correct than the original comparison.

dakersnar · 2022-06-11T02:05:37Z

src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs

-                if (X86.Sse41.IsSupported)
-                    mant = (ulong)(long)Math.Round(dbl);
-                else
+                if (input.Exponent >= 96)


This should prevent all overflowing doubles from making it to Dragon4.

dakersnar · 2022-06-11T02:07:54Z

src/libraries/System.Private.CoreLib/src/System/Decimal.cs

-    // / 10e, where m is an integer such that
-    // -296 <; m <; 296, and e is an integer
+    // / 10^e, where m is an integer such that
+    // -2^96 <; m <; 2^96, and e is an integer


Suggested change

// -2^96 <; m <; 2^96, and e is an integer

// -2^96 <= m <= 2^96, and e is an integer

Is there something I'm missing about the use of ; here?

This comment got corrupted in June 2002. Originally it looked this way:

* The finite set of values of type Decimal are of the form m * / 10e, where m is an integer such that * -296 < m < 296, and e is an integer * between 0 and 28 inclusive.

You should use <, not <=. Also please move / 10^e to the previous line.

dakersnar · 2022-06-11T02:22:39Z

src/libraries/System.Private.CoreLib/src/System/Number.Dragon4.cs

+            // require that the DoubleToNumber handle zero itself.
+            Debug.Assert(mantissa != 0);
+
+            Dragon4State state = Dragon4GetScaleValueMargin(mantissa, exponent, mantissaHighBitIdx, hasUnequalMargins, cutoffNumber, isSignificantDigits);


First half of Dragon4 is now hoisted to this helper function.

dakersnar · 2022-06-11T02:23:08Z

src/libraries/System.Private.CoreLib/src/System/Number.Dragon4.cs

@@ -537,5 +458,171 @@ private static unsafe uint Dragon4(ulong mantissa, int exponent, uint mantissaHi
            Debug.Assert(outputLen <= buffer.Length);
            return outputLen;
        }
+
+        private static unsafe Dragon4State Dragon4GetScaleValueMargin(ulong mantissa, int exponent, uint mantissaHighBitIdx, bool hasUnequalMargins, int cutoffNumber, bool isSignificantDigits)


Here is the hoisted first half of Dragon4

dakersnar · 2022-06-11T02:25:08Z

src/libraries/System.Runtime/tests/System/DecimalTests.cs

@@ -224,11 +224,48 @@ public void Ctor_Double(double value, int[] bits)
        [InlineData(double.MinValue)]
        [InlineData(double.PositiveInfinity)]
        [InlineData(double.NegativeInfinity)]
+        [InlineData(79228162514264337593543950335.0)]


This number is decimal.MaxValue, which is actually stored as 79228162514264337593543950336.0 as a double, which has an exponent of 96 and should overflow when cast to a decimal.

dakersnar · 2022-06-11T02:26:04Z

src/libraries/System.Runtime/tests/System/DecimalTests.cs

+        [Fact]
+        public void Ctor_LargeDouble_RoundtripCastSucceeds()
+        {
+            // Decrementing Decimal's MaxValue to get a number that shouldn't lose precision when cast back and forth
+            double x = Math.BitDecrement(79228162514264337593543950335.0);
+
+            // Cast to a decimal
+            decimal y = new decimal(x);
+
+            // Use strings to compare values of double and decimal, ensuring no precision loss
+            string x_string = x.ToString("G99");
+            string y_string =  y.ToString("G99");
+            Assert.Equal(x_string, y_string);
+
+            // Cast back to double, ensuring no precision loss
+            double z = (double)y;
+            Assert.Equal(x, z);
+
+        }


This is the original bug and is passing now.

dakersnar · 2022-06-11T02:26:50Z

src/libraries/System.Runtime/tests/System/DecimalTests.cs

+        [Fact]
+        public void Ctor_SmallDoubleRoundsUp()
+        {
+            // Create a double with decimal's smallest non-zero value
+            double x = .0000000000000000000000000001;
+
+            // Cast to a decimal
+            decimal y = new decimal(x);
+
+            Assert.NotEqual(decimal.Zero, y);
+
+            // Use strings to ensure decimal is correct
+            string y_string = y.ToString("G99");
+            Assert.Equal(".0000000000000000000000000001", y_string);
+        }


This is meant to test conversion to the smallest non-zero decimal value. WIP.

This reverts commit 1c3556d.

…n is fixed

dakersnar · 2022-07-13T21:56:50Z

The code is working and complete but needs some cleanup. I will either make a fresh PR or update this one tomorrow.

dakersnar and others added 14 commits May 3, 2022 14:38

WIP: decimal conversion bug

391f805

Merge branch 'main' of https://github.com/dotnet/runtime into fix-68042

a6dc156

Merge branch 'main' of https://github.com/dotnet/runtime into fix-68042

edc3f67

Merge branch 'main' of https://github.com/dotnet/runtime into fix-68042

cccae84

Merge branch 'fix-68042' of https://github.com/dakersnar/runtime into…

5a19f23

… fix-68042

Split up Dragon4 into two parts in order to reuse the first half later.

b0721b8

Slight refactor to remove unneccesary calculation from the helper fun…

d213837

…ction

WIP: Decimal.DecCalc

728efa1

Merge branch 'main' of https://github.com/dotnet/runtime into fix-68042

31820e3

WIP: Number.Dragon4.cs

1df3d17

WIP: DecimalTests.cs

246483c

Initial unit test now passing

40fcdd5

Updated decimal unit test file

7e6cd93

WIP: Current state of this fix

e77161f

dakersnar requested review from jeffhandley and tannergooding June 11, 2022 01:59

dotnet-issue-labeler bot added the area-System.Numerics label Jun 11, 2022

ghost assigned dakersnar Jun 11, 2022

dakersnar commented Jun 11, 2022

View reviewed changes

dakersnar added 10 commits July 5, 2022 16:37

Remove unneeded include

29bac4b

Possible working solution

94b874b

Fix naming, fix typo of tests

6525508

Adjust comments, tweak style, canonicalize result

895b413

Update unit test data to be more accurate for double and float

5d9edfa

Fix edge cases

e6dbd62

Fix precision of returned zeros

2d191a1

Fix expected values in Decimal's Generic Math tests

47ac650

Fix test data for InsertDecimal and AppendDecimal tests

6db7625

Fix rounding issues

1d38279

dakersnar added 7 commits July 12, 2022 18:13

Slight improvement for decimal->double conversion

1c3556d

Fix rounding

ceccdb5

Normalize test data

2030e68

Revert "Slight improvement for decimal->double conversion"

f1d0451

This reverts commit 1c3556d.

Remove accidental push to DecCalc

8a28c84

Fix Number.BigInteger DivRem Bug

6a7266c

Remove round trip tests temporarily until decimal to double conversio…

a62767d

…n is fixed

Fix comment typos

23b71de

dakersnar closed this Jul 14, 2022

runfoapp bot mentioned this pull request Jul 14, 2022

jit.1 work item failing on mono #67888

Closed

ghost locked as resolved and limited conversation to collaborators Aug 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Double to Decimal Conversion Refactor (WIP) #70602

Double to Decimal Conversion Refactor (WIP) #70602

dakersnar commented Jun 11, 2022 •

edited

Loading

ghost commented Jun 11, 2022

Problems with previous code

Summary of approach

Status of the work

What is left to do

dakersnar Jun 11, 2022

dakersnar Jun 11, 2022

dakersnar Jun 11, 2022

dakersnar Jun 11, 2022

dakersnar Jun 11, 2022

AntonLapounov Jul 14, 2022

dakersnar Jun 11, 2022

dakersnar Jun 11, 2022

dakersnar Jun 11, 2022

dakersnar Jun 11, 2022

dakersnar Jun 11, 2022

dakersnar commented Jul 13, 2022

	// -2^96 <; m <; 2^96, and e is an integer
	// -2^96 <= m <= 2^96, and e is an integer

Double to Decimal Conversion Refactor (WIP) #70602

Double to Decimal Conversion Refactor (WIP) #70602

Conversation

dakersnar commented Jun 11, 2022 • edited Loading

Problems with previous code

Summary of approach

Status of the work

What is left to do

ghost commented Jun 11, 2022

Problems with previous code

Summary of approach

Status of the work

What is left to do

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

Choose a reason for hiding this comment

dakersnar commented Jul 13, 2022

dakersnar commented Jun 11, 2022 •

edited

Loading