Added C# implementation of System.Math.ScaleB and System.MathF.ScaleB… by alexcovington · Pull Request #42476 · dotnet/runtime

alexcovington · 2020-09-18T21:56:42Z

Implemented System.Math.ScaleB and System.MathF.ScaleB in managed C#, added test cases.

Submitting as a draft pull request first because I'm not quite sure where/how to integrate the tests I've written. System.Private.CoreLib does not have a test directory and I'm having trouble getting build.cmd to find the tests. Any feedback would be appreciated!

This is to address this issue regarding a performance difference in System.Math.ScaleB and System.MathF.ScaleB on Windows and Linux. This PR improves the speed of the function on both platforms and removes the previous bindings/unmanaged implementations.

Edit: updated description and added link to issue.

…, removed old bindings, added test cases.

dnfadmin · 2020-09-18T21:56:59Z

All CLA requirements met.

am11 · 2020-09-18T22:43:44Z

Existing tests are based on xunit framework and located at src/libraries/System.Runtime.Extensions/tests/System/Math{F}.cs.

From PowerShell, you can do:

# build the whole thing just once
$ cd runtime
$ ./build.cmd -c Release

# then switch to the 'test' directory of library of interest, in this case
$ cd src/libraries/System.Runtime.Extensions/tests/

# build and run tests using restored toolset
$ ../../../../.dotnet/dotnet build /t:Test -c Release
# at this point, can modify code in src/libraries/System.Private.CoreLib/src/System and just repeat the command
# on line above to rebuild src+test (and their dependencies) for this library and then run tests.

jkotas · 2020-09-18T22:59:00Z

src/coreclr/src/System.Private.CoreLib/src/System/Math.CoreCLR.cs

        public static extern double Pow(double x, double y);

-        [MethodImpl(MethodImplOptions.InternalCall)]
-        public static extern double ScaleB(double x, int n);


Could you please delete it from the unmanaged runtime:

src/coreclr/src/classlibnative/float/floatdouble.cpp
src/coreclr/src/classlibnative/float/floatsingle.cpp
src/coreclr/src/classlibnative/inc/floatdouble.h
src/coreclr/src/classlibnative/inc/floatsingle.h
src/coreclr/src/vm/ecalllist.h
Everything that references scalbn and scalbnf under src/coreclr/src/pal

Also, I think these lines can be deleted from mono:

runtime/src/mono/mono/metadata/icall-def-netcore.h

Line 148 in 2a1735b

NOHANDLES(ICALL(MATH_25, "ScaleB", ves_icall_System_Math_ScaleB))

runtime/src/mono/mono/metadata/icall-def-netcore.h

Line 177 in 2a1735b

NOHANDLES(ICALL(MATHF_27, "ScaleB", ves_icall_System_MathF_ScaleB))

@am11 icall-decl.h should be preserved, it's used by 6.x mono

Ah, ok I see. We can then just delete two lines in icall-def-netcore.h and leave icall-decl.h and sysmath.c. Updated my comment above.

ghost · 2020-09-18T23:19:05Z

Tagging subscribers to this area: @tannergooding, @pgovind, @jeffhandley
See info in area-owners.md if you want to be subscribed.

hughbe · 2020-09-19T11:18:36Z

src/libraries/System.Private.CoreLib/tests/math/MathFTests.cs

+{
+    class MathTests
+    {
+        public struct ScaleBTest


Why are we not using xunit tests in System.Runtime.Tests?

Saw your comment above - take a look at https://github.com/dotnet/runtime/blob/master/src/libraries/System.Runtime.Extensions/tests/System/MathF.cs

This is exactly what I was looking for before! I just missed it when I was looking for current tests. I'll just add my new test cases to the existing ScaleB tests.

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

Co-authored-by: Adeel Mujahid <adeelbm@outlook.com>

vargaz · 2020-09-23T00:09:45Z

The mono changes look ok to me.

CoffeeFlux · 2020-09-23T13:50:13Z

For Mono, please wrap the corresponding icall implementations in #ifndef ENABLE_NETCORE. We want them around for framework Mono but not netcore.

runtime/src/mono/mono/metadata/sysmath.c

Line 222 in 1905bde

ves_icall_System_Math_ScaleB (gdouble x, gint32 n)

jkotas · 2020-09-23T17:36:18Z

src/coreclr/src/pal/inc/pal.h

 #define log2          PAL_log2
 #define log10         PAL_log10
 #define pow           PAL_pow
-#define scalbn        PAL_scalbn


Could you please also delete src\coreclr\src\pal\tests\palsuite\c_runtime\scalbn and src\coreclr\src\pal\tests\palsuite\c_runtime\scalbnf

src/mono/mono/metadata/sysmath.c

danmoseley · 2020-10-01T20:26:15Z

What's next steps here?
cc @tannergooding

tannergooding · 2020-10-01T20:52:05Z

What's next steps here?

I'll get this reviewed either today or tomorrow. I'm not aware of anything else that would be blocking.

It would likely be good to get perf numbers for x64, x86, ARM32, and ARM64. @DrewScoggins, is there currently an easy way to get these for a particular benchmark (aside from manually running them)?

tannergooding · 2020-10-01T20:57:01Z

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

            throw new ArgumentException(SR.Format(SR.Argument_MinMaxValue, min, max));
        }
+
+        public static double ScaleB(double x, int n)


Was this ported from an existing implementation?

Based on #40956 (comment), it sounds like its based on the MUSL implementation which is MIT (https://git.musl-libc.org/cgit/musl/tree/COPYRIGHT) and so we need a comment around here and also need to update the https://github.com/dotnet/runtime/blob/master/THIRD-PARTY-NOTICES.TXT.

CC. @richlander on if there is anything else required.

The MUSL implementation came from Sun's implementation, so we may need the notice for that too (or just have notice for the Sun's implementation if it was not changed for MUSL).

Yes, this was based on the musl implementation. I added links to the source for each function and included both the musl and Sun license. Let me know if I missed or can add anything.

tannergooding · 2020-10-01T21:07:38Z

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

