Optimize 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' #101858

Sergio0694 · 2024-05-03T18:12:12Z

Closes #101821

This PR optimizes 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' to drop the dependency on Convert.* methods. Because the full matrix of types we have to handle is known, we can just hardcode it here, in a similar way to how other methods doing pretty much the same are already doing in the runtime. Should save a bit of size (eg. not root BigInteger) and be marginally faster.

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

dotnet-policy-service · 2024-05-03T18:16:30Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

jkotas · 2024-05-03T18:46:31Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

+            Failure:
+            Debug.Fail("Unexpected CorElementType: " + dstElementType + ": Not a valid widening target.");
+            dstObject = null;
+            return CreateChangeTypeException(srcEEType, dstEEType, semantics);
        }

        private static bool CanPrimitiveWiden(EETypeElementType destType, EETypeElementType srcType)


Delete - no longer used?

Removed in 54092d4.

Can we delete the CanPrimitiveWiden method and create the exception at the end? I do not think that we care about how fast the exception gets created.

I deleted it in 95ea639🙂
Will leave this open, feel free to resolve if that change matches what you had in mind.

Sergio0694 · 2024-05-03T18:54:53Z

@jkotas was looking at this table:

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

Lines 225 to 242 in 6f18b5e

    
           ReadOnlySpan<ushort> primitiveAttributes = [ 
        
               0x0000, // Unknown 
        
               0x0000, // Void 
        
               0x0004, // Boolean (W = BOOL) 
        
               0xCf88, // Char (W = U2, CHAR, I4, U4, I8, U8, R4, R8) 
        
               0xC550, // SByte (W = I1, I2, I4, I8, R4, R8) 
        
               0xCFE8, // Byte (W = CHAR, U1, I2, U2, I4, U4, I8, U8, R4, R8) 
        
               0xC540, // Int16 (W = I2, I4, I8, R4, R8) 
        
               0xCF88, // UInt16 (W = U2, CHAR, I4, U4, I8, U8, R4, R8) 
        
               0xC500, // Int32 (W = I4, I8, R4, R8) 
        
               0xCE00, // UInt32 (W = U4, I8, R4, R8) 
        
               0xC400, // Int64 (W = I8, R4, R8) 
        
               0xC800, // UInt64 (W = U8, R4, R8) 
        
               0x0000, // IntPtr 
        
               0x0000, // UIntPtr 
        
               0xC000, // Single (W = R4, R8) 
        
               0x8000, // Double (W = R8) 
        
           ];

And noticed that UInt32 is not a widening source for UInt64. Is it just the comment that's wrong? 🤔
I mean, shouldn't UInt32 also be a valid widening source for a UInt64 target?

jkotas · 2024-05-03T20:20:25Z

And noticed that UInt32 is not a widening source for UInt64. Is it just the comment that's wrong?

Looks like a bug to me.

Sergio0694 · 2024-05-03T20:39:36Z

I've added the flag in ee8def8.

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

jkotas · 2024-05-03T22:35:27Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

-                case EETypeElementType.Int16:
-                    short shortValue = Convert.ToInt16(srcObject);
-                    dstObject = dstEEType->IsEnum ? Enum.ToObject(dstEEType, shortValue) : shortValue;
+                    // Can only be sbyte here


Is this correct? Can this be reboxing enum to primitive or vice versa? I think the exact matches are handled earlier.

What do you mean? My understanding was that regardless of whether either value was an enum here, we're just copying the raw primitive value to that local and then RuntimeHelpers.Box will take care of both scenarios at the end, either just boxing the primitive or boxing it as the target enum type. Is that not correct? 🤔

This comment was meant to be on the bool case. The bool-backed enums are partially supported. I am not sure whether we can get here with bool backed enum, but the current code handles the bool backed enums so it would keep it that way.

Oh interesting. I thought we had removed bool-backed enum support in .NET 8. Anyway, fixed in cdccf69. It also allows us to remove that check on rawDstValue not being null, so that's also nice 😄

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

…Utils.cs

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

jkotas · 2024-05-05T03:21:37Z

I got an idea on how to de-duplicate the implementation with the array one - pushed it into this PR.

jkotas · 2024-05-05T03:22:20Z

MichalStrehovsky/rt-sz#17 (comment)

Is this expected or would you expect the savings to be higher?

jkotas · 2024-05-05T03:23:20Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

        }

-        private static bool CanPrimitiveWiden(EETypeElementType destType, EETypeElementType srcType)
+        [MethodImpl(MethodImplOptions.AggressiveInlining)] // Two callers, one of them is potentially perf sensitive
+        internal static bool CanPrimitiveWiden(EETypeElementType dstType, EETypeElementType srcType)


It turns out this method is useful to aid with deduplication, so I put it back.

jkotas · 2024-05-05T03:23:56Z

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs

@@ -232,7 +359,7 @@ private static bool CanPrimitiveWiden(EETypeElementType destType, EETypeElementT
                0xC540, // Int16 (W = I2, I4, I8, R4, R8)
                0xCF88, // UInt16 (W = U2, CHAR, I4, U4, I8, U8, R4, R8)
                0xC500, // Int32 (W = I4, I8, R4, R8)
-                0xCE00, // UInt32 (W = U4, I8, R4, R8)
+                0xCE00, // UInt32 (W = U4, I8, U8, R4, R8)


The bit mask looks correct, it is just the comment that's wrong.

Sergio0694 · 2024-05-05T04:50:15Z

I'm a bit puzzled by eg. the minimal hello world sample showing a 0 bytes diff. We were clearly seeing a bunch of stuff being rooted by this method in our WinRT component. Wondering if there wasn't some other secondary root that was also keep that same stuff alive, or if it's something else. Would be curious to check that same project out with sizoscope once I get a new build of the runtime with this change in as well 🤔

MichalStrehovsky · 2024-05-05T08:51:10Z

I'm a bit puzzled by eg. the minimal hello world sample showing a 0 bytes diff

I would be surprised if the code touched in this PR is part of a minimal hello world. One has to do reflection invocation, or something like Array.SetValue to drag any of this into the app. CsWinRT uses reflection.

MichalStrehovsky · 2024-05-05T11:22:27Z

I've added a two more apps to rt-sz measurements and they do show savings. 3% on a small app that uses reflection. It also still demonstrates savings for something more substantial, like a raw kestrel app. Nice!

Project	Size before	Size after	Difference
avalonia.app-linux	24496560	24496560	0
avalonia.app-windows	22036480	22035456	-1024
hello-linux	1377040	1377040	0
hello-minimal-linux	1110696	1110696	0
hello-minimal-windows	885248	884736	-512
hello-windows	1133056	1133056	0
kestrel-minimal-linux	5772000	5721760	-50240
kestrel-minimal-windows	5072384	5028352	-44032
reflection-linux	2294656	2232032	-62624
reflection-windows	1944576	1888256	-56320
webapiaot-linux	10249144	10249144	0
webapiaot-windows	9206272	9205760	-512

jkotas · 2024-05-05T16:42:11Z

/azp run runtime-nativeaot-outerloop

azure-pipelines · 2024-05-05T16:42:27Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2024-05-06T00:22:29Z

/ba-g Slow mac

jkotas

Thank you for raising the issues and the collaboration!

Sergio0694 · 2024-05-06T00:27:04Z

Happy to help! I really appreciate you taking the time to review and get this one through! 🙂

Sergio0694 · 2024-05-07T16:30:57Z

Leaving this for reference, this PR saves 53 KB in our minimal WinRT component sample, and finally lets us get below 2 MB for the first time too 😄

…#101858) * Optimize 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' * Add missing U4 -> U8 widening flag * Skip type checks when unboxing * Centralized boxing operations * Remove unnecessary code to handle enums * Combine 'Char' cases with 'UInt16' ones * Deduplicate the implementation between array and reflection --------- Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Optimize 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible'

a3b0370

Sergio0694 requested a review from MichalStrehovsky as a code owner May 3, 2024 18:12

dotnet-issue-labeler bot added the area-NativeAOT-coreclr label May 3, 2024

Sergio0694 commented May 3, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 3, 2024

jkotas reviewed May 3, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

Remove comments

b804117

jkotas reviewed May 3, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

Sergio0694 marked this pull request as draft May 3, 2024 18:36

jkotas reviewed May 3, 2024

View reviewed changes

Sergio0694 added 3 commits May 3, 2024 13:32

Fix widening conversions (were actually narrowing)

488e251

Remove unreachable call

54092d4

Add missing U4 -> U8 widening flag

ee8def8

Skip type checks when unboxing

9ed8cd9

build-analysis bot mentioned this pull request May 3, 2024

System.Numerics.Tensors.Tests.SingleGenericTensorPrimitives.SpanScalarDestination_SpecialValues fails #101721

Closed

Centralized boxing operations

a2e5038

Sergio0694 marked this pull request as ready for review May 3, 2024 21:18

jkotas reviewed May 3, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

Remove unnecessary code to handle enums

eeb38bf

jkotas reviewed May 3, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

jkotas reviewed May 3, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request May 4, 2024

slow macOS - "##[error]The job running on agent Azure Pipelines 9 ran longer than the maximum time of 60 minutes." dotnet/dnceng#1883

Open

3 tasks

Sergio0694 force-pushed the user/sergiopedri/optimize-ConvertOrWidenPrimitivesEnumsAndPointersIfPossible branch from 84fbc82 to ff4abbc Compare May 4, 2024 19:44

Sergio0694 added 2 commits May 4, 2024 12:44

Remove 'CanPrimitiveWiden' method

28f9fd7

Fix 'SByte' and 'Byte' cases

6ab6d86

Sergio0694 and others added 2 commits May 4, 2024 13:25

Replace 'RuntimeHelpers.Box' with 'RuntimeImports.RhBox'

f812700

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Restore support for bool-backed enums too

cdccf69

jkotas reviewed May 4, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

Update src/coreclr/nativeaot/System.Private.CoreLib/src/System/Invoke…

d15a9e7

…Utils.cs

jkotas reviewed May 4, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Show resolved Hide resolved

jkotas reviewed May 4, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

jkotas reviewed May 4, 2024

View reviewed changes

src/coreclr/nativeaot/System.Private.CoreLib/src/System/InvokeUtils.cs Outdated Show resolved Hide resolved

Combine 'Char' cases with 'UInt16' ones

62c3674

This was referenced May 4, 2024

Debug assertion failure in System.Net.Http.HttpConnection.SendAsync #100616

Closed

System.Net.Tests.HttpWebRequestTest_Async.GetResponseAsync_ParametersAreNotCachable_CreateNewClient test fails #100912

Closed

github-actions bot mentioned this pull request May 5, 2024

101858 MichalStrehovsky/rt-sz#17

Closed

Deduplicate the implementation between array and reflection

b678254

jkotas reviewed May 5, 2024

View reviewed changes

jkotas approved these changes May 6, 2024

View reviewed changes

jkotas merged commit f501153 into dotnet:main May 6, 2024
101 of 108 checks passed

Sergio0694 deleted the user/sergiopedri/optimize-ConvertOrWidenPrimitivesEnumsAndPointersIfPossible branch May 6, 2024 00:27

github-actions bot locked and limited conversation to collaborators Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' #101858

Optimize 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' #101858

Sergio0694 commented May 3, 2024

dotnet-policy-service bot commented May 3, 2024

jkotas May 3, 2024

Sergio0694 May 3, 2024

jkotas May 3, 2024

Sergio0694 May 4, 2024 •

edited

Loading

Sergio0694 commented May 3, 2024

jkotas commented May 3, 2024

Sergio0694 commented May 3, 2024

jkotas May 3, 2024

Sergio0694 May 4, 2024

jkotas May 4, 2024

Sergio0694 May 4, 2024

jkotas commented May 5, 2024

jkotas commented May 5, 2024

jkotas May 5, 2024

jkotas May 5, 2024

Sergio0694 commented May 5, 2024

MichalStrehovsky commented May 5, 2024

MichalStrehovsky commented May 5, 2024

jkotas commented May 5, 2024

azure-pipelines bot commented May 5, 2024

jkotas commented May 6, 2024

jkotas left a comment

Sergio0694 commented May 6, 2024

Sergio0694 commented May 7, 2024

Optimize 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' #101858

Optimize 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' #101858

Conversation

Sergio0694 commented May 3, 2024

Closes #101821

dotnet-policy-service bot commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 May 4, 2024 • edited Loading

Choose a reason for hiding this comment

Sergio0694 commented May 3, 2024

jkotas commented May 3, 2024

Sergio0694 commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented May 5, 2024

jkotas commented May 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sergio0694 commented May 5, 2024

MichalStrehovsky commented May 5, 2024

MichalStrehovsky commented May 5, 2024

jkotas commented May 5, 2024

azure-pipelines bot commented May 5, 2024

jkotas commented May 6, 2024

jkotas left a comment

Choose a reason for hiding this comment

Sergio0694 commented May 6, 2024

Sergio0694 commented May 7, 2024

Sergio0694 May 4, 2024 •

edited

Loading