Guard overloads for nint/nuint, codegen improvements #3573

Sergio0694 · 2020-11-23T14:51:05Z

PR Type

What kind of change does this PR introduce?

Feature
Optimization

What is the current behavior?

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?

✅ 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:

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)
- Icon has been created (if new sample) following the Thumbnail Style Guide and templates
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 · 2020-11-23T14:51:19Z

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 🙌

Sergio0694 · 2020-11-25T00:22:46Z

#3356 has been merged, so I rebased this PR on top of master and removed "DO NOT MERGE", ready for review! 🚀

Sergio0694 · 2020-11-29T12:57:42Z

Made a big codegen improvement in 380f6ae as I noticed we weren't really producing the best assembly possible even though we had tried manually inverting the conditional blocks. The previous setup was done mostly just for our internal convenience - this is a bit trickier as we need to check the order of parameters for the various exceptions, but the end result is much, much better 🚀

Small example when just calling Guard.IsNotNull and returning 42:

; BEFORE
    sub      rsp, 40
    test     rcx, rcx
    jne      SHORT G_M27046_IG04
    mov      rdx, 0xD1FFAB1E
    mov      rdx, gword ptr [rdx]
    mov      rcx, 0xD1FFAB1E
    call     Microsoft.Toolkit.Diagnostics.ThrowHelper:ThrowArgumentNullExceptionForIsNotNull(System.String)
G_M27046_IG04:
    mov      eax, 42
    add      rsp, 40
    ret

; AFTER
    sub      rsp, 40
    test     rcx, rcx
    je       SHORT G_M39878_IG05
    mov      eax, 42
    add      rsp, 40
    ret
G_M39878_IG05:
    mov      rdx, 0xD1FFAB1E
    mov      rdx, gword ptr [rdx]
    mov      rcx, 0xD1FFAB1E
    call     Microsoft.Toolkit.Diagnostics.ThrowHelper:ThrowArgumentNullExceptionForIsNotNull2(System.String)
    int3

Note how the throw path is now correctly put at the end, the JIT recognizes that (int3 after it, as it knows it will always throw), and most importantly the non faulting path will not incur any jumps now, as we're branching to the exception instead. 🎉

Sergio0694 · 2020-11-30T19:24:22Z

I've restructured the internal architecture for the ThrowHelper.Guard methods so that they all now live in a nested private class within the Guard class, instead of just being internal methods in the ThrowHelper class. This is needed so when we publish a source-only version of these APIs (see #3587), consumers will not see those internal helpers for the Guard class in their IntelliSense (as the Guard class would just be part of their compilation). This way instead, all those helpers are just private and only directly visible from inside that class.

Sergio0694 · 2020-12-01T17:48:56Z

@azchohfi The CI is failing in this PR, but looking at the logs I'm not sure the issue is related to these changes.
It's weird because this PR is in sync with master, but other PRs seem to be running fine, so I'm confused 🤔

I'm seeing this:

"D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj" (default target) (2) -> (BuildNativePackage target) -> [...] \microsoft.net.native.compiler[...]\Microsoft.NetNative.targets(801,5): error : ILT0005: '[...]\ilc\Tools\nutc_driver.exe @"[...]\ilc\intermediate\MDIL\UnitTests.UWP.rsp"' returned exit code -1073741819

Any ideas? 😄

Sergio0694 · 2020-12-09T23:34:53Z

Looking into the CI build error

Tried rebasing on top of master just in case something had gone wrong in this branch ❌
From the CI build log, I get this:

"D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj" (default target) (2) ->
2020-12-09T23:21:15.9226990Z (BuildNativePackage target) ->
2020-12-09T23:21:15.9230614Z C:\Program Files (x86)\Microsoft SDKs\UWPNuGetPackages\microsoft.net.native.compiler\2.2.8-rel-28605-00\tools\Microsoft.NetNative.targets(801,5): error : ILT0005: 'C:\Program Files (x86)\Microsoft SDKs\UWPNuGetPackages\runtime.win10-x86.microsoft.net.native.compiler\2.2.8-rel-28605-00\tools\x86\ilc\Tools\nutc_driver.exe @"D:\a\1\s\UnitTests\UnitTests.UWP\obj\x86\Release\ilc\intermediate\MDIL\UnitTests.UWP.rsp"' returned exit code -1073741819 [D:\a\1\s\UnitTests\UnitTests.UWP\UnitTests.UWP.csproj]

So the error seems to be when building UnitTests.UWP with .NET Native. Trying to repro this locally.

Can repro this locally, definitely seems to be some issue with .NET Native. Trying to find the exact culprit.
Went back up to 6ace41b and it still crashes, so it looks very probably that commit is causing the crash on .NET Native. Seeing if I can pinpoint exactly what part is triggering the crash and whether I can wiggle the code to avoid it.
The error is definitely caused by Guard.IsBitwiseEqualTo. Not sure why exactly, but it's that.
Found a minimal repro for the issue! Pasting this into a blank UWP app crashes the .NET Native compiler:

public static unsafe void IsBitwiseEqualTo<T>(T value, T target, string name)
    where T : unmanaged
{
    if (sizeof(T) == 1)
    {
        if (*(byte*)&value == *(byte*)&target)
        {
            return;
        }

        Throw();
    }
    else if (sizeof(T) == 2)
    {
        if (*(ushort*)&value == *(ushort*)&target)
        {
            return;
        }

        Throw();
    }

    static void Throw() => throw new Exception();
}

Tested this on the latest version 6.2.11 of the .NET Native compiler, same crash there as well.

Reverting the changes in 6ace41b at this point, as a temporary workaround to unblock this PR. There shouldn't be a noticeable performance difference anyway in this case, though of course I'd have preferred to keep the revised version 😄

This reverts commit 6ace41b to work around a .NET Native compiler bug causing a crash. See more details in CommunityToolkit#3573 (comment).

Sergio0694 · 2020-12-10T01:36:01Z

CI passed! 🎉🎉🎉

michael-hawker · 2020-12-10T21:45:40Z

@Sergio0694 did you e-mail this to the dotnetnative @ microsoft address too?

FYI @tommcdon @michals

michael-hawker · 2020-12-10T21:49:05Z

Microsoft.Toolkit/Diagnostics/Generated/ThrowHelper.Collection.tt

+        /// <summary>
+        /// Helper methods to efficiently throw exceptions.
+        /// </summary>
+        private static partial class ThrowHelper


Why is this being wrapped in a class now? I think that's the main thing I'm confused about in the PR (and with all the additional new ThrowHelper wraps below). Is this a copy of the other ThrowHelpers or something else?

