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 unmanaged, ref #4

Open
AhmedZero opened this issue Apr 15, 2022 · 9 comments
Open

use unmanaged, ref #4

AhmedZero opened this issue Apr 15, 2022 · 9 comments

Comments

@AhmedZero
Copy link
Contributor

why don't use the ref keyword in the pointer parameter instead?
and function like ImGui.ListBox

        [DllImport("Mochi.DearImGui.Native", CallingConvention = CallingConvention.Cdecl, EntryPoint = "?ListBox@ImGui@@YA_NPEBDPEAHQEBQEBDHH@Z", ExactSpelling = true)]
        private static extern byte ListBox_PInvoke(byte* label, int* current_item, sbyte** items, int items_count, int height_in_items);

use UnmanagedType.LPArray and use string[];

        [DllImport("Mochi.DearImGui.Native", CallingConvention = CallingConvention.Cdecl, EntryPoint = "?ListBox@ImGui@@YA_NPEBDPEAHQEBQEBDHH@Z", ExactSpelling = true)]
        private static extern byte ListBox_PInvoke(byte* label, int* current_item, [MarshalAs(UnmanagedType.LPArray, ArraySubType = UnmanagedType.LPStr)]string[] items, int items_count, int height_in_items);
@PathogenDavid
Copy link
Member

Hey, sorry for the late response. Taxes consumed all spare energy I had this weekend.

why don't use the ref keyword in the pointer parameter instead?

By default everything is exposed roughly the same as it is in C++. Since this parameter is a pointer in C++ it's a pointer in C# too.

In theory one could write a transformation to automatically convert all pointer parameters to C# byrefs, but I'd be hesitant to do this because it's not always the best thing to do. (Particularly for APIs where the pointer is allowed to be null since I'd rather not assume people know about Unsafe.NullRef<T> or force them to use it for common APIs like ImGui::Begin.)

use UnmanagedType.LPArray and use string[];

In general we avoid using .NET's built-in marshaler because it's pretty bad performance-wise.

It's also being deprecated in .NET 7 in favor of more C#-centric approaches like the upcoming LibraryImportGenerator or Biohazrd.


In this specific case, I simply haven't had a chance to really look at the best way to expose this function in a more developer-friendly way.

In general I want the developer friendly wrappers to guide people towards a pit of success performance-wise, especially for a library where hundreds or even thousands of function calls are being done in a fraction of a 16ms window.

Since this is and ComboBox are the only APIs like this in Dear ImGui, I'd probably be inclined to just write human-friendly wrappers for them manually. (Probably using a shared UTF8 buffer and the function pointer overload instead of the string array one, but I'd want to sit down and think about it more.)

If you need something more friendly today, I'd probably just use BeginListBox/Selectable/EndListBox directly. (That's basically all ImGui::ListBox is anyway, minus some ImGuiListClipper logic which only really matters if your list box has a huge number of items -- in which case you probably need to special case things from C# anyway to avoid the overhead of constant UTF16 -> UTF8 conversions.)

@AhmedZero
Copy link
Contributor Author

I know about marshaller performance. Anyway, I simply use Unsafe.AsPointer instead of using a lot of fixed keywords.

@PathogenDavid
Copy link
Member

I would be careful about using Unsafe.AsPointer since it'd be easy to accidentally create a GC hole which can lead to very random crashes. You'd be better off making a helper method which exposes ref and uses fixed internally. (fixed has basically no performance overhead.)

@AhmedZero
Copy link
Contributor Author

I don't think it has a problem with static variables.
ldarg.0 conv.u ret

@PathogenDavid
Copy link
Member

I don't think it has a problem with static variables.

Today I learned. Not sure if that's an implementation detail I'd want to rely on, but neat nonetheless.

FYI, according to Pro .NET Memory that assumption only applies to primitive types. All user-defined structs used for static fields are allocated in boxes on the normal GC heap. (You can observe this pretty easily using this snippet.)

@AhmedZero
Copy link
Contributor Author

Yes it works well with primitive types.

@AhmedZero
Copy link
Contributor Author

I have an idea to make an option if anyone wants a ref keyword, generate it with the ref keyword.

@PathogenDavid
Copy link
Member

I still don't think it's a good idea to provide overloads that use ref for all pointer parameters across the whole API surface, so I would hesitate to provide an official option without additional extra care.

That being said if you'd like to experiment with doing so yourself, here's a quick and dirty transformation which adds such overloads:

using Biohazrd;
using Biohazrd.CSharp;
using Biohazrd.CSharp.Trampolines;
using Biohazrd.Transformation;
using System.Linq;

namespace Mochi.DearImGui.Generator;

// This transformation is pretty naive and many of the overloads it generates don't make any sense. Use at your own risk!
internal sealed class MakeRefOverloadsTransformation : TransformationBase
{
    protected override TransformationResult TransformFunction(TransformationContext context, TranslatedFunction declaration)
    {
        if (!declaration.Metadata.TryGet(out TrampolineCollection trampolines))
        { return declaration; }

        // Create trampolines which adapt pointers to C# byrefs.
        // We don't want to just loop over the entire trampoline collection because we don't want to adapt a non-primary native trampoline
        Trampoline? TryMakeTrampoline(Trampoline targetTrampoline)
        {
            TrampolineBuilder builder = new(targetTrampoline, useAsTemplate: true);

            foreach (Adapter adapter in targetTrampoline.Adapters)
            {
                // Skip parameters which don't accept input
                if (!adapter.AcceptsInput)
                { continue; }

                // If the parameter isn't a pointer we don't care about it
                if (adapter.InputType is not PointerTypeReference pointerType)
                { continue; }

                // If the parameter is a `const byte*`, we skip it since it's probably meant to be a string
                if (pointerType is { Inner: CSharpBuiltinTypeReference cSharpPointee, InnerIsConst: true } && cSharpPointee == CSharpBuiltinType.Byte)
                { continue; }

                // If the parameter is `void*` we skip it since `ref void` isn't valid C#
                if (pointerType.Inner is VoidTypeReference)
                { continue; }

                // If we got this far, adapt the parameter to a C# byref
                builder.AdaptParameter(adapter, new ByRefAdapter(adapter, ByRefKind.Ref));
            }

            return builder.HasAdapters ? builder.Create() : null;
        }

#if false
        // Handle pointer parameters in the primary trampoline
        {
            if (TryMakeTrampoline(trampolines.PrimaryTrampoline) is Trampoline trampoline)
            { declaration = declaration.WithSecondaryTrampoline(trampoline); }
        }

        // Handle pointer parameters in the secondary trampoline
        foreach (Trampoline secondaryTrampoline in trampolines.SecondaryTrampolines)
        {
            if (TryMakeTrampoline(secondaryTrampoline) is Trampoline trampoline)
            { declaration = declaration.WithSecondaryTrampoline(trampoline); }
        }
#else
        // Only adapt pointer parameters for the "last" trampoline
        // (This should be either the primary trampoline or the interpolated string handler overload. Not the most robust way to identify it but it gets the job done.)
        {
            if (TryMakeTrampoline(trampolines.Last()) is Trampoline trampoline)
            { declaration = declaration.WithSecondaryTrampoline(trampoline); }
        }
#endif

        return declaration;
    }
}

This transformation should be run immediately after ImGuiCreateStringWrappersTransformation.

IE: Modify the are around Program.cs:136 to be:

// ...
library = new ImGuiCreateStringWrappersTransformation().Transform(library);
library = new MakeRefOverloadsTransformation().Transform(library);
library = new StripUnreferencedLazyDeclarationsTransformation().Transform(library);
// ...

@AhmedZero
Copy link
Contributor Author

Thanks 😁.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants