Introduce AOT friendly version of ToLogFont and FromLogFont by kant2002 · Pull Request #70224 · dotnet/runtime

kant2002 · 2022-06-03T21:35:42Z

These methods used by WinForms

ghost · 2022-06-03T21:35:50Z

Tagging subscribers to this area: @dotnet/area-system-drawing
See info in area-owners.md if you want to be subscribed.

Issue Details

These methods used by WinForms

Author:	kant2002
Assignees:	-
Labels:	`area-System.Drawing`, `community-contribution`
Milestone:	-

kant2002 · 2022-06-03T21:36:57Z

/cc @RussKie in case public surface should be modified to be able to use this change.

Wraith2 · 2022-06-05T01:29:36Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

+            Interop.User32.LOGFONT nativeLogFont = ToLogFontInternal(graphics);
+
+            // PtrToStructure requires that the passed in object not be a value type.
+            if (!type.IsValueType)


You can remove the type variable and use typeof(T).IsValueType here.

I do even better, and put constraints on T to be struct. I do not think it's reasonable to allow objects if you are caring about trimmability.

Wraith2 · 2022-06-05T01:30:26Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

+                GCHandle handle = GCHandle.Alloc(logFont, GCHandleType.Pinned);
+                Buffer.MemoryCopy(&nativeLogFont, (byte*)handle.AddrOfPinnedObject(), nativeSize, nativeSize);
+                handle.Free();


Try finally to ensure that free is called?

Wraith2 · 2022-06-05T01:31:07Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

+        /// </summary>
+        /// <param name="lf">A boxed LOGFONT.</param>
+        /// <returns>The newly created <see cref="Font"/>.</returns>
+        public static Font FromLogFont<T>(T lf)


lf -> logFont ?

Thank you. It's easy to be lazy.

ViktorHofer · 2022-06-05T08:52:27Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

            }
        }

+        public unsafe void ToLogFont<T>(T logFont, Graphics graphics)


Aren't you changing the public API surface here? Shouldn't this go through a API design review meeting? System.Drawing.Common ships as a package which doesn't contain a contract assembly and hence customers would start seeing this method.

Honestly I'm not much experienced in this process, so decide to put some code. Should I create separate issue for API proposal?

Here's the API review process: https://github.com/dotnet/runtime/blob/main/docs/project/api-review-process.md.

https://github.com/dotnet/runtime/issues/70358 Submitted

RussKie · 2022-06-06T00:08:21Z

src/libraries/System.Drawing.Common/ref/System.Drawing.Common.netcoreapp.cs

+    public sealed partial class Font
+    {
+        public void ToLogFont<T>(ref T logFont, Graphics graphics) where T: struct  { throw null; }
+        public static Font FromLogFont<T>(in T logFont) where T: struct { throw null; }
+        public static Font FromLogFont<T>(in T logFont, IntPtr hdc) where T: struct { throw null; }
+        public void ToLogFont<T>(ref T logFont) where T: struct { throw null; }
+    }


Shouldn't this be conditionalised with #if NET7_0_OR_GREATER?

/__w/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22255.2/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : Compat issues with assembly System.Drawing.Common: [/__w/1/s/src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj] ##[error].packages/microsoft.dotnet.apicompat/7.0.0-beta.22255.2/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Compat issues with assembly System.Drawing.Common: /__w/1/s/.packages/microsoft.dotnet.apicompat/7.0.0-beta.22255.2/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : MembersMustExist : Member 'public System.Drawing.Font System.Drawing.Font.FromLogFont<T>(T)' does not exist in the implementation but it does exist in the contract. [/__w/1/s/src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj] ##[error].packages/microsoft.dotnet.apicompat/7.0.0-beta.22255.2/build/Microsoft.DotNet.ApiCompat.targets(52,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) MembersMustExist : Member 'public System.Drawing.Font System.Drawing.Font.FromLogFont<T>(T)' does not exist in the implementation but it does exist in the contract.

Is there a reason to enable the new APIs only for net7.0 (in both the ref and impl) instead of net6.0 and upwards?

I simply think that adding new API to .Net 7 would be less of a problem then anything else. Nothing prevent this API to work in .NET 6,

When I as a customer intentionally upgrade my System.Drawing.Common package reference from 6.0.0 to 7.0.0, I would expect that I get new features irrelevant of the underlying target framework that is being used. I think it makes sense to improve both the net6.0 and the net7.0 assets if there's no underlying reason to not do so.

That's good point. I forget about that scenario, even if I use it all the time.

jkotas · 2022-06-06T04:23:44Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

+            Interop.User32.LOGFONT nativeLogFont = ToLogFontInternal(graphics);
+
+            // PtrToStructure requires that the passed in object not be a value type.
+            Marshal.PtrToStructure<T>(new IntPtr(&nativeLogFont), logFont);


It would be best if these new APIs did not depend on the built-in runtime marshalling support at all.

Can these new APIs take void* that points to LOGFONT and require the callers to take of the marshalling if there is any marshalling required?

Would change like this logFont = *(T*)&nativeLogFont; works?

I think I get why you propose void*, because any struct which given to method can have managed fields, then I cannot copy data over layout. @RussKie can we just adopt these small methods to WinForms in appropriate places, having new void* methods in System.Drawing.Common are probably not worth goal to pursue

can we just adopt these small methods to WinForms in appropriate places, having new void* methods in System.Drawing.Common are probably not worth goal to pursue

We can surely consider that.

Looping in @JeremyKuhne as he may have thoughts on the subject as well.

Have we considered the unmanaged constraint? Then you can do everything without Marshal. @jkotas?

unmanaged constraint works for me.

jkotas · 2022-06-07T16:52:26Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

+            ArgumentNullException.ThrowIfNull(logFont);
+
+            int nativeSize = sizeof(Interop.User32.LOGFONT);
+            if (Marshal.SizeOf<T>() != nativeSize)


Suggested change

if (Marshal.SizeOf<T>() != nativeSize)

if (sizeof(T) != nativeSize)

It is not correct to use Marshal.SizeOf here

jkotas · 2022-06-07T16:53:36Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

+            Interop.User32.LOGFONT nativeLogFont = ToLogFontInternal(graphics);
+
+            // PtrToStructure requires that the passed in object not be a value type.
+            logFont = *(T*)&nativeLogFont;


Suggested change

Interop.User32.LOGFONT nativeLogFont = ToLogFontInternal(graphics);

// PtrToStructure requires that the passed in object not be a value type.

logFont = *(T*)&nativeLogFont;

*(Interop.User32.LOGFONT*)&logFont = ToLogFontInternal(graphics);

Avoid extra copy

jkotas · 2022-06-07T16:54:30Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

+            }
+
+            int nativeSize = sizeof(Interop.User32.LOGFONT);
+            if (Marshal.SizeOf<T>() != nativeSize)


jkotas · 2022-06-07T16:54:43Z

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

+            // Now that we know the marshalled size is the same as LOGFONT, copy in the data
+            nativeLogFont = default;
+
+            *(T*)&nativeLogFont = logFont;


Not sure about this one.

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

carlossanlop · 2022-06-29T17:01:28Z

Needs the API proposal to be approved first.

danmoseley · 2022-07-25T16:34:55Z

I'm going to close this PR pending API approval. (This is what we generally do. It's easy to reopen the PR when API is approved.)

Introduce AOT friendly version of ToLogFont and FromLogFont

a179a8f

These methods used by WinForms

ghost added community-contribution Indicates that the PR has been added by a community member area-System.Drawing labels Jun 3, 2022

Wraith2 reviewed Jun 5, 2022

View reviewed changes

ViktorHofer reviewed Jun 5, 2022

View reviewed changes

kant2002 added 5 commits June 5, 2022 16:55

Apply PR feedback

80aaf66

Fix compiltion in refs

772e4bf

Fix compiltion errors

254005c

Finally?

7d28003

Have no idea why this was failing.

fe75151

RussKie reviewed Jun 6, 2022

View reviewed changes

Attempt again

74121e0

jkotas reviewed Jun 6, 2022

View reviewed changes

Get rid of Marshal.PtrStructure

fa23e49

kant2002 mentioned this pull request Mar 14, 2023

[API Proposal]: Trim friendly Font ToLogFont/FromLogFont dotnet/winforms#8846

Closed

jkotas reviewed Jun 7, 2022

View reviewed changes

Fix suggestions

d57baa5

jkotas reviewed Jun 8, 2022

View reviewed changes

src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs Outdated Show resolved Hide resolved

kant2002 and others added 3 commits June 8, 2022 18:43

Update src/libraries/System.Drawing.Common/src/System/Drawing/Font.cs

0b14a96

Co-authored-by: Jan Kotas <jkotas@microsoft.com>

Remove reference

e9198d4

Fix constraints

300304b

runfoapp bot mentioned this pull request Jun 14, 2022

system.net.http.functional.tests crashing #70757

Closed

kant2002 mentioned this pull request Jun 17, 2022

More trimming friendly DragDrop dotnet/winforms#6745

Closed

carlossanlop added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 29, 2022

danmoseley closed this Jul 25, 2022

ghost locked as resolved and limited conversation to collaborators Aug 24, 2022

	if (Marshal.SizeOf<T>() != nativeSize)
	if (sizeof(T) != nativeSize)

Conversation

kant2002 commented Jun 3, 2022 • edited by ViktorHofer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 3, 2022

Uh oh!

kant2002 commented Jun 3, 2022

Uh oh!

Wraith2 Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wraith2 Jun 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Jun 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlossanlop commented Jun 29, 2022

Uh oh!

danmoseley commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

kant2002 commented Jun 3, 2022 •

edited by ViktorHofer

Loading

Wraith2 Jun 5, 2022 •

edited

Loading

Wraith2 Jun 5, 2022 •

edited

Loading

ViktorHofer Jun 6, 2022 •

edited

Loading