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

'ConvertOrWidenPrimitivesEnumsAndPointersIfPossible' in NativeAOT is unnecessarily rooting 'BigInteger' (and a bunch more stuff) due to 'Convert.*' calls #101821

Closed
Sergio0694 opened this issue May 2, 2024 · 3 comments · Fixed by #101858
Labels
area-NativeAOT-coreclr size-reduction Issues impacting final app size primary for size sensitive workloads

Comments

@Sergio0694
Copy link
Contributor

While working on reducing the size for WinRT component using NativeAOT, we found some numeric stuff being kept:

image

Quoting @MichalStrehovsky on Discord:

"Looks like the Convert.ToSingle call in ConvertOrWidenPrimitivesEnumsAndPointersIfPossible is an overkill, we don't need code to parse floats since conversion from string to single would have been disallowed"

This is the entire set that needs to be supported:

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)
];
. Calling into convert just calls IConvertible methods so instead of the subset, one gets conversions from everything that was allocated in the app and supports IConvertible (that's why you get string -> double conversions). I don't know if there's a better mechanism in the BCL. One could just inline conversions for the whole matrix since there is an upper bound but that may not pass Jan's code review. If you can find a more narrow codepath in the BCL that handles the subset, that would be a good fix"

Opening this issue for tracking. @jkotas do you happen to know whether there's already some helper we can reuse here, or do you have any suggestions on how we could remove that dependency on Convert.* calls? Would hardcoding all possible combinations be acceptable, or something else? 🙂

@Sergio0694 Sergio0694 added size-reduction Issues impacting final app size primary for size sensitive workloads area-NativeAOT-coreclr labels May 2, 2024
Copy link
Contributor

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 2, 2024
Copy link
Contributor

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

@jkotas
Copy link
Member

jkotas commented May 3, 2024

We have number of copies of the wideling algorithm both in managed code

private static unsafe void CopyImplPrimitiveTypeWithWidening(Array sourceArray, int sourceIndex, Array destinationArray, int destinationIndex, int length, bool reliable)

private static Exception ConvertOrWidenPrimitivesEnumsAndPointersIfPossible(object srcObject, MethodTable* dstEEType, CheckArgumentSemantics semantics, out object? dstObject)

private static unsafe void CopyImplPrimitiveWiden(Array sourceArray, int sourceIndex, Array destinationArray, int destinationIndex, int length)

public static object ConvertOrWiden(RuntimeType srcType, object srcObject, RuntimeType dstType, CorElementType dstElementType)

And several more in native code.

Avoiding IConvertible,in in the native AOT implementations sounds good to me if it is only ever used for primitives. Bonus points for making the different implementations more uniform. Physically sharing the code may be too hard - I am not sure.

@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label May 6, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr size-reduction Issues impacting final app size primary for size sensitive workloads
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants