GH-35809: [C#] Improvements to the C Data Interface #35810

teo-tsirpanis · 2023-05-28T23:15:08Z

Rationale for this change

This PR fixes issues identified while reading the code of the Apache.Arrow.C namespace.

What changes are included in this PR?

See each commit message for more details.

Are these changes tested?

Using the existing test suite.

Are there any user-facing changes?

No.

Closes: [C#] Issues with the C data interface implementation. #35809

This patches a memory leak.

The last error message is included if it exists, and in `ReadNextRecordBatchAsync` the exception was returned with the `ValueTask`.

…rdBatchAsync`.

There was a reason they were skipped, as explained in the PR that added them.

github-actions · 2023-05-28T23:15:31Z

Closes: [C#] Issues with the C data interface implementation. #35809

github-actions · 2023-05-28T23:15:34Z

⚠️ GitHub issue #35809 has been automatically assigned in GitHub to PR creator.

kou · 2023-05-28T23:31:27Z

See each commit message for more details.

Could you write it in the pull request description because we use squash merge and the merge commit only uses the pull request description.

It seems that this pull request has several logical changes. It may be better that you split this pull request and associated issue to multiple small pull requests/issues for easy to review.

pitrou · 2023-05-30T17:20:14Z

csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs

 using System.Runtime.InteropServices;
 using Apache.Arrow.Memory;

 namespace Apache.Arrow.C
 {
    public static class CArrowArrayExporter
    {
+#if NET5_0_OR_GREATER


Perhaps add a comment explaining why this needs .Net 5.0+? Or will it be obvious to a C# developer?

UnmanagedCallersOnlyAttribute was introduced in .NET 5.

pitrou · 2023-05-30T17:22:55Z

csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs

@@ -203,6 +207,10 @@ private unsafe static void ReleaseArray(CArrowArray* cArray)
        private unsafe static void Dispose(void** ptr)
        {
            GCHandle gch = GCHandle.FromIntPtr((IntPtr)(*ptr));
+            if (!gch.IsAllocated)


Add a comment explaining when this might occur? (perhaps if an exception occurs while exporting the array?)

pitrou · 2023-05-30T17:30:33Z

csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs

@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
            cArray->dictionary = null;
        }

+#if NET5_0_OR_GREATER
+        [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]


Shouldn't the default calling convention be used instead? I'm not sure stdcall is ok on non-Windows platforms.

See #34133 (comment). Ideally we would have used the default calling convention, but that would not be suppported on anything earlier than .NET 5. And in 64-bit platforms the calling convention doesn't matter either way.

Well, the entire point (mostly) of implementing the C Data Interface is to be compatible with non-.Net producers/consumers. Those are extremely likely to use the platform default. So we should get it right at least when possible, i.e. on .Net >= 5.0.

As for https://stackoverflow.com/questions/34832679/is-the-callingconvention-ignored-in-64-bit-net-applications , does it apply here? It's talking about DllImport, which might be different from this?

In any case, keeping the default convention seems more theoretically sound (and forward-looking, perhaps).

cc @westonpace @lidavidm for opinions.

If I understand correctly, are you saying to change this:

arrow/csharp/src/Apache.Arrow/C/CArrowArray.cs

Line 41 in e628ca5

public delegate* unmanaged[Stdcall]<CArrowArray*, void> release;

into this?

#if NET5_0_OR_GREATER public delegate* unmanaged<CArrowArray*, void> release; #else public delegate* unmanaged[Stdcall]<CArrowArray*, void> release; #endif

That would cause an incompatible API surface between the assembly compiled for .NET 6 and that compiled for the earlier frameworks. We have two options:

Lie and keep the stdcall calling convention on the function pointers.

Use the default unmanaged calling convention but support the C interface only on .NET 6+ (we don't target 5 as it is unsupported).

Well, why is this release member public here? It will be exposed to C Data Interface consumers as the release pointer, but needn't (and probably shouldn't) be part of the Arrow C# API.

Arrow C# API users should only see the high-level import and export methods such as ImportArray and ExportArray.

(an important thing to understand is that the C Data Interface is a binary interface, not an API)

If you take a look for example at the Arrow C++ implementation, its release functions are entirely private. For example ReleaseExportedSchema below is in the anonymous namespace, which doesn't expose the function publicly:

arrow/cpp/src/arrow/c/bridge.cc

Line 107 in e628ca5

void ReleaseExportedSchema(struct ArrowSchema* schema) {

So I should make the members of these structs private? That also works.

If there were some reason for the fields to stay public, this one could also probably be defined as a union between a public IntPtr and a private delegate. (I don't know why the fields would need to be public; I'm pretty sure I just followed the pattern that was present for schemas.)

CurtHagenlocher · 2023-05-30T14:40:21Z

csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs

@@ -203,6 +207,10 @@ private unsafe static void ReleaseArray(CArrowArray* cArray)
        private unsafe static void Dispose(void** ptr)
        {
            GCHandle gch = GCHandle.FromIntPtr((IntPtr)(*ptr));
+            if (!gch.IsAllocated)


This is effectively a noop. If the pointer was null, the previous line will throw InvalidOperationException and if it wasn't null then IsAllocated will return true.

The overall change here also means that calling ReleaseArray twice will now throw an exception instead of the second call being a no-op.

CurtHagenlocher · 2023-05-30T14:40:28Z

csharp/src/Apache.Arrow/C/CArrowArrayStreamExporter.cs

+            public static void Free(void** ptr)
+            {
+                GCHandle gch = GCHandle.FromIntPtr((IntPtr)ptr);
+                if (!gch.IsAllocated)


(Same comment about IsAllocated.)

CurtHagenlocher · 2023-05-30T18:52:00Z

csharp/src/Apache.Arrow/C/CArrowArrayExporter.cs

@@ -184,13 +189,12 @@ private unsafe static void ConvertRecordBatch(ExportedAllocationOwner sharedOwne
            cArray->dictionary = null;
        }

+#if NET5_0_OR_GREATER
+        [UnmanagedCallersOnly(CallConvs = new[] { typeof(CallConvStdcall) })]


If there were some reason for the fields to stay public, this one could also probably be defined as a union between a public IntPtr and a private delegate. (I don't know why the fields would need to be public; I'm pretty sure I just followed the pattern that was present for schemas.)

westonpace

It sounds like we might've worked through most of the issues. If I understand it correctly, if someone wants to use 32-bit windows, then they need to use .NET 5 or greater, which seems like a reasonable tradeoff to me.

Given we don't have any 32-bit tests for any of the C# APIs, there may be other issues as well. We can maybe just move forward and worry about 32-bit APIs if a user emerges?

CurtHagenlocher · 2023-06-08T20:30:14Z

It sounds like we might've worked through most of the issues. If I understand it correctly, if someone wants to use 32-bit windows, then they need to use .NET 5 or greater, which seems like a reasonable tradeoff to me.

I'd understood it differently (though perhaps incorrectly). I thought that using the explicit [Stdcall] would work everywhere except 32-bit non-Windows platforms, while removing the [Stdcall] would prevent things from working on .NET < 5.

westonpace · 2023-06-09T12:54:03Z

I'd understood it differently (though perhaps incorrectly). I thought that using the explicit [Stdcall] would work everywhere except 32-bit non-Windows platforms, while removing the [Stdcall] would prevent things from working on .NET < 5.

Yes, looking at the code now I think you're right. I interpreted the above conversation as:

This would be safe but lead to an unstable API surface
If we make these methods/fields private we don't have to worry about the API surface

However, looking at the code, I don't see that change. Are we still waiting on that change? Or is there some reason this wouldn't work?

teo-tsirpanis · 2023-06-09T12:55:57Z

I will make the fields internal soon; I'm pretty busy these days.

teo-tsirpanis · 2023-06-10T16:06:03Z

I made them internal.

westonpace · 2023-06-20T17:28:51Z

I thought the point of making them internal was so that we could use something like this:

#if NET5_0_OR_GREATER
public delegate* unmanaged<CArrowArray*, void> release;
#else
public delegate* unmanaged[Stdcall]<CArrowArray*, void> release;
#endif

However, I don't see that. Also, looks like we will need a rebase.

teo-tsirpanis · 2023-06-21T22:53:50Z

Done @westonpace.

pitrou · 2023-07-05T13:31:34Z

@teo-tsirpanis @westonpace Is there anything left to do here? It would be nice to have this in 13.0.0.

pitrou · 2023-07-06T15:50:47Z

I will say again that I think the callbacks should be Cdecl, not Stdcall. Stdcall is the calling convention for WinAPI functions and nothing else, AFAICT. Cdecl is the default calling convention for any other C/C++ code, such as the callbacks that producers will pass in the release member.

See: https://www.codeproject.com/Articles/1388/Calling-Conventions-Demystified

pitrou · 2023-07-06T15:51:11Z

(and, yes, it would only make a difference on 32-bit x86 Windows machines, and even then, perhaps in some cases the two are equivalent)

westonpace

I'm still not entirely an expert in calling conventions, and I don't fully understand. I do think the default should be cdecl. However, given this only affects extremely rare systems, and we don't even know if there are any users using those systems, I don't want to hold up legitimate fixes while worrying over calling conventions.

pitrou · 2023-07-06T15:59:45Z

Feel free to merge :-)

westonpace · 2023-07-06T17:09:12Z

Sorry, didn't see your comment on #36506 but it looks like you got the change in just in time. Fortunately, it seems CI is passing :) I'll keep monitoring it.

westonpace · 2023-07-06T17:12:29Z

All tests passed, thanks for your attention and the cleanup @teo-tsirpanis

eerhardt · 2023-07-06T23:04:23Z

csharp/src/Apache.Arrow/C/CArrowArrayStream.cs

@@ -35,15 +35,23 @@ public unsafe struct CArrowArrayStream
        ///
        /// Return value: 0 if successful, an `errno`-compatible error code otherwise.
        ///</summary>
-        public delegate* unmanaged[Stdcall]<CArrowArrayStream*, CArrowSchema*, int> get_schema;


If we make these function pointers internal, how are users of these APIs supposed to call them?

They should not; the importer will take care of calling them.

I don't fully follow. If I wanted to take some C arrow data (let's say coming from C++) and natively work with it - not using the "normal" C# ArrowTypes, Schema, RecordBatches, etc. but instead manually calling into the native functions on the CArrowArrayStream, that is no longer possible? Why not?

The reason for interoping at the native layer would be for performance - I wouldn't need to allocate a bunch of managed objects just to interact with the Arrow information coming from some other library.

See #35810 (comment) for an explanation on why the function pointer fields are internal.

Another option would be to only publicly expose the function pointer fields on net5.0+. Keep them internal for netstandard and netfx where they need to be declared Cdecl (and honestly function pointers aren't really meant to be used on netfx and netstandard since they came in C# 9 - https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version).

That way users on net5+ can still call the function pointers, if they need to. For netstandard and netfx, they are no worse off - they can't call them with this change anyway.

conbench-apache-arrow · 2023-07-13T08:36:55Z

Conbench analyzed the 6 benchmark runs on commit 88cb5170.

There were 2 benchmark results indicating a performance regression:

Commit Run on ursa-thinkcentre-m75q at 2023-07-10 08:20:40Z
- params=4096, source=cpp-micro, suite=parquet-encoding-benchmark
Commit Run on ursa-i9-9960x at 2023-07-13 05:36:26Z
- engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-01, scale_factor=1

The full Conbench report has more details.

teo-tsirpanis added 11 commits May 28, 2023 15:02

Clean-up NativeDelegate.

71bb92a

Specify the calling convention of the unmanaged delegates.

5a7c883

Use[UnmanagedCallersOnly] on modern .NET.

f879fa8

Optimize zeroing of the C structs.

ed68f43

Free the GCHandle when releasing the exported stream.

b0bbeaa

This patches a memory leak.

Refactor the release code of exported arrays.

1b47731

Improve exception throwing in imported ArrayStreams.

a595a9a

The last error message is included if it exists, and in `ReadNextRecordBatchAsync` the exception was returned with the `ValueTask`.

Honor the canellation token in `ImportedArrowArrayStream.ReadNextReco…

f9f310a

…rdBatchAsync`.

Use Array.Empty.

540c190

Use the correct memory freeing method.

473a9be

Remove [UnmanagedFunctionPointer] from the unmanaged delegates.

154b86c

There was a reason they were skipped, as explained in the PR that added them.

teo-tsirpanis requested a review from westonpace as a code owner May 28, 2023 23:15

github-actions bot added Component: C# awaiting review Awaiting review labels May 28, 2023

pitrou reviewed May 30, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 30, 2023

pitrou reviewed May 30, 2023

View reviewed changes

This was referenced Jun 6, 2023

feat(csharp): adding C# functionality apache/arrow-adbc#697

Merged

GH-33856: [C#] Implement C Data Interface for C# #35496

Merged

CurtHagenlocher reviewed Jun 8, 2023

View reviewed changes

westonpace approved these changes Jun 8, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jun 8, 2023

Make the function pointer fields in the C API structs internal.

e9365fc

teo-tsirpanis force-pushed the c-interface-opt branch from ca13a32 to 12d9695 Compare June 21, 2023 19:24

Merge branch 'main' into c-interface-opt

e10cad8

teo-tsirpanis force-pushed the c-interface-opt branch from 12d9695 to 410a710 Compare June 21, 2023 22:38

Use the default unmanaged calling convention where supported.

d7d830f

teo-tsirpanis force-pushed the c-interface-opt branch from 410a710 to d7d830f Compare June 21, 2023 22:45

pitrou changed the title ~~GH-35809: [C#] Improvements to the C interface.~~ GH-35809: [C#] Improvements to the C Data Interface Jul 6, 2023

westonpace approved these changes Jul 6, 2023

View reviewed changes

Replace Stdcall with Cdecl.

09e6a24

westonpace merged commit 88cb517 into apache:main Jul 6, 2023

westonpace removed the awaiting merge Awaiting merge label Jul 6, 2023

teo-tsirpanis deleted the c-interface-opt branch July 6, 2023 17:09

westonpace mentioned this pull request Jul 6, 2023

[C#] Calling convention specifier incorrect for C#'s C data interface #36506

Closed

eerhardt reviewed Jul 6, 2023

View reviewed changes

github-actions bot added the awaiting changes Awaiting changes label Jul 6, 2023

GH-35809: [C#] Improvements to the C Data Interface #35810

GH-35809: [C#] Improvements to the C Data Interface #35810

Conversation

teo-tsirpanis commented May 28, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented May 28, 2023

github-actions bot commented May 28, 2023

kou commented May 28, 2023

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

westonpace left a comment

Choose a reason for hiding this comment

CurtHagenlocher commented Jun 8, 2023

westonpace commented Jun 9, 2023

teo-tsirpanis commented Jun 9, 2023

teo-tsirpanis commented Jun 10, 2023

westonpace commented Jun 20, 2023

teo-tsirpanis commented Jun 21, 2023

pitrou commented Jul 5, 2023

pitrou commented Jul 6, 2023

pitrou commented Jul 6, 2023 • edited Loading

westonpace left a comment

Choose a reason for hiding this comment

pitrou commented Jul 6, 2023

westonpace commented Jul 6, 2023

westonpace commented Jul 6, 2023

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

conbench-apache-arrow bot commented Jul 13, 2023

teo-tsirpanis commented May 28, 2023 •

edited by github-actions bot

Loading

pitrou commented Jul 6, 2023 •

edited

Loading