Add GetLParam<T> and GetLParamRef<T> by sharwell · Pull Request #835 · dotnet/winforms

sharwell · 2019-04-19T21:13:10Z

This pull request implements the new GetLParam<T> and GetLParamRef<T> (#792) as internal methods, and uses them to avoid marshaling overhead for existing value types.

The first commit is shared with #818.

danmoseley · 2019-04-21T06:50:06Z

cc @JeremyKuhne

zsd4yr · 2019-04-22T15:42:37Z

honestly LGTM; I don't really understand the implications however...

src/Common/src/UnsafeNativeMethods.cs

JeremyKuhne · 2019-04-23T18:11:19Z

@jkotas, @stephentoub, @jkoritzinsky Any feedback on using this ref T approach versus unsafe and T* (i.e. ->) to access struct members in native data? Are there any unobvious pro/cons? Thanks!

jkotas · 2019-04-23T20:32:13Z

I think it is better to stick to regular unmanaged pointers for interop. It makes the intent much more obvious. For example, I do think that the following change is an improvement?

NativeMethods.WINDOWPOS* wp = (NativeMethods.WINDOWPOS *)m.LParam;

to

ref NativeMethods.WINDOWPOS wp = ref m.GetLParamRef<NativeMethods.WINDOWPOS>();

danmoseley · 2019-04-27T20:41:18Z

@sharwell do you plan to recast this in terms of pointers, as requested by @jkotas above?

sharwell · 2019-04-28T01:24:34Z

@danmosemsft I'll take a look Monday

sharwell · 2019-04-28T20:11:13Z

@sharwell do you plan to recast this in terms of pointers, as requested by @jkotas above?

I investigated this change this morning, and my conclusion is I do not believe the change leads to an improved outcome. There is no pointer representation equivalent to ref readonly, and I view the readonly constraint as a valuable restriction on usage.

Managed references also does a better job of restricting the locations where unsafe operations are used.

codecov · 2019-04-28T20:26:53Z

Codecov Report

Merging #835 into master will increase coverage by 0.00387%.
The diff coverage is 16.58537%.

@@                 Coverage Diff                 @@
##              master        #835         +/-   ##
===================================================
+ Coverage   28.83584%   28.83972%   +0.00388%     
===================================================
  Files           1078        1078                 
  Lines         293919      293921          +2     
  Branches       38408       38408                 
===================================================
+ Hits           84754       84766         +12     
+ Misses        205131      205118         -13     
- Partials        4034        4037          +3

Flag	Coverage Δ
#Debug	`28.83972% <16.58537%> (+0.00387%)`	⬆️
#production	`18.75421% <16.58537%> (+0.00452%)`	⬆️
#test	`98.68151% <ø> (ø)`	⬆️

jkotas · 2019-04-29T06:43:29Z

Conclusion is I do not believe the change leads to an improved outcome.

It is matter of personal preference. Here is a quick summary of pros and cons:

Managed references:

More overhead (e.g. each instantiation of GetLParamRef is several hundred bytes of memory, conversions from pointers to byrefs turn the GC tracking on that increases size of GCInfo and generates worse code here and there because of that)
Unsafe is hidden.
Able to use readonly.

Unmanaged pointers:

Leaner and faster
Unsafe is explicit. The unsafe code looks more like C/C++, less like C#.
Unable to use readonly.

I prefer leaner and faster code with explicit unsafe because of it is much easier to see that is going on. I believe it is also where the CoreCLR/CoreFX repos are leaning towards.

If other folks prefer the opposite and understand consequences of such preference, I do not have a problem with it. Ultimately, it is up to the WinForms maintainers to make the choice.

sharwell · 2019-04-29T12:20:54Z

I believe the perceived magnitude of this downside differs between us:

... The unsafe code looks more like C/C++, less like C#.

The pointers don't represent the way I would write (or suggest others write) C#. It doesn't mean it can't be done that way (it's a matter of style, not correctness), but if managed references aren't a possibility here it would likely save a bunch of time to go ahead and close this.

More overhead (e.g. each instantiation of GetLParamRef is several hundred bytes of memory, conversions from pointers to byrefs turn the GC tracking on that increases size of GCInfo and generates worse code here and there because of that)

I would expect this to improve over time as managed references become the preferred style for users.

JeremyKuhne · 2019-04-29T18:05:01Z

I prefer leaner and faster code with explicit unsafe because of it is much easier to see that is going on.

I agree with this when it comes to direct interop with native code, particularly at these low levels of the stack. I believe there is value in giving uplevel code the best performance possible. On top of performance, the obfuscated "unsafeness" concerns me when it comes to long term maintenance. You can mitigate that somewhat by liberally putting Unsafe in methods that do unsafe things. If WinForms is ok with using ref instead I would highly recommend using System.Runtime.CompilerServices.Unsafe.AsRef directly (or at worst, remove the indirection of GetLParamRef<T> or get "Unsafe" somewhere in the name).

I would expect this to improve over time as managed references become the preferred style for users.

Quite possibly, but we should be very cautious about taking hits based on potential future improvements.

Ultimately, it is up to the WinForms maintainers to make the choice.

For the record, that isn't me. I'm on the CoreFx team, and I'm here because I've been spending quite a bit of time on interop and I also am very interested in seeing interop improved in WinForms and WPF.

sharwell · 2019-04-29T23:19:38Z

You can mitigate that somewhat by liberally putting Unsafe in methods that do unsafe things.

I wouldn't be opposed to this. We can't use Unsafe.AsRef directly because it doesn't have an overload that takes an IntPtr.

JeremyKuhne · 2019-04-29T23:28:29Z

We can't use Unsafe.AsRef directly because it doesn't have an overload that takes an IntPtr.

Well, not without adding unsafe and either casting to (void*) or calling .ToPointer(). ;P

tannergooding · 2019-04-29T23:35:10Z

I would expect this to improve over time as managed references become the preferred style for users.

I don't think this will ever get as slim as raw pointers. You also have other issues, such as not being able to accurately handle/represent things like null.

tannergooding · 2019-04-29T23:39:40Z

src/System.Windows.Forms/src/System/Windows/Forms/Control.cs

-                if(nmhdr->code == NativeMethods.TTN_SHOW) {
-                    m.Result = UnsafeNativeMethods.SendMessage(new HandleRef(null, nmhdr->hwndFrom), Interop.WindowMessages.WM_REFLECT + m.Msg, m.WParam, m.LParam);
+        private void WmNotify(ref Message m) {
+            ref readonly NativeMethods.NMHDR nmhdr = ref m.GetLParamRef<NativeMethods.NMHDR>();


Is there actually any guarantee that the underlying nmhdr is readonly or are you just using this to say the local WmNotify shouldn't modify the state?

tannergooding · 2019-04-29T23:41:28Z

src/System.Windows.Forms/src/System/Windows/Forms/ImageList.cs

+            {
+                IntPtr srcPtr, destPtr;
+                if (IntPtr.Size == 4) {
+                    srcPtr = new IntPtr(sourceData.Scan0.ToInt32() + offsetSrc);


IntPtr has a operator +(IntPtr pointer, int offset) method.

zsd4yr · 2019-05-06T16:39:30Z

hey @sharwell , i kind of fell out of touch here while i was OOF; how is this going?

zsd4yr · 2019-05-10T20:50:43Z

@jkotas , @tannergooding , and @JeremyKuhne -- could one of you please approve when your perspective is satisfied?

@sharwell pinging again 😄

tannergooding · 2019-05-10T22:06:45Z

@zsd4yr, I think the perspective given so far was that this PR is largely unnecessary and sticking with raw pointers/unsafe code is probably better (e.g: #835 (comment)).

sharwell requested a review from a team as a code owner April 19, 2019 21:13

zsd4yr mentioned this pull request Apr 22, 2019

[API Addition] Message.GetLParam<T>() #792

Closed

JeremyKuhne reviewed Apr 23, 2019

View reviewed changes

src/Common/src/UnsafeNativeMethods.cs Outdated Show resolved Hide resolved

sharwell added 12 commits April 28, 2019 15:14

Add generic APIs Message.GetLParam and GetLParamRef

3a3bd94

Use GetLParamRef instead of GetLParam for NMHDR

e729665

Use GetLParamRef instead of pointers for NMHDR

f549946

Pass NMHDR by readonly reference

885388d

Use GetLParamRef for NMLISTVIEW

4b0e59b

Use generic GetLParam for NMLVFINDITEM

6836b3a

Use GetLParamRef for NMTOOLBAR

8a971e8

Use GetLParamRef for NMTBHOTITEM

39f5cc4

Use GetLParamRef and readonly references for TV_ITEM

3faa76f

Use readonly references for NMTREEVIEW

0bfd4c5

Use references instead of pointers for LParam access

3a783fd

Remove unnecessary use of 'unsafe'

5fd53ba

sharwell force-pushed the getlparam branch from 217bdab to 5fd53ba Compare April 28, 2019 20:17

sharwell mentioned this pull request Apr 28, 2019

Use Unsafe.AsRef to implement UnsafeNativeMethods.PtrToRef #898

Merged

tannergooding reviewed Apr 29, 2019

View reviewed changes

zsd4yr closed this May 10, 2019

ghost locked as resolved and limited conversation to collaborators Feb 6, 2022

Conversation

sharwell commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Apr 21, 2019

Uh oh!

zsd4yr commented Apr 22, 2019

Uh oh!

Uh oh!

JeremyKuhne commented Apr 23, 2019

Uh oh!

jkotas commented Apr 23, 2019 • edited by zsd4yr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Apr 27, 2019

Uh oh!

sharwell commented Apr 28, 2019

Uh oh!

sharwell commented Apr 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 28, 2019

Codecov Report

Uh oh!

jkotas commented Apr 29, 2019

Uh oh!

sharwell commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyKuhne commented Apr 29, 2019

Uh oh!

sharwell commented Apr 29, 2019

Uh oh!

JeremyKuhne commented Apr 29, 2019

Uh oh!

tannergooding commented Apr 29, 2019

Uh oh!

tannergooding Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

tannergooding Apr 29, 2019

Choose a reason for hiding this comment

Uh oh!

zsd4yr commented May 6, 2019

Uh oh!

zsd4yr commented May 10, 2019

Uh oh!

tannergooding commented May 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sharwell commented Apr 19, 2019 •

edited

Loading

jkotas commented Apr 23, 2019 •

edited by zsd4yr

Loading

sharwell commented Apr 28, 2019 •

edited

Loading

sharwell commented Apr 29, 2019 •

edited

Loading