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

Remove LINQ expression dependencies on NativeAOT #1429

Merged
merged 31 commits into from
Jan 18, 2024

Conversation

Sergio0694
Copy link
Member

This PR attempts to fix the IL3050 warnings in CsWinRT due to System.Linq.Expressions being pulled in. The goal is to just remove that dependency entirely when on NativeAOT, and allow the linker to correctly trim all of that code, which should also improve the binary size a bit more. There were a few issues preventing this from happening:

  • Some checks for NativeAOT (via RuntimeFeature) were using unsupported patterns (see here)
  • Some paths were unconditionally relying on LINQ expressions (eg. Nullable<T>)

This PR fixes both of those issues for all occurrences.

@Sergio0694 Sergio0694 added performance Related to performance work authoring Related to authoring feature work AOT labels Jan 7, 2024
@dongle-the-gadget
Copy link
Contributor

Seems like we are missing some types in GetValueDelegateForFunctionPointer?

  • Point
  • Size
  • Rect
  • Array types

@Sergio0694
Copy link
Member Author

I just copied those from what was being registered in Projections.cctor. How come those weren't there? 🤔
As for array types... Why would those ever be in a Nullable<T> instance? Isn't IReference<T> just WinRT's "boxed" type?

@dongle-the-gadget
Copy link
Contributor

IReference<T> can also store arrays.

@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/trim-linq-expressions branch from 033cff4 to 6073923 Compare January 7, 2024 15:55
@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/trim-linq-expressions branch from e175069 to 71bb101 Compare January 9, 2024 00:27
@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/trim-linq-expressions branch 2 times, most recently from 1df5c4b to 19964df Compare January 10, 2024 00:17
@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/trim-linq-expressions branch from d25239f to 4fa67b3 Compare January 10, 2024 11:03
@Sergio0694
Copy link
Member Author

Can confirm that this fully removes the IL3050 warning from LINQ expressions when publishing! 🎉

image

Removing the assembly entirely is handled by #1435.

@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/trim-linq-expressions branch from 87d21e4 to 5f5ee04 Compare January 13, 2024 19:23
@Sergio0694
Copy link
Member Author

Worth noting, this also saves ~128 KB!


internal static unsafe Delegate GetValueDelegateForAbi()
{
if (typeof(T) == typeof(int) ||
Copy link
Member

Choose a reason for hiding this comment

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

So because of this, I feel like most of the types listed here won't come to this code path. Do you think having some specific nullable optimizations for each type is better or do you think have a generic version that handles all here is better? The main difference I see is with the former, we can list them as UnManagedCallersOnly whereas here we would need to do GetfunctionPointerForDelegate. Or should we keep both as is, and have this as a fallback in case that code path wasn't used?

Copy link
Member Author

@Sergio0694 Sergio0694 Jan 17, 2024

Choose a reason for hiding this comment

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

Oh, that's interesting, I didn't notice that. To clarify, all changes in this PR were solely driven by trying to fix all AOT warnings and allow all of that LINQ expression code to be trimmed out, basically I did the minimum amount of changes to satisfy that. I'm still seeing generic instantiations for all of these types, for instance:

image

So I'm inclined to say keeping both code paths can't hurt, and it might still help with trimming since it would make those generic instantiations get the specialized paths (even though we're not 100% sure they're actually executed at runtime). I would say perhaps let's just keep this for now and then we can potentially look into removing code once we've double checked that it is never in fact used anywhere, and also that it doesn't regress trimming? 🤔

What do you think?

"Do you think having some specific nullable optimizations for each type is better or do you think have a generic version that handles all here is better?"

Mmh good question. I can't say I have an exact answer here. It also depends on whether we'll be able to fully eliminate all other generic instantiations. If not, I wonder whether the additional code size and complexity is worth it. In theory there shouldn't be much overhead from just that single proxy delegate. But this is also something we could always revisit later, not a big deal I think either way.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, we can keep both for now.

Copy link
Member

Choose a reason for hiding this comment

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

Note that in your sceenshot, that is all BoxedIReferenceArrayImpl which is different from BoxedIReferenceImpl. I know we have worked to do with arrays possibly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh sorry, looked at the wrong line 😅
Still seeing some instantiations from the correct type too, though not for primitive types:

image

So it seems they're still "duplicate", but with less potentially unnecessary instantiations?

Copy link
Member Author

Choose a reason for hiding this comment

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

The type with a single type parameter does seem to only have one instantiation 🤔

image

Copy link
Member

Choose a reason for hiding this comment

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

For the first set, those come from here. I wasn't sure if I wanted to create specializations for each one so I had created a BoxedValueIreferenceImpl that takes both the type and the ABI type as generics so it was AOT compatible and had implemented it that way. With your changes, there might be opportunity in a future PR to make it again use the one generic one if that is better.

@Sergio0694
Copy link
Member Author

Updated diff, sizoscope doesn't report everything but the .dll-s are actually 310 KB smaller!! 🎉

image

@Sergio0694 Sergio0694 merged commit cf9f275 into staging/AOT Jan 18, 2024
9 checks passed
@Sergio0694 Sergio0694 deleted the user/sergiopedri/trim-linq-expressions branch January 18, 2024 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AOT authoring Related to authoring feature work performance Related to performance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants