Terminal.Gui icon indicating copy to clipboard operation
Terminal.Gui copied to clipboard

AOT support with .Net 8?

Open GuybrushX opened this issue 1 year ago • 10 comments

Required reading:

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

ToDos:

  • [ ] Annotate anywhere reflection is used.
  • [ ] Figure out how to refactor ConfigurationManager given it is heavy on reflection AND serialization

GuybrushX avatar Jan 01 '24 10:01 GuybrushX

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.

dodexahedron avatar Jan 19 '24 06:01 dodexahedron

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 avatar Jan 19 '24 15:01 tig

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

dodexahedron avatar Jan 19 '24 23:01 dodexahedron

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....

tig avatar Jan 20 '24 16:01 tig

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 avatar Jan 22 '24 20:01 dodexahedron

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 avatar Jan 25 '24 04:01 dodexahedron

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.

dodexahedron avatar Jan 25 '24 04:01 dodexahedron

Yep. I already started migrating to LibraryImport as part of

  • #2610

tig avatar Jan 25 '24 04:01 tig

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 avatar Feb 11 '24 07:02 dodexahedron

Here's the discussion post: https://github.com/gui-cs/Terminal.Gui/discussions/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....

dodexahedron avatar Feb 11 '24 08:02 dodexahedron