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

Avoid pinning this in instance method for struct COM objects #972

Closed
Sergio0694 opened this issue Jun 22, 2023 · 1 comment · Fixed by #975
Closed

Avoid pinning this in instance method for struct COM objects #972

Sergio0694 opened this issue Jun 22, 2023 · 1 comment · Fixed by #975
Assignees
Labels
enhancement New feature or request

Comments

@Sergio0694
Copy link
Member

Sergio0694 commented Jun 22, 2023

Overview

I'm trying out CsWin32 with the "allowMarshaling": false mode enabled, which generates blittable struct types for all COM types, instead of interfaces. That is great, but I noticed the generated code is leaving a bit of performance on the table. Specifically:

This is an example, but the same applies to all instance methods across all generated types. Pinning this is actually not needed, because the actual value will always be on native memory (since users would always just receive a pointer to a native COM object).

We can improve this by instead generating:

public uint AddRef()
{
    return ((delegate *unmanaged [Stdcall]<IUnknown*,uint>)lpVtbl[1])((IUnknown*)Unsafe.AsPointer(ref this));
}

Note: this is the same thing also done by eg. TerraFX.Interop.Windows, ComputeSharp and other libraries.

Open questions

  • We might want this gated behind a separate JSON flag maybe? But I'd argue this should just always be the case. If anyone ever copied the actual value pointed to by a pointer to a COM object, that would most definitely just be a bug.
  • We might want to only do this if Unsafe.AsPointer is available on the target project (which can be checked easily enough).

Context

  • CsWin32 version: 0.3.2-beta
  • Target Framework: tested on .NET 8 Preview 5 and .NET Standard 2.0
  • LangVersion (if explicitly set by project): 11
@Sergio0694 Sergio0694 added the bug Something isn't working label Jun 22, 2023
@AArnott
Copy link
Member

AArnott commented Jun 23, 2023

Thanks for the tip.

@AArnott AArnott added enhancement New feature or request and removed bug Something isn't working labels Jun 23, 2023
@AArnott AArnott self-assigned this Jun 23, 2023
AArnott added a commit that referenced this issue Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants