Skip to content

Conversation

@Sergio0694
Copy link
Member

@Sergio0694 Sergio0694 commented Jun 21, 2020

Adds the .NET 5 target to Microsoft.Toolkit.HighPerformance

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

The Microsoft.Toolkit.HighPerformance package maxes out at .NET Core 3.1.
The Microsoft.Toolkit package maxes out at .NET Standard 2.1.

Additionally, Microsoft.Toolkit doesn't have proper nullability annotations, and it reports installing additional dependencies if installed in a .NET 5 apps. The extra dependency is System.Runtime.CompilerServices.Unsafe which is actually built-in on .NET 5, but consumers not aware of this would still see the installation prompt from NuGet as reporting an extra indirect dependency.

What is the new behavior?

✅ Added .NET 5 target to Microsoft.Toolkit.HighPerformance
✅ Added .NET 5 target to Microsoft.Toolkit
✅ Enabled global nullability annotations to Microsoft.Toolkit and improved the codebase.
✅ Enabled C# 9 in both projects, with some extra code tweaks.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented Jun 21, 2020

Thanks Sergio0694 for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost assigned azchohfi Jun 21, 2020
@michael-hawker michael-hawker added this to the future milestone Jun 23, 2020
@michael-hawker
Copy link
Member

.NET 5 is flux still, so we probably have to sit on this a bit until we know more in terms of timings and plans.

@Sergio0694
Copy link
Member Author

@michael-hawker Yeah absolutely, that's why I opened this as draft 👍

This was mostly to keep a working branch with .NET 5 up to date (so I can merge new changes from the other branches as they come in), and ready to go with the new .NET 5 features where applicable. Then whenever we're ready to actually add support for .NET 5 to the toolkit, the PR will be all setup already 🚀

@Sergio0694 Sergio0694 added the high-performance 🚂 Issues/PRs for the Microsoft.Toolkit.HighPerformance package label Jul 7, 2020
@Sergio0694 Sergio0694 force-pushed the feature/net-5-target branch 2 times, most recently from 277cd28 to a03ef74 Compare October 9, 2020 11:47
@Sergio0694
Copy link
Member Author

Sergio0694 commented Oct 9, 2020

Hey @azchohfi, wanted to run an idea by you 😄

Once .NET 5 comes out, both CoreCLR and Mono will be able to run the same .NET 5 assemblies with no trouble, with Mono now officially supporting .NET 5 targets directly. This means that we will no longer have a way to tell what runtime we're currently on, so weird runtime-specific tricks are technically no longer guaranteed to work, since Mono has a different layout for some objects (eg. ND arrays). I've already done some work today in #3353 to improve the portable versions of those APIs, and I'm thinking with the new .NET 5 target we will probably just have to switch back to the portable implementation again to ensure it runs fine on both CoreCLR and Mono. I was thinking, would it be possible/reasonable to set the CI to run the .NET 5 unit test project for the HighPerformance package with both CoreCLR and Mono? I saw this tweet from Ben Adams earlier today with some benchmarks he run with both runtimes, so I know it's possible, but I'm not sure how complex it would be to setup in the CI and/or if this is something you would be interested in. Alternatively, would it be possible to have an easy to use script to at least be able to run the Mono tests locally, so that we can validate the builds every now and then, or at least this PR before merging it?

I mean, I think I've addressed all the critical runtime specific hacks in the other PR, moving to a portable implementation we can also use on the .NET 5 target (adapted the implementation from the original Span<T> portable version from CoreFX), but I figured it would be a good idea to double check everything actually works fine for .NET 5 😄

Let me know what you think!

EDIT: asked in the .NET Evolution Discord server and two Mono engineers suggested these two possible approaches:

They both need Linux, but they should also work on WSL on Windows. In particular they said the second one was tested on WLS on an app created with dotnet new console. I think as a starting point it might be useful to setup a Mono .NET 5 project that just includes the same shared test project for the HighPerformance package, that can at least be individually run through WSL. This way we could at least manually validate the package at any point in time. Of course eventually having this directly in the CI would be super cool too 🚀

