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

Convert Decimal to all-managed implementation #10437

Closed
danmoseley opened this issue Jun 2, 2018 · 5 comments · Fixed by dotnet/coreclr#18948
Closed

Convert Decimal to all-managed implementation #10437

danmoseley opened this issue Jun 2, 2018 · 5 comments · Fixed by dotnet/coreclr#18948
Assignees
Labels
area-Meta Hackathon Issues picked for Hackathon

Comments

@danmoseley
Copy link
Member

CoreCLR's is in
https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/src/System/Decimal.cs
https://github.com/dotnet/coreclr/blob/master/src/classlibnative/bcltype/decimal.cpp

The place to start is by grabbing the code verbatim from CoreRT and measuring performance, then optimizing to fix any regression:

https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Decimal.cs
https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Decimal.DecCalc.cs

Ensuring tests have good coverage, of course.

@danmoseley
Copy link
Member Author

Once this is done the resulting implementation should move to System.Private.CoreLib/shared/System/Decimal.cs and both CoreCLR and CoreRT will use it.

@pentp
Copy link
Contributor

pentp commented Jun 2, 2018

dotnet/corert#4452 and dotnet/corert#4997 have most of the performance work done and the remaining performance regressions affect only x86 (everything else is already faster than the current native implementation). The following table has the remaining methods that need work, in particular, ADD and MUL are significantly slower.
(updated - FromDouble, HashCode and DIV removed)

32-bit x86

ADD Mean Error StdDev Scaled
Native 28.43 ns 0.0251 ns 0.0090 ns 1.00
CoreRT 64.05 ns 0.3440 ns 0.1227 ns 2.25
CoreRT2 35.95 ns 0.0438 ns 0.0156 ns 1.26
MUL Mean Error StdDev Scaled
Native 19.23 ns 0.0267 ns 0.0069 ns 1.00
CoreRT 47.70 ns 0.3100 ns 0.1106 ns 2.48
CoreRT2 28.09 ns 0.0383 ns 0.0137 ns 1.46
Negate Mean Error StdDev Scaled
Native 1.744 ns 0.0029 ns 0.0007 ns 1.00
CoreRT 7.583 ns 0.0181 ns 0.0065 ns 4.35
CoreRT2 1.855 ns 0.0012 ns 0.0003 ns 1.06

@pentp
Copy link
Contributor

pentp commented Jun 2, 2018

Test coverage should be pretty exhaustive already from dotnet/corefx#24053 and dotnet/corefx#26338.

@jkotas
Copy link
Member

jkotas commented Jun 2, 2018

Also, compat issue with the CoreRT implementation needs to be addressed before this can land in CoreCLR: dotnet/corert#4994

@pentp
Copy link
Contributor

pentp commented Jun 2, 2018

I'll start working on making decimal a readonly struct in corert so that dotnet/corert#4994 can be resolved before decimal is shared with CoreCRL.

@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta Hackathon Issues picked for Hackathon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants