Introduce AOT friendly version of ToLogFont and FromLogFont #70224

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

@@ -291,6 +291,34 @@ public unsafe void ToLogFont(object logFont, Graphics graphics)
            }
        }

+        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 <[email protected]>

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 the community-contribution Indicates that the PR has been added by a community member label Jun 3, 2022

dotnet-issue-labeler bot added the area-System.Drawing label 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 <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce AOT friendly version of ToLogFont and FromLogFont #70224

Introduce AOT friendly version of ToLogFont and FromLogFont #70224

kant2002 commented Jun 3, 2022 •

edited by ViktorHofer

Loading

ghost commented Jun 3, 2022

kant2002 commented Jun 3, 2022

Wraith2 Jun 5, 2022 •

edited

Loading

kant2002 Jun 5, 2022

Wraith2 Jun 5, 2022

Wraith2 Jun 5, 2022 •

edited

Loading

kant2002 Jun 5, 2022

ViktorHofer Jun 5, 2022

kant2002 Jun 5, 2022

ViktorHofer Jun 6, 2022

kant2002 Jun 7, 2022

RussKie Jun 6, 2022

ViktorHofer Jun 6, 2022 •

edited

Loading

kant2002 Jun 9, 2022

ViktorHofer Jun 9, 2022

kant2002 Jun 9, 2022

jkotas Jun 6, 2022

kant2002 Jun 6, 2022

kant2002 Jun 6, 2022

RussKie Jun 6, 2022

RussKie Jun 6, 2022

JeremyKuhne Jun 6, 2022

jkotas Jun 6, 2022

jkotas Jun 7, 2022

jkotas Jun 7, 2022

jkotas Jun 7, 2022

jkotas Jun 7, 2022

kant2002 Jun 8, 2022

carlossanlop commented Jun 29, 2022

danmoseley commented Jul 25, 2022

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

Introduce AOT friendly version of ToLogFont and FromLogFont #70224

Introduce AOT friendly version of ToLogFont and FromLogFont #70224

Conversation

kant2002 commented Jun 3, 2022 • edited by ViktorHofer Loading

ghost commented Jun 3, 2022

kant2002 commented Jun 3, 2022

Wraith2 Jun 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wraith2 Jun 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlossanlop commented Jun 29, 2022

danmoseley commented Jul 25, 2022

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