All these methods have always been wrapped in a class, it's just that before they were inside the public ThrowHelper class, and internal, while now they're all public and inside a new, private ThrowHelper class that's nested inside Guard. There is no behavioral different, it's just a code reorganization. The rationale for this is to make the code compatible (or rather, to make it better) in relation to the source-only Microsoft.Toolkit.Diagnostics package (#3587). The idea being that if we publish the source-only package and wanted to just copy-paste the Guard and ThrowHelper classes into consuming projects, they would've been able to also see all those internal helpers, which would've just cluttered their IntelliSense. With this structure instead all those internal helpers would still be invisible outside of the Guard class, even if the class was just added as source code directly into your project 🙂

Sergio0694 · 2020-12-10T21:49:55Z

@michael-hawker Yup, sent a full repro to them about an hour ago 😄

ghost · 2021-01-28T16:40:41Z

Hello @michael-hawker!

Because this pull request has the auto merge :zap: 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.

This reverts commit f286447 to work around a .NET Native compiler bug causing a crash. See more details in CommunityToolkit/WindowsCommunityToolkit#3573 (comment).

Sergio0694 added feature 💡 DO NOT MERGE ⚠️ optimization ☄ Performance or memory usage improvements .NET Components which are .NET based (non UWP specific) labels Nov 23, 2020

Sergio0694 added this to the 7.0 milestone Nov 23, 2020

ghost added the in progress 🚧 label Nov 23, 2020

ghost requested review from michael-hawker, azchohfi and Kyaa-dost November 23, 2020 14:51

Sergio0694 force-pushed the feature/nint-guard-apis branch from 15f71cb to 3b5eb17 Compare November 25, 2020 00:21

Sergio0694 removed the DO NOT MERGE ⚠️ label Nov 25, 2020

Sergio0694 added 11 commits December 9, 2020 23:54

Minor code tweaks

6ace41b

Added nint/nuint comparable Guard overloads

8c01abc

Refactored Guard.IsCloseTo unit tests

0258fec

Improved IsCloseTo unit tests

a412d31

Codegen improvements to Guard.IsCloseTo

84d447f

Added Guard.IsCloseTo overloads for nint type

6c8985f

Improved Guard APIs codegen

bfa1ebd

Renamed files to match new class structure

525be0c

Minor code refactoring for new class structure

e585856

Decoupled Guard helpers from ThrowHelper class

4ef9074

Bumped MSBuild.Sdk.Extras package version

467bc3b

Sergio0694 force-pushed the feature/nint-guard-apis branch from c03141b to 467bc3b Compare December 9, 2020 22:54

Revert "Minor code tweaks" to fix .NET Native error

4362738

This reverts commit 6ace41b to work around a .NET Native compiler bug causing a crash. See more details in CommunityToolkit#3573 (comment).

michael-hawker reviewed Dec 10, 2020

View reviewed changes

Rosuavio approved these changes Dec 11, 2020

View reviewed changes

Merge branch 'master' into feature/nint-guard-apis

1069076

Sergio0694 mentioned this pull request Dec 12, 2020

.NET 5 support for Notifications package #3622

Merged

7 tasks

Merge branch 'master' into feature/nint-guard-apis

4d140d8

michael-hawker approved these changes Jan 28, 2021

View reviewed changes

michael-hawker added the auto merge ⚡ label Jan 28, 2021

ghost merged commit f41314e into CommunityToolkit:master Jan 28, 2021

Sergio0694 deleted the feature/nint-guard-apis branch January 28, 2021 18:20

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Guard overloads for nint/nuint, codegen improvements #3573

Guard overloads for nint/nuint, codegen improvements #3573

Uh oh!

Sergio0694 commented Nov 23, 2020 •

edited

Loading

Uh oh!

ghost commented Nov 23, 2020

Uh oh!

Sergio0694 commented Nov 25, 2020

Uh oh!

Sergio0694 commented Nov 29, 2020

Uh oh!

Sergio0694 commented Nov 30, 2020

Uh oh!

Sergio0694 commented Dec 1, 2020

Uh oh!

Sergio0694 commented Dec 9, 2020 •

edited

Loading

Uh oh!

Sergio0694 commented Dec 10, 2020

Uh oh!

michael-hawker commented Dec 10, 2020

Uh oh!

michael-hawker Dec 10, 2020

Uh oh!

Sergio0694 Dec 10, 2020 •

edited

Loading

Uh oh!

Sergio0694 commented Dec 10, 2020

Uh oh!

ghost commented Jan 28, 2021

Uh oh!

Uh oh!

Guard overloads for nint/nuint, codegen improvements #3573

Guard overloads for nint/nuint, codegen improvements #3573

Uh oh!

Conversation

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

Uh oh!

PR Type

What is the current behavior?

What is the new behavior?

PR Checklist

Uh oh!

ghost commented Nov 23, 2020

Uh oh!

Sergio0694 commented Nov 25, 2020

Uh oh!

Sergio0694 commented Nov 29, 2020

Uh oh!

Sergio0694 commented Nov 30, 2020

Uh oh!

Sergio0694 commented Dec 1, 2020

Uh oh!

Sergio0694 commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Looking into the CI build error

Uh oh!

Sergio0694 commented Dec 10, 2020

Uh oh!

michael-hawker commented Dec 10, 2020

Uh oh!

michael-hawker Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

Sergio0694 Dec 10, 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!

Sergio0694 commented Dec 10, 2020

Uh oh!

ghost commented Jan 28, 2021

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.

Uh oh!

Uh oh!

Sergio0694 commented Nov 23, 2020 •

edited

Loading

Sergio0694 commented Dec 9, 2020 •

edited

Loading

Sergio0694 Dec 10, 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.