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

The new System.MathF APIs should have wrappers created so they are available on .NETStandard #20113

Closed
tannergooding opened this issue Feb 5, 2017 · 33 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Numerics
Milestone

Comments

@tannergooding
Copy link
Member

The new System.MathF APIs (#1151) where implemented in dotnet/coreclr#5492. However, they are currently only available in netcoreapp1.1 base applications.

Until they make their way back into the Desktop framework, I propose that simple wrapper implementations be provided so the APIs are accessible in .NETStandard based libraries.

The wrapper function should just call the corresponding System.Math API and cast the result back to System.Single.

For example, System.Acos(float) would be implemented as:

namespace System
{
    public static class MathF
    {
        public static float Acos(float value)
        {
            return (float)Math.Acos(value);
        }
    }
}
@karelz
Copy link
Member

karelz commented Feb 6, 2017

Do you propose to include the in .NET Standard, or build a package which is on top of .NET Standard?
We have to think carefully through the implications. These things seem easy, but have unexpected implications in future releases and might limit our ability to include the APIs in .NET Standard in future.

@tannergooding
Copy link
Member Author

I was requesting that they be included in .NET Standard, presumably as part of System.Runtime.Extensions?

Are you indicating that this might not be possible due to any loss of precision that would occur when the runtime support is merged back into the Desktop framework?

@karelz
Copy link
Member

karelz commented Feb 6, 2017

Including them in the standard, before they are truly available on Desktop, may be very hard to do if Desktop just forwards the types into Desktop. We have burned ourselves in the area recently (System.Net.Http), so I would like to be extra careful before we do anything like that.
cc @weshaggard @ericstj

What is the value? How many customers are waiting for the APIs? Is there a reasonable workaround for them?

@tannergooding
Copy link
Member Author

Understood.

The value comes from being able to consume the System.MathF APIs everywhere and through a single reference, without needing any kind of workarounds.

I am unsure how many customers are waiting on the APIs.

The workaround if users wan't to consume the APIs today, and they want to target more than just .netcoreapp1.1, then they need to create their own class that wraps the functionality (calling System.MathF on netcore and calling into System.Math and casting the result for Desktop).

@karelz
Copy link
Member

karelz commented Feb 6, 2017

Given the fact we could not even provide the full functionality (without down-casting), I think the workaround is reasonable place to be in.

@tannergooding
Copy link
Member Author

@karelz, what is the answer going to be moving forward for APIs added that require runtime support but have reasonable workarounds to also get it functioning on Desktop?

We did something very similar with SIMD, where platforms that either don't support the hardware intrinsics or don't have runtime support yet would just fallback to using a slower and possibly less precise implementation.

@karelz
Copy link
Member

karelz commented Feb 9, 2017

Right now we do not have plans. We will adjust them based on customer demand.
cc: @danmosemsft @weshaggard

@tannergooding
Copy link
Member Author

My opinion here is that we should be providing the surface area on all platforms, whether it is available or not. This allows users to use APIs, no matter what framework they are targeting. If the API is not available, it will throw and the user can handle it as they desire. My understanding is that this is what is being done for a number of APIs in .NETStandard 2.0.

Additionally, for APIs which have a reasonable workaround (such as emulating the functionality, as we did with System.Numerics.Vectors; or by calling the double-precision functions and downcasting, as we can do here) then I think it that is much more reasonable to implement the workaround than it is to tell customers that they cannot use the APIs on these other platforms until the runtime support gets merged in (which could take an indeterminate amount of time).

@karelz
Copy link
Member

karelz commented Feb 9, 2017

Compiler errors are much easier for consumers to deal with than runtime errors. With runtime errors you are never sure when you're done, until you exercise every single code path in your app/library (which is sci-fi).

Yes, we did that for a bunch of APIs, but we tried very hard to minimize number of such cases and minimize the impact (i.e. based on usage data from nuget). Having it as default policy would be pretty bad.
If we see customer demand (incl. one off workaround by customers), we will tackle the problem. Right now I don't see the demand yet.

@weshaggard
Copy link
Member

If the API is not available, it will throw and the user can handle it as they desire. My understanding is that this is what is being done for a number of APIs in .NETStandard 2.0.

That is the exceptional case that we are dealing with there for the initial pull together of the .NET Standard 2.0 work. Going forward we don't yet have the full plan worked out (see dotnet/standard#127) but we will have a process for adding things to the standard and platforms that don't support that version of the standard will not be able to use libraries that build against that standard version.

While I think the MathF functions are a potential addition to a future version of .NET Standard, I don't think they should be included in 2.0. However being added to the standard and being supported everywhere are two different things. For new things that are independent we can produce a OOB library package that would provide an implementation for other platforms until it makes it to that platform (see System.ValueTuple as an example), but we cannot easily expose new APIs in assembly identities that are already inbox on a given platform because that causes various unification issues (System.Net.Http is an example of this).

So given that we added these MathF functions into System.Runtime.Extensions which does already exist as an assembly identity in the .NET Framework I would suggest we not provide an implementation for these there yet. It will cause a pretty large set of complex assembly binding issues for minimal value.

@mellinoe
Copy link
Contributor

mellinoe commented Feb 9, 2017

Another option: we can move this class to a new extension library, entirely on top of the standard. It can have a specialized implementation for .NET Core 2.0 which type-forwards to System.Private.CoreLib.dll, but is otherwise fully portable.

@weshaggard
Copy link
Member

Yes that is an option if we felt there was enough value for having it but if the portable implementation is simply going to cast I don't know if there is really enough value.

@jkotas
Copy link
Member

jkotas commented Feb 9, 2017

Agree with @weshaggard . We invest into building and shipping the extension library if the types involved are exchange types, thus they are likely to be used in public signatures because of it has ripple effects. MathF is not an exchange type.

Note that this does not prevent to anybody out there who feels strongly about this to build the portable extension library as their own nuget package. It does not have to be done by corefx team and live in corefx repo. There are actually quite a few nuget packages like that supported by the .NET community, e.g. that make the newer types available on top of .NET Framework 2.0.

@tannergooding
Copy link
Member Author

The benefit here is that users can consume the API without needing to target any additional/special packages (or even 3rd party) packages. Then, when the runtime support is finally merged back to Desktop, they get a decent performance increase.

Currently, one cannot reference NETStandard.Library and consume the System.MathF APIs without creating a custom wrapper/workaround. This limits adoption of something that will eventually be available everywhere and that has a reasonably simple workaround to make it available everywhere today.

@weshaggard
Copy link
Member

weshaggard commented Feb 9, 2017

Currently, one cannot reference NETStandard.Library and consume the System.MathF APIs without creating a custom wrapper/workaround. This limits adoption of something that will eventually be available everywhere and that has a reasonably simple workaround to make it available everywhere today.

While this is unfortunate it is the state of the world we live in. We just did a lot of work to decouple .NET Core from .NET Standard in order to enable folks to have freedom to be able to add new APIs much quicker for .NET Core then for the Standard. If we go back to the old model, which I don't believe anyone wants, to tie these together again then it will result in folks not being able to innovate or add new APIs without a ton more work.

Pretty much all the new APIs we are adding are going to be added to .NET Core first and get some vetting as .NET Core only APIs and then later on get added to .NET Standard and .NET Framework.

@mellinoe
Copy link
Contributor

mellinoe commented Mar 3, 2017

I don't really agree with some of the points above. MathF is a completely standalone type, with no technical restrictions barring it from being shipped by itself, in a library targeting .NET Standard 1.0. It was only put in System.Runtime.Extensions for the sake of expediency. I think @tannergooding would have preferred to do it another way if we had given him different guidance when the library was added.

Pretty much all the new APIs we are adding are going to be added to .NET Core first and get some vetting as .NET Core only APIs and then later on get added to .NET Standard and .NET Framework.

I don't think that's true. I would much rather see us building .NET Standard libraries where possible, rather than .NET Core libraries, if there is no technical reason for a preference either way. Note that I'm talking about libraries that TARGET the standard, not those that are a part of them. I'm also not saying that we wouldn't reserve the ability to ship a special implementation for .NET Core (like the case is here), just that it makes more sense to default to a ".NET Standard library" rather than a ".NET Core library". For example, if we were writing System.Collections.Immutable today, I find it hard to believe that we would target netcoreapp rather than netstandard.

@karelz
Copy link
Member

karelz commented Mar 3, 2017

I think it is perfectly fine to ship libraries targeting the LATEST .NET Standard from CoreFX. However, shipping libraries targeting older Standards is pain - we don't have any test infra for those. It is not impossible, but very costly (infra and also the package matrix).
Also, by adding 'every new type' in each release into a new library in CoreFX will soon get out of hand on complexity.

My suggestion would be: If there is strong community desire to ship any part of CoreFX targeting older .NET Standard version, let those who are interested and willing to pay the cost do that in a fork (the MIT license allows it AFAIK). If we see strong uptake on some of such packages, we can always change our direction and help streamline that from CoreFX repo.
However, it is not cost/benefit-desirable to do it as the first thing. The added complexity and cost is just way too high and we have much higher priorities in CoreFX (landing .NET Standard 2.0 on ALL platforms, improving x-plat, improving performance everywhere, etc.).

@tannergooding
Copy link
Member Author

Now that .NET Standard 2.0 is closer to landing, is this something that can be revisted (as a NetStandard 2.1 API)?

CoreFX has already created a simple wrapper for MathF on NetStandard for internal use as it simplifies the code in a number of places (and allows it to be faster on NetCore).

My reason for wanting this comes down to the same, it simplifies code greatly for users who are going to be consuming System.MathF anyways.

@karelz
Copy link
Member

karelz commented Jul 7, 2017

It is too early to discuss .NET Standard 2.1. Why do you think it needs to be part of .NET Standard. Isn't the wrapper a viable solution? (not pushing back, just trying to understand)

@tannergooding
Copy link
Member Author

The current CoreFX wrapper is internal and not available for general use (currently System.Numerics.Vector is the only consumer).

I want System.MathF to be part of NETStandard so that I can use System.MathF in my code (without having to write my own wrapper) and have it work on both Desktop (where it wraps System.Math) and CoreCLR (where it resolves down to an FCALL).

Writing my own wrapper is viable, but really shouldn't be necessary for something that will eventually be part of Desktop anyways.

@tannergooding
Copy link
Member Author

tannergooding commented Jul 7, 2017

Additionally, a wrapper requires you to cross-compile (one for Desktop that wraps and one for CoreCLR that doesn't). This makes it difficult to consume System.MathF from within a NetStandard library itself.

If I write a NetStandard library and want to consume System.MathF I have to:

  • Create a separate project that cross-compiles (Desktop and CoreCLR)
  • Create my own wrapper for System.MathF
  • Have that wrapper call System.Math on Desktop and System.MathF on CoreCLR
  • Consume my wrapper everywhere (thus adding a forced layer of indirection)

I then additionally have to update my wrapper if/when Desktop gets System.MathF itself (thus requiring me to patch or ship a new version of my library).

If it is part of NetStandard then:

  • NetStandard will ship it as part of the reference contract for System.Runtime.Extensions (as it already does, but for both Desktop and CoreCLR rather than just for CoreCLR)
  • I can call System.MathF directly
    • On Desktop, this will wrap System.Math
    • On CoreCLR, this will be a direct FCALL

Then, if/when Desktop gets System.MathF simply running on the newer version of the desktop framework should result in me getting better perf (users can recompile their code or modify their app config and I don't need to do anything).

@ashoulson
Copy link

ashoulson commented Jan 11, 2018

Would this also include the BitConverter.Int32BitsToSingle and BitConverter.SingleToInt32Bits functions? It would be very handy to have these for serialization in NetStandard environments. Particularly for network implementations that have NetCore talking with NetFramework.

@tannergooding
Copy link
Member Author

If System.MathF had a wrapper for .NET Standard, then BitConverter.Int32BitsToSingle and its counterpart would need to be included as well (as they are required by System.MathF).

@ghost
Copy link

ghost commented Jan 30, 2018

I think it will be easier to add an overloaded version for each method in the Math Class with float parameters, instead of having new MathF Class.

@tannergooding
Copy link
Member Author

Commented here on why that is not possible.

because it would be a breaking change for recompiled code.

Math.Sqrt(4) today resolves to Math.Sqrt(double) (because an implicit conversion to double exists, and the only overload is for double). However, if you add a new Math.Sqrt(float) overload, overload resolution comes into play. int can be implicitly converted to either float or double, but float is preferred, so the recompiled code would call Math.Sqrt(float), which can cause an observable difference in results for certain inputs.

@svick
Copy link
Contributor

svick commented Jan 30, 2018

@tannergooding

which can cause an observable difference in results for certain inputs

It's worse than that, it can cause recompilation to fail (or do something entirely different). Consider e.g.:

var x = Math.Sqrt(42);
x = 3.14;

If you recompile this with float Math.Sqrt(float) added, the type of x will change from double to float, which means the second line will no longer compile (CS0664 Literal of type double cannot be implicitly converted to type 'float'). I think that would be much worse breaking change than losing precision for some computations.

@ghost
Copy link

ghost commented Jan 30, 2018

Got it, put there is another way:
Math.SqrtF(x) instead of MathF.Sqrt(x)
Math is a static class. This will not affect any thing.

@tannergooding
Copy link
Member Author

It was decided at the time to use MathF.

C# has a feature called "using statics", which allows you to do using static System.Math; using static System.MathF;

You can then invoke the methods via Sqrt(value) and overload resolution will pick the correct one (this is not breaking since it requires users to explicitly opt-in to the behavior).

@ghost
Copy link

ghost commented Jan 31, 2018

Still, I knew about MathF just today by accident, and still don't t know where it is and how to add it to my project. If it were implemented as a part of the Math class, all of us would know and use it now by default.
So, where is it?

@tannergooding
Copy link
Member Author

It is only available in netcoreapp2.0 or higher.

It should be "just available" if you create a netcoreapp2.0 project.

@krisrok
Copy link

krisrok commented May 28, 2018

meh, still nothing for netstandard2.0 ?

@tannergooding
Copy link
Member Author

These are up for .NETStandard VNext: dotnet/standard#823

@tannergooding
Copy link
Member Author

Going to close for now, will reopen if the dotnet/standard story here changes.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Numerics
Projects
None yet
Development

No branches or pull requests

9 participants