+        public static double ScaleB(double x, int n)
+        {
+            double y = x;
+            if (n > 1023)


Its probably not particularly important here, but the MSVC and the JIT tends to emit (assuming no PGO data):

if (condition) { // predicted } else { // unpredicted }

This is inverse from what Clang/GCC do, so the n > 1023 and n < -1022 cases are going to be predicted as taken by default, while I believe they are edge cases in practice.

You can compare JIT (noting this is 3.1 output, not .NET 5, so there may be a couple minor differences): https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHuGAHKAJYA3bBhgNIAO1wYGAEwgBXYABtxAZRYBBADIBRAEIB9FuQYBeBgA4AdAE4rVlEgCsSAOwo65ci6t6AajoABncAbgYAekiGYIRyXnJg0jouHn5hUXEpGXklVQ1tfWMWUgsGUhtSUhdQuis/WvdSYPIUPTgQqwjo2PjeOCTqtO4MkTEJCGlZBWU1Bk1dQxNGSztg0PI7OxqUT2Dt0h6YuISXVOoRpkZiciQ8uY0wbDUDAApZgoYENAYBSVkkgAlFdONQeBCHl8AJ7lBBhK4QgQAMwYb0kDAAfAwhnQQeDIYSwYTCbCAFSWRbFEzkBEEkk8DFwSy4ukMpGo9FYnHJPGIknE9mkhgUhZFZamNlCyFMlm8qXSngotEY7G4/GKiGCzUy8qs/lCgC+BshxvpppNMBUuHEyq5AB4GINkqQNQKTRDyZTxSUymSxUsSnQFeyMQE5dUnQxziGGXaMY7ndU3ULtZqvQHqaURZmJcGPSSwxGynBo/nzYr4wxE0NXQWifXQ+Uk0dGxCzYqOySzSbPvNFOUDAIMCwpkIYFAxFAbABJAGoIcYXAAFQgABF8mo3tuVFMAOZAt5xOjI1EBBjA6uOlyuoGxnjEdwMckMRQhs2GoA

To MSVC and Clang: https://godbolt.org/z/MEYGcz

This is good to know. I tried reworking the logic to take this into consideration, roughly something like:

if (n <= 127 && n >= -126) { // n is in expected range, just compute and return result } else { if (n > 127) { // Make adjustments } else if (n < -126) { // Make adjustments } }

But I'm not seeing an improvement in perf. I can push if you'd like to take a look, but not sure it's worth it.

I can push if you'd like to take a look, but not sure it's worth it.

Right, I don't think in this particular case it is particular import, I just wanted to call it out as it might be relevant if additional work is done in this or related areas later.

I was thinking that if those edge cases were moved into their own method, this could become inlineable. But it doesn't seem to get inlined even then.

tannergooding · 2020-10-01T21:09:37Z

src/libraries/System.Runtime.Extensions/tests/System/MathF.cs

-        [InlineData( 2.71828183f,            2.71828183f,            CrossPlatformMachineEpsilon * 10)]     // value:  (e)              expected: (e)
-        [InlineData( 3.14159265f,            3.14159265f,            CrossPlatformMachineEpsilon * 10)]     // value:  (pi)             expected: (pi)
-        [InlineData( float.PositiveInfinity, float.PositiveInfinity, 0.0f)]
+        [InlineData(float.NegativeInfinity, float.PositiveInfinity, 0.0f)]


It looks like the diff for this file is bigger because the whitespace was stripped from the attributes.

It's probably a losing battle to try and fight with the formatter to keep the "columns" aligned, so I don't think it needs to be fixed; just wanted to call it out.

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

DrewScoggins · 2020-10-02T17:46:35Z

What's next steps here?

I'll get this reviewed either today or tomorrow. I'm not aware of anything else that would be blocking.

It would likely be good to get perf numbers for x64, x86, ARM32, and ARM64. @DrewScoggins, is there currently an easy way to get these for a particular benchmark (aside from manually running them)?

We already generate reports for the category subsets that are not Library and Runtime, but looking at the performance tests for this space, they are not in their own category. So what we can do is just generate a report that covers just the performance tests that we care about by name. We won't have this data for ARM32, as we don't have that hardware in the perf lab, but we should be able to generate the data for all of the other configs. Also this would be after the change already went in.

tannergooding · 2020-10-02T17:56:47Z

Also this would be after the change already went in.

Right, I know (roughly) how to do that bit. I was interested if we had any way to kick of a perf run against the current PR and to pull that info out before merging, short of just checking it out and running the tests locally on my own hardware.

ViktorHofer · 2020-12-08T14:52:58Z

// Auto-generated message

69e114c which was merged 12/7 removed the intermediate src/coreclr/src/ folder. This PR needs to be updated as it touches files in that directory which causes conflicts.

To update your commits you can use this bash script: https://gist.github.com/ViktorHofer/6d24f62abdcddb518b4966ead5ef3783. Feel free to use the comment section of the gist to improve the script for others.

tannergooding · 2021-01-21T21:12:32Z

Is this currently blocked on anything and if so, can we help push it forward?

alexcovington · 2021-01-21T21:14:47Z

@tannergooding My apologies, I think this slipped through the cracks on my end. I'll merge with master and push again.

tannergooding · 2021-01-21T21:16:43Z

Thanks, and no worries! Just wanted to check in and see (and make sure I wasn't missing some action item on my end) 😄

…idn't resolve

tannergooding · 2021-01-25T18:33:40Z

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

            return x;
        }
+
+        public static float ScaleB(float x, int n)


Just noting, since this PR was opened, AMD open sourced their own libm implementation which includes scaleb: https://github.com/amd/aocl-libm-ose/blob/7193ff6fdf72ecd68df4e0e19262da84f1d95a3e/src/ref/scalbnf.c

It notably has a different algorithm and error handling logic (unnecessary in the C# implementation) but it might be worth comparing in the future to see which is better.

tannergooding

LGTM. This should be good to merge assuming @jkotas doesn't have any other feedback about the JIT side changes.

jkotas

LGTM. Thank you!

ghost · 2021-01-25T18:48:01Z

Hello @tannergooding!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

tannergooding · 2021-01-25T22:15:26Z

Thanks again for the contribution @alexcovington!

@adamsitnik, @kunalspathak, @DrewScoggins we should validate this shows up as an improvement in perf triage.

src/mono/mono/metadata/sysmath.c

DrewScoggins · 2021-02-02T17:36:58Z

Here is the improvement on Ubuntu

DrewScoggins/performance-2#3838

Windows x64

DrewScoggins/performance-2#3857

Arm64

DrewScoggins/performance-2#3848

Windows x86

DrewScoggins/performance-2#3867

Added C# implementation of System.Math.ScaleB and System.MathF.ScaleB…

25ce953

…, removed old bindings, added test cases.

Dotnet-GitSync-Bot added the area-Infrastructure-coreclr label Sep 18, 2020

tannergooding added area-System.Numerics and removed area-Infrastructure-coreclr labels Sep 18, 2020

jkotas reviewed Sep 18, 2020

View reviewed changes

hughbe reviewed Sep 19, 2020

View reviewed changes

alexcovington added 2 commits September 22, 2020 09:18

Moved tests into System.Runtime.Extensions tests.

7f47129

Removed unmanaged scalbn and scalbnf from pal and mono

57c4931

alexcovington marked this pull request as ready for review September 22, 2020 21:03

alexcovington requested review from CoffeeFlux, akoeplinger, lambdageek, marek-safar, steveisok, thaystg and vargaz as code owners September 22, 2020 21:03

monojenkins mentioned this pull request Sep 22, 2020

Added C# implementation of System.Math.ScaleB and System.MathF.ScaleB… mono/mono#20424

Merged

am11 reviewed Sep 22, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Math.cs Outdated Show resolved Hide resolved

am11 reviewed Sep 22, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Math.cs Show resolved Hide resolved

am11 reviewed Sep 22, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Math.cs Show resolved Hide resolved

alexcovington and others added 2 commits September 22, 2020 15:52

Update src/libraries/System.Private.CoreLib/src/System/Math.cs

0525bcd

Co-authored-by: Adeel Mujahid <adeelbm@outlook.com>

Styling changes.

dadcf12

Conditionally include ScaleB for Mono.

cfc46a3

jkotas reviewed Sep 23, 2020

View reviewed changes

am11 reviewed Sep 23, 2020

View reviewed changes

src/mono/mono/metadata/sysmath.c Outdated Show resolved Hide resolved

tannergooding reviewed Oct 1, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Math.cs Show resolved Hide resolved

Added comments for ScaleB, updated licenses for musl and Sun

bfb30e0

Fixed whitespace.

594b184

This was referenced Oct 2, 2020

OSX deprovision jaredpar/runfo#41

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

jeffhandley assigned tannergooding Dec 7, 2020

alexcovington added 2 commits January 21, 2021 13:51

Resolving merge conflicts with master

824cc07

Removing old references, fixing double ScaleB definition that merge d…

47c3633

…idn't resolve

tannergooding reviewed Jan 25, 2021

View reviewed changes

tannergooding approved these changes Jan 25, 2021

View reviewed changes

jkotas approved these changes Jan 25, 2021

View reviewed changes

tannergooding added the auto-merge label Jan 25, 2021

ghost merged commit 220bf97 into dotnet:master Jan 25, 2021

am11 reviewed Jan 25, 2021

View reviewed changes

src/mono/mono/metadata/sysmath.c Show resolved Hide resolved

adamsitnik mentioned this pull request Jan 26, 2021

MathF.ScaleB performance in Windows 10 much worse than Ubuntu 20.04 #40956

Closed

ghost locked as resolved and limited conversation to collaborators Mar 4, 2021

This pull request was closed.

Conversation

alexcovington commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfadmin commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

am11 commented Sep 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

am11 Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vargaz commented Sep 23, 2020

Uh oh!

CoffeeFlux commented Sep 23, 2020

Uh oh!

jkotas Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danmoseley commented Oct 1, 2020

Uh oh!

tannergooding commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrewScoggins commented Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannergooding commented Oct 2, 2020

Uh oh!

ViktorHofer commented Dec 8, 2020

Uh oh!

tannergooding commented Jan 21, 2021

Uh oh!

alexcovington commented Jan 21, 2021

Uh oh!

tannergooding commented Jan 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannergooding left a comment

Choose a reason for hiding this comment

Uh oh!

jkotas left a comment

Choose a reason for hiding this comment

alexcovington commented Sep 18, 2020 •

edited

Loading

dnfadmin commented Sep 18, 2020 •

edited

Loading

am11 Sep 18, 2020 •

edited

Loading

jkotas Sep 23, 2020 •

edited

Loading

DrewScoggins commented Oct 2, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.