Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Optimize 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible'#101858

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

Sergio0694
Copy link
Contributor

Closes#101821

This PR optimizes 'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' to drop the dependency onConvert.* 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 rootBigInteger) and be marginally faster.

neon-sunset, PaulusParssinen, breadnone, nietras, and alexfalconflores reacted with hooray emoji
@dotnet-policy-servicedotnet-policy-servicebot added the community-contributionIndicates that the PR has been added by a community member labelMay 3, 2024
@dotnet-policy-serviceDotnet Policy Service
Copy link
Contributor

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

@Sergio0694Sergio0694 marked this pull request as draftMay 3, 2024 18:36
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)
Copy link
Member

Choose a reason for hiding this comment

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

Delete - no longer used?

Sergio0694 reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Removed in54092d4.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

@Sergio0694Sergio0694May 4, 2024•
edited
Loading

Choose a reason for hiding this comment

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

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

@Sergio0694
Copy link
ContributorAuthor

@jkotas was looking at this table:

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 thatUInt32 is not a widening source forUInt64. Is it just the comment that's wrong? 🤔
I mean, shouldn'tUInt32 also be a valid widening source for aUInt64 target?

@jkotas
Copy link
Member

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
Copy link
ContributorAuthor

I've added the flag inee8def8.

@Sergio0694Sergio0694 marked this pull request as ready for reviewMay 3, 2024 21:18
case EETypeElementType.Int16:
short shortValue = Convert.ToInt16(srcObject);
dstObject = dstEEType->IsEnum ? Enum.ToObject(dstEEType, shortValue) : shortValue;
// Can only be sbyte here
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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 thenRuntimeHelpers.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? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

This comment was meant to be on thebool 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.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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

@jkotas
Copy link
Member

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

@jkotas
Copy link
Member

MichalStrehovsky/rt-sz#17 (comment)

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

}

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)
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

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

@Sergio0694
Copy link
ContributorAuthor

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
Copy link
Member

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 likeArray.SetValue to drag any of this into the app. CsWinRT uses reflection.

@MichalStrehovsky
Copy link
Member

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!

ProjectSize beforeSize afterDifference
avalonia.app-linux 24496560 244965600
avalonia.app-windows 22036480 22035456-1024
hello-linux 1377040 13770400
hello-minimal-linux 1110696 11106960
hello-minimal-windows 885248 884736-512
hello-windows 1133056 11330560
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 102491440
webapiaot-windows 9206272 9205760-512
jkotas and PaulusParssinen reacted with thumbs up emojiSergio0694 reacted with rocket emoji

@jkotas
Copy link
Member

/azp run runtime-nativeaot-outerloop

@azure-pipelinesAzure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

/ba-g Slow mac

Copy link
Member

@jkotasjkotas left a comment

Choose a reason for hiding this comment

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

Thank you for raising the issues and the collaboration!

Sergio0694 reacted with heart emoji
@jkotasjkotas merged commitf501153 intodotnet:mainMay 6, 2024
@Sergio0694
Copy link
ContributorAuthor

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

@Sergio0694Sergio0694 deleted the user/sergiopedri/optimize-ConvertOrWidenPrimitivesEnumsAndPointersIfPossible branchMay 6, 2024 00:27
@Sergio0694
Copy link
ContributorAuthor

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 😄

image

MichalStrehovsky and nietras reacted with hooray emoji

michaelgsharp pushed a commit to michaelgsharp/runtime that referenced this pull requestMay 9, 2024
…#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>
Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull requestMay 30, 2024
…#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>
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 7, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@jkotasjkotasjkotas approved these changes

@MichalStrehovskyMichalStrehovskyAwaiting requested review from MichalStrehovskyMichalStrehovsky is a code owner

Assignees
No one assigned
Labels
area-NativeAOT-coreclrcommunity-contributionIndicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
3 participants
@Sergio0694@jkotas@MichalStrehovsky
[8]ページ先頭
©2009-2025 Movatter.jp