EDIT 2: alright, this worked fine for me to run a sample .NET 5 project with Mono so far 😄

  • Enable WSL2 on Windows
  • Install Ubuntu
  • Install the .NET 5 SDK (setup the packages repo from here, then the .NET 5 SDK from here)
  • Run dotnet new console in some new folder
  • Copy over the two files from here to this folder, overwriting the .csproj file (I just used curl in this case)
  • dotnet build and dotnet run and you'll get the app running with .NET 5 Mono! 🚀

EDIT 3: shared unit test project for the base packages all passing on .NET 5 Mono! 🚀

image

Now on to test the HighPerformance package 🤣🤞

Seeing if I can get a unit test project from the toolkit to work with this too now 😄

EDIT 4: victory! 🎉🎉🎉

image

All the unit tests from the HighPerformance package on #3353 are passing on .NET 5 Mono! 🚀
The ones from #3367 are failing due to the incorrect signature key for System.Private.CoreLib on .NET 5, will go and fix that.

EDIT 5: looks like adding the .NET 5 target and adjusting the compiler directives for the IgnoreAccessCheckTo attribute in the Microsoft.Toolkit.HighPerformance solves the assembly loading errors on .NET 5. I would say it might be best to then wait for this PR to be merged before merging #3367 so that when that is merged it'll already be working on all supported runtimes.

EDIT 6: as suggested by a Mono engineer in the .NET Evolution server, I added MONO_ENV_OPTIONS=--version to double check that the code was actually running with Mono, and that actually seems to not be the case. Trying again using the configuration from the MonoNET5Sample repo now.

EDIT 7: well, using the MonoNET5Sample setup I could run a program with .NET 5 Mono LLVM just fine, but that doesn't work with dotnet test, as that just defaults to using .NET 5 CoreCLR. Not sure how to actually get the unit test runner to use Mono 🤔

EDIT 8: found a solution, see below 😄

@Sergio0694
Copy link
Member Author

Sergio0694 commented Oct 13, 2020

I figured it out! 🎉🎉🎉
Posting this to make this more accessible, as it contains a working solution 😄

Here are the steps to modify a unit test project to run it on .NET 5 Mono:

  • Delete Directory.Build.props, Directory.Build.targets and nuget.config from the root
  • Create a new Program class and paste this manual unit test runner I wrote, source. This is needed because .NET 5 Mono can only run with dotnet run and not with dotnet test, so we need to manually run the unit tests from a normal application.
  • Open the .csproj file and use this code for the first PropertyGroup:
<PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net5.0</TargetFramework>
    <PublishTrimmed>true</PublishTrimmed>
    <TrimMode>link</TrimMode>
    <RuntimeSuffix>.LLVM.AOT</RuntimeSuffix>
    <StartupObject>UnitTests.NetCore.Program</StartupObject>
  </PropertyGroup>
  • Still in that .csproj file, paste this code somewhere:
<ItemGroup>
    <KnownFrameworkReference
      Include="Microsoft.NETCore.App.Mono$(RuntimeSuffix)"
      TargetFramework="$(TargetFramework)"
      RuntimeFrameworkName="Microsoft.NETCore.App"
      DefaultRuntimeFrameworkVersion="$(BundledNETCorePlatformsPackageVersion)"
      LatestRuntimeFrameworkVersion="$(BundledNETCorePlatformsPackageVersion)"
      TargetingPackName="Microsoft.NETCore.App.Ref"
      TargetingPackVersion="$(BundledNETCorePlatformsPackageVersion)"
      RuntimePackNamePatterns="Microsoft.NETCore.App.Runtime.Mono$(RuntimeSuffix).**RID**"
      RuntimePackRuntimeIdentifiers="$(RuntimeIdentifier)"
      IsTrimmable="true" />
    <FrameworkReference Remove="Microsoft.NETCore.App" />
    <FrameworkReference
      Include="Microsoft.NETCore.App.Mono$(RuntimeSuffix)"
      IsImplicitlyDefined="true"
      Pack="true"
      PrivateAssets="All" />
  </ItemGroup>
  • Open bash from Ubuntu WSL2, and run these commands:
export MONO_THREADS_SUSPEND="preemptive"
export MONO_ENV_OPTIONS="--llvm --ffast-math"
dotnet run -c Release -r linux-x64

Final result:

image

@alexchx @michael-hawker Not the simplest solution, but we now have a way to run our tests on Mono as well 😄

NOTE: 100% of the unit tests in both the standard and HighPerformance projects (from #3353) pass on .NET Mono LLVM 🎉

@Sergio0694
Copy link
Member Author

Adding this to the 7.0 milestone as we absolutely need to get this merged in for the next release or the HighPerformance package will be broken on .NET 5 once #3367 gets merged in (as the dummy System.Private.CoreLib project wouldn't be loaded on .NET 5 without the necessary .csproj update that I can include here). Without the fixes in this PR, the ND array indexers would likely be broken on .NET 5 Mono as well, as the fixes in #3353 wouldn't kick in there as they'd just be detecting .NET 5 and assume they're on CoreCLR. This PR fixes this too. Long story short, I've verified that the HighPerformance package passes the unit tests on .NET 5 and .NET 5 Mono as well, and since .NET 5 is already go-live anyway we should definitely include support for it in the next release 😄

@Sergio0694 Sergio0694 modified the milestones: future, 7.0 Oct 25, 2020
@michael-hawker michael-hawker added the next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. label Oct 29, 2020
@Sergio0694 Sergio0694 force-pushed the feature/net-5-target branch from d9791b7 to aabf73d Compare November 5, 2020 18:01
@Sergio0694 Sergio0694 marked this pull request as ready for review November 5, 2020 22:17
@Sergio0694 Sergio0694 added .NET Components which are .NET based (non UWP specific) feature 💡 improvements ✨ optimization ☄ Performance or memory usage improvements and removed DO NOT MERGE ⚠️ labels Nov 21, 2020
@Sergio0694
Copy link
Member Author

Updated the PR, pulled all changes from master and made some more tweaks, removed "DO NOT MERGE" now! 🚀

@Sergio0694 Sergio0694 changed the title .NET 5 target for HighPerformance package .NET 5 target for Toolkit and HighPerformance packages Nov 22, 2020
/// Throws an <see cref="OverflowException"/> when the "column" parameter is invalid.
/// </summary>
public static void ThrowOverflowException()
private static void ThrowOverflowException()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this public in 6.1? This is just an internal helper, eh?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankfully not 🤣
This is just an internal helper that I added in #3353, but that accidentally left out as public there. I think it was because this was initially in an internal throw helper class (so the method was public), then I moved it here and forgot to change the accessibility modifier. Really glad I spotted it in time for 7.0 😅

public static unsafe byte ToByte(this bool flag)
{
return Unsafe.As<bool, byte>(ref flag);
return *(byte*)&flag;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New optimization from .NET 5 compiler???

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahahah nope, just good old C# trickery 😄
This was actually a suggestion from Tanner, the idea here is that the two versions do exactly the same anyway, but using raw pointers means the GC doesn't have to also track the additional metadata to ensure that if a GC collection happens while this method is running, the locals are properly handled. In our case that's not needed since the target values are already pinned (they're just local arguments passed by copy). So using raw pointers will produce the same exact codegen, but with some theoretical minor indirect benefits on top of that 👍

@ghost
Copy link

ghost commented Nov 24, 2020

Hello @michael-hawker!

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.

@ghost ghost merged commit d261a24 into CommunityToolkit:master Nov 24, 2020
ghost pushed a commit that referenced this pull request Jan 28, 2021
## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

<!-- - Bugfix -->
- Feature
- Optimization
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## What is the current behavior?
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
There are no `nint`/`nuint` overloads for the any `Guard` APIs. These types have been introduced with C# 9, so it makes sense to add proper support for them given that #3356 added full support for C# 9 and .NET 5 to these packages.
Also there were some codegen bits in `Guard.IsCloseTo` that were not ideal.
Additionally, the codegen for the `Guard` APIs didn't leverage compiler support properly.

## What is the new behavior?
<!-- Describe how was this issue resolved or changed? -->
✅ Added new `nint`/`nuint` overloads to `Guard` APIs
🚀 Improved codegen in a number of `Guard.IsCloseTo` overloads
🚀 Improved codegen for faulting blocks

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->
- [ ] Sample in sample app has been added / updated (for bug fixes / features)
    - [ ] Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [X] Contains **NO** breaking changes
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto merge ⚡ feature 💡 high-performance 🚂 Issues/PRs for the Microsoft.Toolkit.HighPerformance package improvements ✨ .NET Components which are .NET based (non UWP specific) optimization ☄ Performance or memory usage improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants