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

String marshallers for UTF-16 and UTF-8 #249

Merged
merged 7 commits into from
Oct 21, 2020

Conversation

elinor-fung
Copy link
Member

@elinor-fung elinor-fung commented Oct 19, 2020

  • String marshallers for UTF-16 and UTF-8
  • Require explicit marshalling information (CharSet or MarshalAs) for char or string
    • See updated compatibility doc
  • Add CharEncoding.Ansi
    • Not supported for char, support for string will be added in another change
  • Add and update tests

There is currently a good amount of duplication between Utf16StringMarshaller and Utf8StringMarshaller. I'm planning on doing some refactoring to pull out common bits when I deal with CharSet.Ansi and CharSet.Auto in a subsequent change.

cc @AaronRobinsonMSFT @jkoritzinsky

UTF-16:

public static partial string Method(string p, in string pIn, ref string pRef, out string pOut)
{
    unsafe
    {
        pOut = default;
        string __retVal;
        //
        // Setup
        //
        ushort *__retVal_gen_native;
        ushort *__pIn_gen_native;
        ushort *__pRef_gen_native;
        ushort *__pOut_gen_native;
        //
        // Marshal
        //
        bool pIn__usedCoTaskMem = false;
        if (pIn == null)
        {
            __pIn_gen_native = null;
        }
        else
        {
            int pIn__byteLen = (pIn.Length + 1) * sizeof(ushort);
            if (pIn__byteLen > 520)
            {
                __pIn_gen_native = (ushort *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUni(pIn);
                pIn__usedCoTaskMem = true;
            }
            else
            {
                ushort *pIn__stackalloc = stackalloc ushort[pIn.Length + 1];
                ((System.ReadOnlySpan<char>)pIn).CopyTo(new System.Span<char>(pIn__stackalloc, pIn.Length + 1));
                __pIn_gen_native = pIn__stackalloc;
            }
        }

        __pRef_gen_native = (ushort *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUni(pRef);
        //
        // Invoke
        //
        fixed (char *__p_gen_native = p)
        {
            __retVal_gen_native = Method__PInvoke__((ushort *)__p_gen_native, &__pIn_gen_native, &__pRef_gen_native, &__pOut_gen_native);
        }

        //
        // Unmarshal
        //
        __retVal = __retVal_gen_native == null ? null : new string ((char *)__retVal_gen_native);
        pRef = __pRef_gen_native == null ? null : new string ((char *)__pRef_gen_native);
        pOut = __pOut_gen_native == null ? null : new string ((char *)__pOut_gen_native);
        //
        // Cleanup
        //
        if (pIn__usedCoTaskMem)
        {
            System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__pIn_gen_native);
        }

        System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__pRef_gen_native);
        System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__pOut_gen_native);
        return __retVal;
    }

    [System.Runtime.InteropServices.DllImportAttribute("DoesNotExist", CharSet = System.Runtime.InteropServices.CharSet.Unicode, EntryPoint = "Method")]
    extern private static unsafe ushort *Method__PInvoke__(ushort *p, ushort **pIn, ushort **pRef, ushort **pOut);
}

UTF-8:

public static partial string Method(string p, in string pIn, ref string pRef, out string pOut)
{
    unsafe
    {
        pOut = default;
        string __retVal;
        //
        // Setup
        //
        byte *__retVal_gen_native;
        byte *__p_gen_native;
        byte *__pIn_gen_native;
        byte *__pRef_gen_native;
        byte *__pOut_gen_native;
        //
        // Marshal
        //
        bool p__usedCoTaskMem = false;
        if (p == null)
        {
            __p_gen_native = null;
        }
        else
        {
            int p__byteLen = (p.Length + 1) * 3 + 1;
            if (p__byteLen > 260)
            {
                __p_gen_native = (byte *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUTF8(p);
                p__usedCoTaskMem = true;
            }
            else
            {
                byte *p__stackalloc = stackalloc byte[p__byteLen];
                p__byteLen = System.Text.Encoding.UTF8.GetBytes(p, new System.Span<byte>(p__stackalloc, p__byteLen));
                p__stackalloc[p__byteLen] = 0;
                __p_gen_native = (byte *)p__stackalloc;
            }
        }

        bool pIn__usedCoTaskMem = false;
        if (pIn == null)
        {
            __pIn_gen_native = null;
        }
        else
        {
            int pIn__byteLen = (pIn.Length + 1) * 3 + 1;
            if (pIn__byteLen > 260)
            {
                __pIn_gen_native = (byte *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUTF8(pIn);
                pIn__usedCoTaskMem = true;
            }
            else
            {
                byte *pIn__stackalloc = stackalloc byte[pIn__byteLen];
                pIn__byteLen = System.Text.Encoding.UTF8.GetBytes(pIn, new System.Span<byte>(pIn__stackalloc, pIn__byteLen));
                pIn__stackalloc[pIn__byteLen] = 0;
                __pIn_gen_native = (byte *)pIn__stackalloc;
            }
        }

        __pRef_gen_native = (byte *)System.Runtime.InteropServices.Marshal.StringToCoTaskMemUTF8(pRef);
        //
        // Invoke
        //
        __retVal_gen_native = Method__PInvoke__(__p_gen_native, &__pIn_gen_native, &__pRef_gen_native, &__pOut_gen_native);
        //
        // Unmarshal
        //
        __retVal = System.Runtime.InteropServices.Marshal.PtrToStringUTF8((System.IntPtr)__retVal_gen_native);
        pRef = System.Runtime.InteropServices.Marshal.PtrToStringUTF8((System.IntPtr)__pRef_gen_native);
        pOut = System.Runtime.InteropServices.Marshal.PtrToStringUTF8((System.IntPtr)__pOut_gen_native);
        //
        // Cleanup
        //
        if (p__usedCoTaskMem)
        {
            System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__p_gen_native);
        }

        if (pIn__usedCoTaskMem)
        {
            System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__pIn_gen_native);
        }

        System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__pRef_gen_native);
        System.Runtime.InteropServices.Marshal.FreeCoTaskMem((System.IntPtr)__pOut_gen_native);
        return __retVal;
    }
}

[System.Runtime.InteropServices.DllImportAttribute("DoesNotExist", EntryPoint = "Method")]
extern private static unsafe byte *Method__PInvoke__(byte *p, byte **pIn, byte **pRef, byte **pOut);

@elinor-fung elinor-fung added the area-DllImportGenerator Source Generated stubs for P/Invokes in C# label Oct 19, 2020
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments around some possible refactoring and maybe using Span in the generated code.

// else
// {
// int <byteLenIdentifier> = (<managedIdentifier>.Length + 1) * sizeof(ushort);
// if (<byteLenIdentifier> > <StackAllocBytesThreshold>)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we want to abstract out the conditional stackalloc concept to a base class? We already do this twice and arrays will do this as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that is the plan - was going to do it in a subsequent change

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this would just call dotnet/runtime#25423

@AaronRobinsonMSFT
Copy link
Member

@elinor-fung and @jkoritzinsky From now on when a new IMarshallingGenerator is added how about we provide the generated code snippet in the PR? The output from the T Method(T, in T, ref T, out T) unit test should be enough to have a conversation.

Thoughts?

@jkoritzinsky
Copy link
Member

That sounds good to me.

@jkotas
Copy link
Member

jkotas commented Oct 20, 2020

These marshalers are not performance critical. It is not important to inline the logic for performance reasons for them. Would it be better to build them using the standard struct-based marshallers that wrap the logic? The marshaling logic can then be optimized independently with ease.

@AaronRobinsonMSFT
Copy link
Member

... using the standard struct-based marshallers that wrap the logic?

@jkotas Can you elaborate on this? Are we referencing the internal StubHelpers class or is this a well-known pattern?

@jkotas
Copy link
Member

jkotas commented Oct 20, 2020

I meant the MarshalUsing attribute from https:/dotnet/runtimelab/blob/DllImportGenerator/DllImportGenerator/designs/StructMarshalling.md.

The idea would be to transform void F([MarshalAs(UnmanagedType.LPUTF8Str)] string s) to void F([MarshalUsing(UTF8StringMarshaler)] string s) and the rest would just fallout naturally.

@AaronRobinsonMSFT
Copy link
Member

@jkotas I like this approach for V2 as an extension. For V1, my concern would be where would this and other types live? Pulling this into NetCoreApp would require these types eventually live in SPCL, which is something I would like to avoid at all costs. For V2, we could provide an extension library with these types and that is something I am all for.

Does my concern make sense or am I missing something?

@jkotas
Copy link
Member

jkotas commented Oct 20, 2020

The source generator can paste an internal copy of these marshaler types into each library that needs them for now. I think it is better to do that than to manually inline the logic for each use.

I see the inlining of the logic as an important optimization for marshalers that do very little (e.g. just pin). The marshaler type abstraction would be measurably more expensive for those given the current limits of JIT optimizations. The inlining is not worth it for heavy marshalers that allocate memory, copy blocks of memory around, etc. The relative performance overhead should be fairly small for them. In fact, inlining of the more complex logic is likely performance de-optimization overall because of the increased code size. Having the logic for these marshalers in regular C# will make it easy to modify and optimize them.

@AaronRobinsonMSFT
Copy link
Member

The source generator can paste an internal copy of these marshaler types into each library that needs them for now. I think it is better to do that than to manually inline the logic for each use.

I was hoping to avoid adding types the user wasn't aware of - not a big deal if we do though. I think this approach is something we should consider post our first pass on NetCoreApp. I could easily be persuaded if we had some performance tests that could help move the needle here and demonstrate the cost/benefit. We are planning on meeting with the performance team today to talk about guidance for performance tests. Once we understand that guidance, performance test authoring was going to start. I will make the string scenarios priority one so we can investigate this opportunity. I believe we have time to get in the "everything inlined" approach for some marshallers - I would prefer all - and then optimize as we uncover bottlenecks/opportunities. I do agree there are wins to the approach, but I personally don't understand all the dimensions and would like to have a system to experiment with prior to adding these optimization.

The TL;DR is I want to continue with the current approach and apply it to the string marshallers. Once we have performance tests in place we can explore this suggestion fully and measure the tradeoffs.

@jkotas Is that a fair plan?

@jkotas
Copy link
Member

jkotas commented Oct 20, 2020

The throughput performance is only one of the factors. Other factors to consider include:

  • Code/binary size
  • Security. Having non-trivial logic operating on unmanaged pointers stamped many times unnecessarily will be frowned upon by the people wearing security hats.

demonstrate the cost/benefit

I see the complex marshaling logic factored into a marshaler types as the baseline solution, and the manually inlined code as the extra cost that we need to demonstrate the benefit of.

I would like to have a system to experiment
I want to continue with the current approach

Fine with me.

@elinor-fung elinor-fung merged commit ac6762f into dotnet:DllImportGenerator Oct 21, 2020
@elinor-fung elinor-fung deleted the stringMarshallers branch October 21, 2020 18:30
jkoritzinsky pushed a commit to jkoritzinsky/runtime that referenced this pull request Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-DllImportGenerator Source Generated stubs for P/Invokes in C#
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants