SOLID principles

[TrabacchinLuigi]

passing the logger in the methods of the buffer pools violates single responsibility and the open closed principles, it should be passed in the constructor and be transparent to the contract

[TrabacchinLuigi]

As i'm trying to refactor to accomodate my assumptions that SOLID is the way to go, I've notice that we have just one buffer pool for every mounted filesystem, would be of any impact to have one for each filesystem ?

Also the static Dokan class begs to be non static, since init could be a constructor and shutdown a dispose

[LTRData]

As i'm trying to refactor to accomodate my assumptions that SOLID is the way to go, I've notice that we have just one buffer pool for every mounted filesystem, would be of any impact to have one for each filesystem ?

Also the static Dokan class begs to be non static, since init could be a constructor and shutdown a dispose

If you are using .NET Core (including .NET 5 or 6) the Dokan.net buffer pool is practically irrelevant and could in my opinion be considered deprecated. It is much better in those frameworks to implement IDokanOperationsUnsafe and use the native pointers directly in the .NET code without copying to and from byte arrays. .NET Core has built-in, very efficient, buffer pools that are automatically used if needed when calling Read and Write operations on Stream objects that do not implement Span based implementations of these calls.

[LTRData]

For reference, look at how I implemented IDokanOperationsUnsafe in the Mirror sample in my fork: https://github.com/LTRData/dokan-dotnet/blob/LTRData.DokanNet-initial/sample/DokanNetMirror/Mirror.cs

It uses Span<byte> wrappers for native pointers to allow the native pointers to be sent to the Stream I/O calls without any need for byte arrays.

[Liryna]

I imagine it is too soon to drop the non .NET Core framework ? Could we have two implementations depending on the framework used ?

[LTRData]

I imagine it is too soon to drop the non .NET Core framework ? Could we have two implementations depending on the framework used ?

Yes, the way I have done it in my fork I use the existing Dokan.net buffer pool when it builds for .NET Framework and the built-in .NET Core mechanisms when building for Core based frameworks (when needed, but in most cases not at all). I think that is a reasonable compromise because it allows the library to still be used with .NET Framework, it will just be a bit less efficient in that case. I do not think it is necessary at this time to do large amounts of work to optimize the existing buffer pool for .NET Framework though since it will be less used near future.

[TrabacchinLuigi]

any trubles in building with unsafe on ? i absolutely agree on dropping the buffer at all in newer version of the framework we can work with spans

[TrabacchinLuigi]

also, i think we could give a shot to the microsoft System.Memory package to port all of that back to older frameworks, but i'm not sure about .net4

[LTRData]

any trubles in building with unsafe on ? i absolutely agree on dropping the buffer at all in newer version of the framework we can work with spans

Not sure what problems that would be? By definition a library like Dokan needs full trust and administrative privilegies anyway so that should really never be a problem.

[LTRData]

also, i think we could give a shot to the microsoft System.Memory package to port all of that back to older frameworks, but i'm not sure about .net4

No, unfortunately that is not possible. It only helps within the application itself, but it does not add the necessary overloads to Stream.Read and Stream.Write when running under .NET Framework, so in one way or another you need byte arrays for all I/O in .NET Framework anyway.

[TrabacchinLuigi]

so, i've tried and .net4 isn't supported at all, also as LTRData told us we don't have all the overloads, but maybe we could provide some, i'll see what i can do... maybe we should consider dropping .net4... not sure how many people still use that

[LTRData]

Any implementation that targets 4.5 and later that can use System.Memory package can implement the unsafe operations interface and then use things like new Span<byte>(buffer.ToPointer(), (int)bufferLength) to wrap the unmanaged pointer in a Span<byte>. That does not need any changes to Dokan.net library code. It would probably however be useful for implementers to have a sample project that shows how to do that.

[TrabacchinLuigi]

I'm doing exactly that, but it will take me some time to address mostly the tests, since many of them are integration tests and not unit tests, i think I'll propose a feature branch with solid principles in mind and no .net4 unless explicitly request

[TrabacchinLuigi]

this seems too easy, is there some downside in doing something like this ?

#if NETSTANDARD1_3 || NET45_OR_GREATER
public static Int32 Read(this FileStream fileStream, Span<Byte> span)
{
    for (var i = 0; i < span.Length; i++)
    {
        var val = fileStream.ReadByte();
        if (val < 0) return i;
        span[i] = (Byte)val;
    }
    return span.Length;
}

public static void Write(this FileStream fileStream, Span<Byte> span)
{
    for (var i = 0; i < span.Length; i++)
    {
        fileStream.WriteByte(span[i]);
    }
}
#endif

[LTRData]

That looks very inefficient, unfortunately. It increases the number of I/O requests significantly. It is almost always better to buffer I/O than sending lots of small requests.

I would think that you could implement something that uses ArrayPool though if you are really looking for a better buffer pool implementation in .NET Framework. Look for example at how Stream.Read with Span<byte> is implemented in .NET Core using ArrayPool when the Stream does not implement span-based I/O and needs temporary byte arrays. It might be possible to do something similar in .NET Framework too with packages like System.Memory. https://github.com/dotnet/runtime/blob/227ff3d53784f655fa04dad059a98b3e8d291d61/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs#L677

It will obviously not be as efficient as in .NET Core though because it will use byte arrays even when they are not actually need by underlying implementations, but it will at least probably be a bit better than today's buffer pool.

[TrabacchinLuigi]

And what about this ? I've just tested it, and seems to work fine to me. We get the IntPtr back from the Span and we use the methods we wrote to implement the unsafe mirror... no buffers whatsoever

i'm not sure about the fact that we need the fixed block...

#if NETSTANDARD1_3 || NET45_OR_GREATER
public static Int32 Read(this FileStream fileStream, Span<Byte> span)
{
    var intPtr = span.GetPointer();
    if (NativeMethods.ReadFile(fileStream.SafeFileHandle, intPtr, (UInt32)span.Length, out var bytesRead, IntPtr.Zero))
    {
        return bytesRead;
    }
    return 0;
}

public unsafe static IntPtr GetPointer<T>(this Span<T> span) where T : unmanaged
{
    fixed (T* spanPtr = &MemoryMarshal.GetReference(span))
    {
        return new IntPtr(spanPtr);
    }
}

public unsafe static IntPtr GetPointer<T>(this ReadOnlySpan<T> span) where T : unmanaged
{
    fixed (T* spanPtr = &MemoryMarshal.GetReference(span))
    {
        return new IntPtr(spanPtr);
    }
}

public unsafe static void Write(this FileStream fileStream, ReadOnlySpan<Byte> span)
{
    var intPtr = span.GetPointer();
    NativeMethods.WriteFile(fileStream.SafeFileHandle, intPtr, (UInt32)span.Length, out var bytesWritten, IntPtr.Zero);
}
#endif

[LTRData]

Yes absolutely, that works as long as you do not also use the usual Read and Write methods somewhere (it will throw an exception when the file position is not at the expected location etc).

But it is not much different from the existing UnsafeMirror sample really because it only solves the problem for FileStream objects, not for other kinds of Stream implementations. It could of course be useful anyway!

[LTRData]

I think also that you should change the way you get the pointer from the Span<byte>. It works in this particular case but it does not generally work for all kinds of Span<byte> instances because if it is created from an array for example it will only keep the array pinned inside the fixed block, the unmanaged pointer could potentially point to something else after that when GetPointer returns. In this particular case though since the Span<byte> is created from an unmanaged pointer from start it is still valid.

A better solution is to omit that function completely and have the fixed block around the call to ReadFile so that you can be sure that it is valid until at least when the ReadFile call is finished.

[TrabacchinLuigi]

@Liryna i've pushed a first commit of this changes, let me know if dropping .net4 is an option https://github.com/TrabacchinLuigi/dokan-dotnet/tree/feature-spans

[Liryna]

Yeah let's drop 4.0. maybe some people still use it but it is really getting old.

Please try to keep the change as clean as possible to have a clear history. Like changing types to var as part of the commit is not necessary but could be as second PR

[Liryna]

@TrabacchinLuigi friendly ping, is that still in your todo ?

[TrabacchinLuigi]

Yep, sorry got COVID this week and my priority shifted on watching LOTR again lol the next week I'll probably be quite busy to get back on track with work, but I'll do it

[Liryna]

No worries! Take care of yourself first!