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

AOT support with .Net 8? #3109

Open
2 tasks
GuybrushX opened this issue Jan 1, 2024 · 10 comments
Open
2 tasks

AOT support with .Net 8? #3109

GuybrushX opened this issue Jan 1, 2024 · 10 comments
Labels
question v2 For discussions, issues, etc... relavant for v2

Comments

@GuybrushX
Copy link

GuybrushX commented Jan 1, 2024

Required reading:

ToDos:

  • Annotate anywhere reflection is used.
  • Figure out how to refactor ConfigurationManager given it is heavy on reflection AND serialization
@tig tig added the question label Jan 3, 2024
@dodexahedron
Copy link
Collaborator

I've been thinking about this, as well.

There's a fair bit of reflection that would make it difficult to do, as-is, without, at minimum, use of attributes to exempt some code from trimming.

@tig @BDisp This, I think, is a pretty valuable goal to strive for in V2, or a later release, considering where and how the library is generally going to be used.

The biggest blocker for full AOT compatibility, right now, is reflection.

Both for AOT and just in general, I'd urge everyone to try to avoid reflection in the library itself if at all possible, even though sometimes it may result in an easier implementation.

At minimum, when it does get used, or when GetType or similar methods are called on a type not known at compile-time, I would urge everyone to at least decorate the containing method, property, or class with [RequiresUnreferencedCode], so that the compiler knows a given unit is incompatible with trimming.

That's a blunt instrument and doesn't fix the problem, but at least lets consumers know they cant use that code if they intend to AOT, because the compiler understands that attribute and will throw appropriate warnings/errors when used in AoT situations. And it also makes it easy to track down and address such code at a future time.

But there are also other attributes that can be used in reflection scenarios that allow more nuance in the compiler behavior, and which can actually allow the associated code to be trim-compatible, such as DynamicallyAccessedMembers.

@tig
Copy link
Collaborator

tig commented Jan 19, 2024

Both for AOT and just in general, I'd urge everyone to try to avoid reflection in the library itself if at all possible, even though sometimes it may result in an easier implementation.

I haz sad. Reflection is FUN. ;-)

But you're right, and I'll get over it.

We should add to the first post of this issue a list of known places we use reflection as Todo's.

@tig tig added the v2 For discussions, issues, etc... relavant for v2 label Jan 19, 2024
@dodexahedron
Copy link
Collaborator

Came across this today, which puts a lot of the relevant concepts in a real-world context.

https://sentry.engineering/blog/should-you-could-you-aot

@tig
Copy link
Collaborator

tig commented Jan 20, 2024

Came across this today, which puts a lot of the relevant concepts in a real-world context.

https://sentry.engineering/blog/should-you-could-you-aot

I learned a ton. Thanks.

Updated first post in this issue....

@dodexahedron
Copy link
Collaborator

So...

I brought this up in another issue, but AoT makes it even more relevant, now:

ConfigurationManager may not be prudent to spend significant effort on, for this, because the dotnet Configuration stuff does it all and is trim-friendly, since it uses code generation.

I know a lot of work went into that, and I certainly empathize there, but I think it is very worth considering either adapting it to use the built-in stuff or replacing it with the built-in stuff. Even just from the point of view of maintainability, not having to deal with an in-house configuration infrastructure would be a load off of the project and future development.

@dodexahedron
Copy link
Collaborator

dodexahedron commented Jan 25, 2024

Another biggie that isn't optional at all:

P/Invoke

All DllImport attributes and the methods they adorn need to be migrated to the new LibraryImport form, which uses source generation and is AoT compatible.

DllImport uses runtime code generation and will always be trimmed out as a result, making it completely broken.

It's a pretty simple change though.

Here are two examples:

public static partial class ConsoleKeyMapping {

#if !WT_ISSUE_8871_FIXED // https://github.com/microsoft/terminal/issues/8871
	/// <summary>
	/// Translates (maps) a virtual-key code into a scan code or character value, or translates a scan code into a virtual-key code.
	/// </summary>
	/// <param name="vk"></param>
	/// <param name="uMapType">
	/// If MAPVK_VK_TO_CHAR (2) - The uCode parameter is a virtual-key code and is translated into an un-shifted
	/// character value in the low order word of the return value. 
	/// </param>
	/// <param name="dwhkl"></param>
	/// <returns>An un-shifted character value in the low order word of the return value. Dead keys (diacritics)
	/// are indicated by setting the top bit of the return value. If there is no translation,
	/// the function returns 0. See Remarks.</returns>
        [LibraryImport("user32", EntryPoint = "MapVirtualKeyExW", StringMarshalling = StringMarshalling.Utf16)]
	internal static partial uint MapVirtualKeyEx (VK vk, uint uMapType, nint dwhkl);

	/// <summary>
	/// Retrieves the active input locale identifier (formerly called the keyboard layout).
	/// </summary>
	/// <param name="idThread">0 for current thread</param>
	/// <returns>The return value is the input locale identifier for the thread.
	/// The low word contains a Language Identifier for the input language
	/// and the high word contains a device handle to the physical layout of the keyboard.
	/// </returns>
	[LibraryImport("user32", EntryPoint = "GetKeyboardLayout", StringMarshalling = StringMarshalling.Utf16)]
	internal static partial  nint GetKeyboardLayout (nint idThread);
 ...
}

Key differences:

  • The containing class has to be partial (it's source generation after all).
  • The method itself is required to have an access modifier (source generation)
  • The method must be marked static partial (again, source generation)
    • extern goes away completely
  • It is not necessary or recommended to specify the file extension for the required library. The resolver will automatically attempt to use platform-specific rules.
  • It is also not necessary to include the 'lib' prefix or suffix for libraries on linux, as that is a permutation the runtime will try, if the no-prefix version doesn't exist. Just use the naked library name without lib prefix or suffix, and without extension.
  • I also opportunistically changed the IntPtrs to nint, which is the dotnet 7 and up way of doing it.

There's also a free performance benefit to doing it, even for non-AoT situations, as the old method is never eligible for most optimizations, whereas this method is, at compile-time, which is awesome. At minimum, it'll usually inline the calls, avoiding a stack push and potential context switch.

@dodexahedron
Copy link
Collaborator

Related to #3212, ReSharper is also capable of having custom patterns defined which are then treated as inspections.

This would allow, for example, defining a pattern that matches "DllImport" and flags it as an error, so that future uses are also prevented.

@tig
Copy link
Collaborator

tig commented Jan 25, 2024

Yep. I already started migrating to LibraryImport as part of

@dodexahedron
Copy link
Collaborator

I made some really interesting discoveries about DllImport, LibraryImport, and what all that stuff is REALLY doing....

I'll be posting a discussion for it. I'll link it here when it's posted...

Short version is DllImport may not be as unacceptable as the docs make it seem...

@dodexahedron
Copy link
Collaborator

Here's the discussion post:
#3238

I've posted the initial written code and the first level of analysis, with two more to come.

I think you'll find it pretty interesting....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question v2 For discussions, issues, etc... relavant for v2
Projects
Status: No status
Development

No branches or pull requests

3 participants