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

Updating the 'System.Runtime.Extensions' contracts to include the new single-precision Math APIs. #12183

Merged
merged 4 commits into from Oct 26, 2016

Conversation

Projects
None yet
6 participants
@tannergooding
Copy link
Member

tannergooding commented Sep 29, 2016

This updates the System.Runtime.Extensions contracts to include the new single-precision Math APIs approved in #1151 and implemented in CoreCLR in dotnet/coreclr#5492.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Sep 29, 2016

Unit Tests for the System.MathF class still need to be written (I am currently working on porting the existing System.Math tests).

@hughbe

This comment has been minimized.

Copy link
Collaborator

hughbe commented Sep 29, 2016

Have these APIs only been approved for .net core?
If so, you'll need to expose these new APIs under netcoreapp1.1, see #12036

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Sep 29, 2016

@terrajobst, could you clarify on which TFMs the new APIs have been approved for?

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Oct 10, 2016

Yes, these APIs were approved for .NET Core in #1151.
@mellinoe can provide additional guidance/help if needed.

@mellinoe

This comment has been minimized.

Copy link
Contributor

mellinoe commented Oct 10, 2016

We can only add this to netcoreapp, so you'll need to follow other recent PR's that have done that, and/or refer to the document @weshaggard is working on above.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 10, 2016

@mellinoe, the document above is somewhat confusing, as it says any change that is not part of netstandard but that depends directly on a runtime change, then the target framework should be netstandard<latest>.

It seems like these changes have an actual dependency on the Microsoft.TargetingPack.Private.CoreCLR, based on the their being an actual runtime dependency with FCALLs being added.

Is this not the case?

@mellinoe

This comment has been minimized.

Copy link
Contributor

mellinoe commented Oct 10, 2016

@weshaggard Could you clarify that point a bit? These are new API's for .NET Core, which rely on new FCALL's.

@weshaggard

This comment has been minimized.

Copy link
Member

weshaggard commented Oct 10, 2016

Thanks for the feedback on the documentation. I will try and incorporate and update once I can get back to that. As @mellinoe points out though these are definitely in the bucket of netstandard APIs they just aren't part of any standard yet and as such they need to be added as .NET Core specific for now.

In the mean time I suggest using #12226 as an example for how to add APIs to .NET Core specifically.

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Oct 15, 2016

@tannergooding was the info from @weshaggard helpful? Anything else we can help with here?

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 15, 2016

@karelz, yes it was. I have just had my focus elsewhere due to the current schedule. I am going to take some time today to fix up this and the other PR.

@tannergooding tannergooding force-pushed the tannergooding:math branch from 0cd99a4 to 3289957 Oct 18, 2016

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 18, 2016

Pretty sure I've updated these all appropriately now.

System.MathF tests are still blocked on getting the CoreCLR side merged so that we can get a Microsoft.TargetingPack.Private.CoreCLR package with the required changes.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 18, 2016

@mellinoe, the CoreCLR side changes have been merged.

About how long does it take before a Microsoft.TargetingPack.Private.CoreCLR is available for me to update everything to (and is there an automated/documented process for moving that forward)?

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 18, 2016

Actually, it looks like @dotnet-bot does automated PRs at least once a day. I suppose I will need to wait for the next one (after the current one is merged) to complete.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 18, 2016

Looks like it will be one more PR. The one that just came up (#12756) is a commit before my changes 😄

@tannergooding tannergooding force-pushed the tannergooding:math branch from 3289957 to 8dbcd0d Oct 20, 2016

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 20, 2016

Rebased onto HEAD. Going to start writing the System.MathF tests now that the contract assembly is available.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 20, 2016

I've ported the existing System.Math unit tests for the new System.MathF APIs.

The coverage that exists for System.Math is surprisingly small. I think I'm going to open an issue for expanding it in a separate PR. I would think, at the very least, we could replicate the test coverage we have over the native implementations in the PAL layer (this correlates to roughly 20 tests per API, at a minimum, and ensures we cover the interesting values as well).

@mellinoe

This comment has been minimized.

Copy link
Contributor

mellinoe commented Oct 20, 2016

As a general note, it would be nice to get the test cases factored out into [Theory]s. Better logging and better separation of individual issues, if they occur.

@mellinoe
Copy link
Contributor

mellinoe left a comment

Changes look good overall; I made a few small comments on the tests. Thanks for all of the work you've done on this, I'm looking forward to using it when we get it through 😄

[Fact]
public static void Abs()
{
AssertEqual(MathF.PI, MathF.Abs(MathF.PI));

This comment has been minimized.

Copy link
@mellinoe

mellinoe Oct 20, 2016

Contributor

I don't see this two-parameter AssertEquals method anywhere. Am I missing something?

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 20, 2016

Author Member

This was a mistake on my side, the calls that do not have an expected precision (and should be exact values) should still call Assert.Equal.

// This is essentially equivalent to the Xunit.Assert.Equal(double, double, int) implementation
// available here: https://github.com/xunit/xunit/blob/2.1/src/xunit.assert/Asserts/EqualityAsserts.cs#L46

var expectedRounded = MathF.Round(expected, precision);

This comment has been minimized.

Copy link
@mellinoe

mellinoe Oct 20, 2016

Contributor

The use of a MathF function here raises a flag for me, since this is a core assertion used by the rest of the tests. It would be nice if the MathF.Round tests didn't use this, at the very least.

This comment has been minimized.

Copy link
@tannergooding

tannergooding Oct 20, 2016

Author Member

This is the exact behavior that the double tests currently use. But I agree, it is undesirable.

I am going to go ahead and convert this to use the same logic as the PAL tests I wrote, which is:

var delta = MathF.Abs(result - expected);
if (delta > allowedVariance)
{
    throw new EqualException(...);
}

Where allowedVariance is the cross platform machine epsilon (4.76837158e-07f) multiplied by the a power of 10 to allow for proper rounding (so an expected result of 0.xxxxxxxxx is just machine epsilon, while x.xxxxxxxx is machine epsilon * 10, and 0.0xxxxxxxxx is machine epsilon / 10).

This is at least better in the sense that MathF.Abs calls to the existing Math.Abs function, which is known to be good.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 20, 2016

@mellinoe, I agree that converting these to use [Theory] is desirable. I will log that as part of the bug (mentioned above) to expand the test coverage here and in System.Math.

I am going to assign the bug to myself and start on it immediately, but would like to get the initial change completed since it provides parity with the System.Math coverage and will allow me to finish writing the performance tests for the CoreCLR repo.

@mellinoe

This comment has been minimized.

Copy link
Contributor

mellinoe commented Oct 20, 2016

but would like to get the initial change completed since it provides parity with the System.Math coverage and will allow me to finish writing the performance tests for the CoreCLR repo

Sounds good to me.

@tannergooding tannergooding force-pushed the tannergooding:math branch from c171fab to 0faca11 Oct 20, 2016

@tannergooding tannergooding force-pushed the tannergooding:math branch 2 times, most recently from 340f144 to 5196633 Oct 20, 2016

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 21, 2016

The contracts for System.MathF were slightly messed up when the NGEN aliasing issue was fixed. #12836 contains the necessary fixes (beta-24620-03+)

@tannergooding tannergooding force-pushed the tannergooding:math branch from 5196633 to 74c99e0 Oct 22, 2016

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 23, 2016

@mellinoe, I must be missing some change that causes System.Runtime.Extensions to attempt to load System.MathF from the Microsoft.TargetingPack.Private.CoreCLR package rather than from System.Runtime.Extensions itself...

Are you aware of what is missing here?

@tannergooding tannergooding force-pushed the tannergooding:math branch 2 times, most recently from 0503741 to fa650ee Oct 23, 2016

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 24, 2016

Looks like what I'm missing is whatever causes the TypeForwardedTo declarations to appear in Sytem.Runtime.Extensions. However, these appear to be automatically generated by something (and it isn't obvious to me from going through the project and targets files).

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 24, 2016

Ok, after a lot more digging, it looks like the project ends up running GenFacades post build. It also looks like this ends up using bin\ref\System.Runtime.Extensions\4.2.0.0\System.Runtime.Extensions.dll as the contracts assembly, rather than bin\ref\System.Runtime.Extensions\4.2.0.0\netcoreapp1.1\System.Runtime.Extensions.dll, so it doesn't end up creating the redirects for System.MathF.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 24, 2016

I can 'fix' the issue and get tests passing by adding a new source file and manually adding the appropriate type forward. However, I do not believe this is the correct fix.

Could someone please confirm what the correct fix is here?

@tannergooding tannergooding force-pushed the tannergooding:math branch from fa650ee to f459826 Oct 24, 2016

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 25, 2016

Ping @mellinoe to answer (or direct to someone for answering) the above question.

@mellinoe

This comment has been minimized.

Copy link
Contributor

mellinoe commented Oct 25, 2016

@tannergooding Sorry about not getting back to you. I'll take a look at your branch and see if I can figure out the problem.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 25, 2016

Thanks!

@mellinoe

This comment has been minimized.

Copy link
Contributor

mellinoe commented Oct 26, 2016

@tannergooding It looks like there is a limitation in how we are resolving the contract references right now: dotnet/buildtools#1041. We're in a sort of intermediate state right now where we have two different build configurations for the reference assembly, and the one we're choosing right now is not correct for this scenario, as you discovered. Eventually the problem will go away because we will only have one reference assembly, but that is still in the works.

Right now, just go ahead and add the manual type-forward file like you mentioned above. If you don't mind, please put a comment in the csproj file (or in the .cs file) mentioning dotnet/buildtools#1041 so that we remember to revert it when that is fixed.

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 26, 2016

@mellinoe, Thanks for the confirmation. I have added the manual TypeForwardedToAttribute for System.MathF and ensured that both the .cs file and the .csproj file have a link to the bug.

Slightly separate here, but I noticed the following in the .csproj file (https://github.com/dotnet/corefx/blob/master/src/System.Runtime.Extensions/src/System.Runtime.Extensions.csproj#L382)

<ItemGroup Condition="'$(TargetGroup)' == 'net462' Or '$(TargetGroup)' == 'net463'">
  <TargetingPackReference Include="mscorlib" />
  <TargetingPackReference Include="System" />
</ItemGroup>
<ItemGroup Condition="'$(TargetGroup)' != 'net462' Or '$(TargetGroup)' != 'net463'">
  <TargetingPackReference Include="System.Private.CoreLib" />
</ItemGroup>

Specifically, the condition on the second ItemGroup is using Or, when I would have expected it to use And (as is the case in several other .csproj files I checked).

Is this by-design, or can I include a fix for it here?

@tannergooding tannergooding force-pushed the tannergooding:math branch from 02ec356 to f7d1819 Oct 26, 2016

@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 26, 2016

@mellinoe, everything is looking good here.

@mellinoe

This comment has been minimized.

Copy link
Contributor

mellinoe commented Oct 26, 2016

Specifically, the condition on the second ItemGroup is using Or, when I would have expected it to use And (as is the case in several other .csproj files I checked).
Is this by-design, or can I include a fix for it here?

This is probably harmless since nothing seems to be breaking in those configurations. Strictly speaking we should probably fix it, but I was chatting with @ericstj and we thought that we really should only have one of these net46 configurations in the first place. So if we wanted to fix it, we would probably just delete one of those configurations and the problem would go away. I would say just ignore it for this work here.

@mellinoe

This comment has been minimized.

Copy link
Contributor

mellinoe commented Oct 26, 2016

Everything looks good to me. Thanks a lot for all the work you've put into this. It looks like a really high-quality addition, and I know I'm planning on using it in some of my projects.

@mellinoe mellinoe merged commit ec87e62 into dotnet:master Oct 26, 2016

10 checks passed

Innerloop CentOS7.1 Debug Build and Test Build finished.
Details
Innerloop CentOS7.1 Release Build and Test Build finished.
Details
Innerloop Linux ARM Emulator SoftFP Debug Cross Build Build finished.
Details
Innerloop Linux ARM Emulator SoftFP Release Cross Build Build finished.
Details
Innerloop OSX Debug Build and Test Build finished.
Details
Innerloop OSX Release Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Debug Build and Test Build finished.
Details
Innerloop Ubuntu14.04 Release Build and Test Build finished.
Details
Innerloop Windows_NT Debug Build and Test Build finished.
Details
Innerloop Windows_NT Release Build and Test Build finished.
Details
@tannergooding

This comment has been minimized.

Copy link
Member Author

tannergooding commented Oct 26, 2016

Thanks! I will continue working on getting the remaining work (perf tests, expanded unit tests, etc) done in my spare time!

@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016

@tannergooding tannergooding deleted the tannergooding:math branch Feb 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.