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

Use GeneratedDllImport in System.Console #52740

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented May 14, 2021

Library Original (B) Converted (B) + (B) + (%)
linux 74752 80384 5632 7.53
windows 65536 73728 8192 12.50
linux R2R 179712 196096 16384 9.12
windows R2R 146432 167936 21504 14.69

cc @AaronRobinsonMSFT @jkoritzinsky

@@ -14,7 +14,12 @@ internal static partial class Kernel32

internal delegate bool ConsoleCtrlHandlerRoutine(int controlType);

#if DLLIMPORTGENERATOR_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

Who is setting this define? I don't see it set anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Is the DllImportGenerator going to be an "opt-in" feature? Or will it be built-in to a user's project by default when they target net6.0+?

If it is the latter, then this define constant doesn't seem to add much value, since every net6.0+ project will get it by default.

Copy link
Member

Choose a reason for hiding this comment

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

The current plan is for DllImportGenerator to be "opt-in" and for .NET 6.0 to be used internally only.

The point of the define is to enable a well-defined constant so we can provide a code fix that conditionally changes the code to use GeneratedDllImport when on .NET 6.0 and still use DllImport downlevel.

Copy link
Member Author

@elinor-fung elinor-fung May 14, 2021

Choose a reason for hiding this comment

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

At the moment, it is not going to be exposed to users for net6.0 at all. We are just targeting runtime right now, but it would be opt-in later.

The define is an attempt to make it easier for multi-targeting scenarios - like this file that also gets used for non-net6.0+.

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadStdin", SetLastError = true)]
internal static extern unsafe int ReadStdin(byte* buffer, int bufferSize);
[GeneratedDllImport(Libraries.SystemNative, EntryPoint = "SystemNative_ReadStdin", SetLastError = true)]
internal static unsafe partial int ReadStdin(byte* buffer, int bufferSize);

[DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_InitializeConsoleBeforeRead")]
internal static extern void InitializeConsoleBeforeRead(byte minChars = 1, byte decisecondsTimeout = 0);
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't we using GeneratedDllImport on InitializeConsoleBeforeRead and UninitializeConsoleAfterRead?

Copy link
Member Author

Choose a reason for hiding this comment

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

We only targeted the ones that had non-blittable types or had settings that would require runtime handling (SetLastError=true for example), since the others would already be inline-able.

@elinor-fung elinor-fung force-pushed the convert-SystemConsole branch from f7a9d35 to 74dc0e2 Compare May 19, 2021 19:39
@elinor-fung elinor-fung merged commit 9f32647 into dotnet:feature/use-dllimport-generator May 20, 2021
@elinor-fung elinor-fung deleted the convert-SystemConsole branch May 20, 2021 23:29
@ghost ghost locked as resolved and limited conversation to collaborators Jun 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants