Need help to choose between friendly and unsafe overloads

[netcorefan1]

Hello,
I'm in the process of converting all of my old code into CsWin32, but the presence of unsafe overloads makes me doubtful on what I should choose.
Are there any advantages with the unsafe versions (speed, memory usage etc) ?
I don't care if there is more code to write since all the needed apis will only be written once and exposed through my own wrapper.
What I really care is safety. A quick list of what to do and what to not do would be really appreciated and helpful for all the other users that are in the same boat and need to know if going unsafe or not when porting their old code.
Thanks

[AArnott]

Rule no. 1: Don't be scared by the unsafe keyword. That's just what C# requires to use pointers, and using pointers is often less error prone than using less natural substitutes like IntPtr.

Friendly overloads may use the unsafe keyword as well. Friendly overloads may remove the need to pass pointers in, but not always.

Ultimately, friendly overloads and the extern methods they act as a front for just offer a more friendly signature. Sometimes they drop a reserved parameter, or change a pointer to a ref. They also very commonly accept and return SafeHandle instead of a struct like HANDLE to help you with releasing native resources.

Ultimately the overload you choose is up to you. Friendly overloads are typically preferred by many because they're easier to work with. But (especially when SafeHandle gets involved), it does add some behavior that can be good or bad, depending on what you want. For example if you're in a tight loop and calling the function many times and it returns a handle which you want to dispose of right away, a SafeHandle may just be adding unwanted GC pressure to your program.

Most of the time, both overloads will have similar performance. But for game development or other applications that require very careful perf considerations, the non-friendly overload is often the better choice for perf because it never allocates memory.

[netcorefan1]

Many thanks for your detailed explanation and sorry for my delay, but I needed to do extended researches and experiments before getting back to you with some concrete things!

As soon as I got more inside of CsWin32, I saw that in many situations there is no choice. An example is NtQuerySystemInformation which only accepts unmanaged pointers. I was initially skeptical, but now I must admit that I like much more the unsafe world.
I will post here the different possible ways I found to interact with the generated code from CsWin32 along with what are my concerns and probably concerns for many other people too. I use NtQuerySystemInformation as a sample, but the same applies to many other functions generated from the library.

public unsafe delegate bool SearchQueryDelegate(ReadOnlySpan<char> state, SYSTEM_PROCESS_INFORMATION* processInfo);
public unsafe uint FindProcessIdBy(SearchQueryDelegate query, ReadOnlySpan<char> objState) {
    uint retLen;
    NtQuerySystemInformation(SystemProcessInformation, null, 0, &retLen);
    // Allocate some extra space (+1000) in case some extra threads or processes were created inbetween calls
    var buffer = SpanOwner<byte>.Allocate((int)(retLen + 1000)); 
    fixed (byte* ptrSpan = buffer.Span)
    {
	while (true)
	{
		var status = NtQuerySystemInformation(SystemProcessInformation, ptrSpan, (uint)buffer.Length, &retLen);
		if (status == 0) break;
		if (status != STATUS_INFO_LENGTH_MISMATCH) throw new Exception();
		buffer.Dispose();
		buffer = SpanOwner<byte>.Allocate((int)retLen);
	}

	var sysInfo = (SYSTEM_PROCESS_INFORMATION*)ptrSpan;
	while (true) {
		if (query(objState, sysInfo)) {
			buffer.Dispose();
			return (uint)sysInfo->UniqueProcessId.Value;
		}

		if (sysInfo->NextEntryOffset == 0) { break; }
		sysInfo = (SYSTEM_PROCESS_INFORMATION*)((ulong)sysInfo + sysInfo->NextEntryOffset);
	}
   }
   buffer.Dispose();
}

// objState is to avoid closure
var res = FindProcessIdBy((state, pInfo) => pInfo->ImageName.Buffer.AsSpan().StartsWith(state) && pInfo->NumberOfThreads > 5, "explorer");

Do you see anything wrong with the code? Pheraps I should find a way to remove unsafe from the delegate in order to reduce the unsafe context, but I'm wondering how and if this could cause allocations.

As you see I directly access the Span inside the delegate pInfo->ImageName.Buffer.AsSpan().StartsWith(state) &&.... It is very convenient, but while debugging I saw that the generated code creates a new instance at each invocation:

public Span<char> AsSpan() => this.Value is null ? default(Span<char>) : new Span<char>(this.Value, this.Length);

This does not seems to happen if I call RtlEqualUnicodeString and pass the same instance obtained from RtlCreateUnicodeString.
This would be lot of boilerplate code:

var objState = "explorer";
// unsafe doesn't scare me, but dangerous...
PCWSTR pobjState  = new PCWSTR((char*)Unsafe.AsPointer(ref objState.DangerousGetReference())); 
UNICODE_STRING uobjState;
RtlCreateUnicodeString(&uobjState, pobjState);
// delegate version not implemented for simplicity
if (RtlEqualUnicodeString(sysInfo->ImageName, uobjState, false)) {

}

In addition to that, unless I'm missing something obvious, I would be stuck to equality comparison. I see that CsWin32 generates so many similar structs pretty much everywhere. So, I'm wondering how you would recommend to deal with this problem.

If I can further abuse your patience, I would have a couple of other questions that probably many other users have.

CsWin32 generates void* as type (NtQuerySystemInformation(SYSTEM_INFORMATION_CLASS SystemInformationClass, void* SystemInformation, uint SystemInformationLength, uint* ReturnLength);, but I saw in the official doc that this is not recommended. Any precautions people should be aware of?

In none of the samples code posted here I saw anyone talking about NativeMemory class and I'm surprised is not popular. I tried it and I was surprised to see how the code above became greatly simplified (no fixed statement, castings, and many other things). Basically I ended up with an exact replica of the native c++ version I was going to convert. Adapting the code has been very fast because basically it was just a matter of copy and paste while with the above code I had to so many things during the conversion phase.
I must miss something very obvious if no one here takes it even in consideration.

During the iteration I do sysInfo = (SYSTEM_PROCESS_INFORMATION*)((ulong)sysInfo + sysInfo->NextEntryOffset); just like native code (not sure if ulong is the right type). The alternative I tested is:
sysInfo = (SYSTEM_PROCESS_INFORMATION*)Unsafe.Add<byte>(sysInfo, (int)sysInfo->NextEntryOffset);
Is this just a semantic difference or there is something else that makes one version better over the other?

To simplify the code I found out that I could replace a fixed statement with var ptrSpan = (byte*)Unsafe.AsPointer(ref buffer.DangerousGetReference()); I am correct to deduce that the only difference is that a fixed statement allows to target specific blocks inside the method while in the other way, it will stay pinned until the method exits? What about safety (DangerousGetReference() does not sounds good)?

I had not yet chance to test unsafe vs non unsafe code in real word. However, I think that get into the habit of writing low allocation code is good. Once one get used to this pattern it will be the same as writing regular code. It also helps to understand better how the runtime works and (if) when will come the time for performance need, one will know where to look for.

Many thanks for your support

[AArnott]

Do you see anything wrong with the code?

I'm afraid I don't have time or expertise in particular win32 functions to adequately review your code. Sorry.

public unsafe delegate bool SearchQueryDelegate(ReadOnlySpan<char> state, SYSTEM_PROCESS_INFORMATION* processInfo);

Pheraps I should find a way to remove unsafe from the delegate in order to reduce the unsafe context, but I'm wondering how and if this could cause allocations.

You can sometimes avoid the unsafe keyword by changing the second parameter from a pointer to a ref struct. In this case that would look like ref SYSTEM_PROCESS_INFORMATION. Depending on whether the data is already initialized, in or out may be more appropriate parameter modifiers. That would probably be fine, provided the struct doesn't serve as a header for data that follows immediately after, since that would require that you use pointers anyway and defeat the point.

It is very convenient, but while debugging I saw that the generated code creates a new instance at each invocation:

No, you're not creating a new instance there. Span<T> is a value-type, not a reference type. That means there is no allocation when you create a new one. It's just a value on the stack (like int) -- not like List<T> which would incur an allocation and the associated perf cost. Creating a span is very cheap.

var objState = "explorer";
// unsafe doesn't scare me, but dangerous...
PCWSTR pobjState  = new PCWSTR((char*)Unsafe.AsPointer(ref objState.DangerousGetReference())); 

That code is broken. It appears to get the pointer to a GC-allocated string and initialize a PCWSTR with that pointer. But there is no guarantee by the runtime that the string will remain at that position in memory. In fact this is the last line to reference objState, so the GC could free that memory, yet you still retain a native pointer to that released memory. Don't do that.

CsWin32 generates void* as type (NtQuerySystemInformation(SYSTEM_INFORMATION_CLASS SystemInformationClass, void* SystemInformation, uint SystemInformationLength, uint* ReturnLength);, but I saw in the official doc that this is not recommended. Any precautions people should be aware of?

What official doc did you see? NtQuerySystemInformation itself declares the parameter as PVOID, which is equivalent to void* in C#. When doing interop, you have to keep the types the same. It's all well and good for C# in general to avoid void* when you can. But for interop, it's not really up to us. We have to match the native side for the interop to succeed.
I mean, if you knew that the value you'd be passing was a pointer to Foo, you could declare the parameter as Foo* and that would be fine. But from the win32 definition, we don't know the type, and so CsWin32 declares it that way. This function probably supports pointers to a variety of types and that would explain why it uses PVOID.

In none of the samples code posted here I saw anyone talking about NativeMemory class

That seems like a useful class for allocating non-GC memory, sure. CsWin32 doesn't generally emit code that allocates memory where NativeMemory would be an adequate substitute, which I suppose is why you don't see it here. In perf critical paths in applications, NativeMemory could be a useful way to allocate memory without incurring GC pressure. But for most cases, I prefer to allocate on the stack or on the managed heap and pin the memory during a single method call in order to avoid having to write finally blocks to guard against native memory leaks.

Is this just a semantic difference or there is something else that makes one version better over the other?

Pointer arithmetic can be great, but it's also subtle. For example adding 1 to int* moves the pointer by 4 bytes, IIRC. So just know what your pointer type is before moving it, and you should be fine with just raw arithmetic. I don't think I'd bother calling Unsafe.Add myself.

I am correct to deduce that the only difference is that a fixed statement allows to target specific blocks inside the method while in the other way, it will stay pinned until the method exits? What about safety (DangerousGetReference() does not sounds good)?

NO. You must use fixed (or call the GC class's static methods to get a GCHandle) to ensure managed objects aren't moved. Simply calling DangerousGetReference does not pin the object, and couldn't be expected to do so, since it has no way to un-pin it later.

[netcorefan1]

Pheraps I should find a way to remove unsafe from the delegate in order to reduce the unsafe context, but I'm wondering how and if this could cause allocations Unfortunately I don

You can sometimes avoid the unsafe keyword by changing the second parameter from a pointer to a ref struct. In this case that would look like ref SYSTEM_PROCESS_INFORMATION. Depending on whether the data is already initialized, in or out may be more appropriate parameter modifiers. That would probably be fine, provided the struct doesn't serve as a header for data that follows immediately after, since that would require that you use pointers anyway and defeat the point.

Thanks for the detailed explanation. I get the point where sometimes, trying to reduce the unsafe context do not bring any advantage.

It is very convenient, but while debugging I saw that the generated code creates a new instance at each invocation:

No, you're not creating a new instance there. Span<T> is a value-type, not a reference type. That means there is no allocation when you create a new one. It's just a value on the stack (like int) -- not like List<T> which would incur an allocation and the associated perf cost. Creating a span is very cheap.

Time ago I performed a benchmark and saw that passing an existing instance (if (RtlEqualUnicodeString(sysInfo->ImageName, uobjState, false)) { } resulted in faster execution times. However, as you say, given the cost of a Span creation, this would probably be an useless micro optimization that would not make a big difference in a real world scenario.

var objState = "explorer";
// unsafe doesn't scare me, but dangerous...
PCWSTR pobjState  = new PCWSTR((char*)Unsafe.AsPointer(ref objState.DangerousGetReference())); 

That code is broken. It appears to get the pointer to a GC-allocated string and initialize a PCWSTR with that pointer. But there is no guarantee by the runtime that the string will remain at that position in memory. In fact this is the last line to reference objState, so the GC could free that memory, yet you still retain a native pointer to that released memory. Don't do that.

I get the point. Trying to avoid fixed statements just to make the code cleaner and more readable can result in access violation exceptions or, even worse, the wrong returned data. I already experienced such things and it's a real nightmare.

CsWin32 generates void* as type (NtQuerySystemInformation(SYSTEM_INFORMATION_CLASS SystemInformationClass, void* SystemInformation, uint SystemInformationLength, uint* ReturnLength);, but I saw in the official doc that this is not recommended. Any precautions people should be aware of?

What official doc did you see? NtQuerySystemInformation itself declares the parameter as PVOID, which is equivalent to void* in C#. When doing interop, you have to keep the types the same. It's all well and good for C# in general to avoid void* when you can. But for interop, it's not really up to us. We have to match the native side for the interop to succeed. I mean, if you knew that the value you'd be passing was a pointer to Foo, you could declare the parameter as Foo* and that would be fine. But from the win32 definition, we don't know the type, and so CsWin32 declares it that way. This function probably supports pointers to a variety of types and that would explain why it uses PVOID.

I saw such suggestion under Pointer types section, but they probably means exactly what you said.

In none of the samples code posted here I saw anyone talking about NativeMemory class

That seems like a useful class for allocating non-GC memory, sure. CsWin32 doesn't generally emit code that allocates memory where NativeMemory would be an adequate substitute, which I suppose is why you don't see it here. In perf critical paths in applications, NativeMemory could be a useful way to allocate memory without incurring GC pressure. But for most cases, I prefer to allocate on the stack or on the managed heap and pin the memory during a single method call in order to avoid having to write finally blocks to guard against native memory leaks.

Given that I can guarantee the required blocks to free the memory, do you see any possible conflicts with how CsWin32 manages the memory? I'm very tempted, but the fact that no one is talking about this class (even in the web) makes me think that there must be a good reason if people still prefer the hardest ways.

I am correct to deduce that the only difference is that a fixed statement allows to target specific blocks inside the method while in the other way, it will stay pinned until the method exits? What about safety (DangerousGetReference() does not sounds good)?

NO. You must use fixed (or call the GC class's static methods to get a GCHandle) to ensure managed objects aren't moved. Simply calling DangerousGetReference does not pin the object, and couldn't be expected to do so, since it has no way to un-pin it later.

As above, you demonstrated me another way on how trying to avoid fixed statements can result in real hell. GCHandle, another precious helper underevaluated!

Thanks for your suggestions. I'm sure it will help lot of people to avoid bad things. I don't want to abuse your time. Unless you feel to add something else, I will close the discussion as resolved.

[AArnott]

Given that I can guarantee the required blocks to free the memory, do you see any possible conflicts with how CsWin32 manages the memory?

CsWin32 doesn't manage memory. It takes the pointers you give it, or occasionally pins references you pass into the friendly overloads for the life of the method call. So you should be fine here.

[netcorefan1]

Many thanks @AArnott ! Don't want to further abuse your time and I'll close the discussion as resolved.
Thanks again