Remove TryGetPointer call from UvTcpConnection #1312

KodrAus · 2017-03-14T03:44:19Z

Following the discussion in #1298

This is a really simple code change to remove the call to TryGetPointer in UvTcpConnection. It looks like the semantics are similar, but I don't think I'm fully following where that memory comes from and when it gets cleaned up, so want to make sure I'm not missing something important.

It probably also has a slight performance penalty.

I don't expect this is correct yet, just a starting point. Once this one's sorted I'll look at the RIO impl.

davidfowl · 2017-03-14T05:25:22Z

src/System.IO.Pipelines.Networking.Libuv/UvTcpConnection.cs

@@ -232,15 +233,11 @@ private unsafe Uv.uv_buf_t OnAlloc(UvStreamHandle handle, int status)
            var inputBuffer = _input.Writer.Alloc(2048);

            _inputBuffer = inputBuffer;
+
+            // REVIEW: Does this leak?
+            var pinned = inputBuffer.Memory.Pin();


You should call Free in the OnRead callback

KodrAus · 2017-03-14T06:00:41Z

@davidfowl Like this? It feels kind of weird having to call Free and null the buffer variable, but maybe it's not actually a big deal.

davidfowl · 2017-03-14T06:02:57Z

src/System.IO.Pipelines.Networking.Libuv/UvTcpConnection.cs

@@ -6,6 +6,7 @@
 using System.IO;
 using System.Threading.Tasks;
 using System.IO.Pipelines.Networking.Libuv.Interop;
+using System.Buffers;


Sort usings

davidfowl · 2017-03-14T06:04:53Z

@davidfowl Like this? It feels kind of weird having to call Free and null the buffer variable, but maybe it's not actually a big deal.

Yea, the problem is that OnRead can get called without OnAlloc so you have to know if you did indeed call .Pin so we don't try to double free (decrement the ref count)

davidfowl · 2017-03-14T06:06:22Z

src/System.IO.Pipelines.Networking.Libuv/UvTcpConnection.cs

@@ -214,6 +223,8 @@ private static void ReadCallback(UvStreamHandle handle, int status, object state
                        }
                    }
                }
+
+                inputBufferHandle.Free();


Why is this only freeing in the else case?

I'm only calling Free in the blocks where _inputBuffer is null'd.

I think you have to call it everywhere there's a commit

Hmm sorry, do you mind explaining why that's the case? If we commit but keep the buffer then wouldn't it be a problem to drop the handle if it's still going to be used?

Hmm sorry, do you mind explaining why that's the case?

Why would that be a problem? The pin only needs to last as long as the native call that copies data into it. From then the GC is free to move it around. Once the OnRead callback is called it can be freed, otherwise, you may as well keep the TryGetPointer calls.

If we commit but keep the buffer then wouldn't it be a problem to drop the handle if it's still going to be used?

Why? OnAlloc would be called again and we would pin the buffer again. The cases where OnRead gets called before OnAlloc are pretty rare. The usual pattern is OnAlloc, OnRead

Ok I think I get it now, I was conflating the lifetime of the handle with the lifetime of the buffer, rather than when it needs to be pinned.

KrzysztofCwalina · 2017-03-14T15:35:53Z

cc: @mjp41

mjp41 · 2017-03-14T17:31:25Z

src/System.IO.Pipelines.Networking.Libuv/UvTcpConnection.cs

@@ -24,6 +22,7 @@ public class UvTcpConnection : IPipeConnection

        private Task _sendingTask;
        private WritableBuffer? _inputBuffer;
+        private MemoryHandle? _inputBufferPin;


That you have had to implement this with a nullable type, makes we wonder if we should make Free callable several times, and only the first does something.

I was going to get around to asking if we should make Free idempotent. I've explicitly always set it to null in the unpin method because it's not safe to call multiple times. So that rules out the possibility of that happening

mjp41 · 2017-03-14T17:31:55Z

src/System.IO.Pipelines.Networking.Libuv/UvTcpConnection.cs

@@ -179,6 +180,7 @@ private static void ReadCallback(UvStreamHandle handle, int status, object state
                error = new IOException(uvError.Message, uvError);

                _inputBuffer?.Commit();


Should _inputBuffer be set to null at this point?

It's possible OnAlloc didn't get called.

So if OnAlloc hasn't been called, won't it already be null?

That's why we use ?

Drawaes · 2017-03-14T20:33:13Z

If your are aiming for parity with gchandle the there should be an "IsAllocated" property
Then you can do Is allocated okay then free

Drawaes · 2017-03-14T20:35:11Z

This allows a user who knows for sure it will be allocated to not have to pay the price of a check. But a user who is unsure can implement the check.

And example passing a buffer to an async method unmanaged, you need to pin before but it may return sync in which case you just free and carry on, in the async callback you need to check.

mjp41 · 2017-03-15T11:00:05Z

LGTM, shall we raise an issue to make Free idempotent, or provide a IsAllocated? (@davidfowl, @KrzysztofCwalina)

KrzysztofCwalina · 2017-03-15T15:02:50Z

Why would we want to allow multiple calls to free? Isn't it a bug to call it multiple times? If multiple threads call it, it seems like it's left to change that one of the threads won't need the handle after another thread frees it.

benaadams · 2017-03-15T15:13:46Z

Why would we want to allow multiple calls to free? Isn't it a bug to call it multiple times?

Both MemoryHandle and its internal GCHandle are pass by copy structs so you'd need to deal with that if you want to catch it.

I assume GC Handle's extren InternalFree is idempotent; so is only the multi-call to _owner.Release(); that needs caution?

jkotas · 2017-03-15T15:32:55Z

Why would we want to allow multiple calls to free? Isn't it a bug to call it multiple times?

It is convenient to allow calling Free if the handle was not allocated at all. Considers something like this:

class MyAsyncReader
{
    MemoryHandle<byte> _handle;

    ...

    ~MyReader()
    {
        // This can be null if an exception happened before the _handle was allocated so you need to check for IsAllocated
        _handle.Free();
    }
}

Similar if you do cleanup using try/finally blocks. You either need to structure your try/finally blocks just right, or check for IsAllocated before calling Free.

It matches what C/C++ does. operator delete or free can be passed null and do nothing in that case.

GCHandle.Free is over engineered in a bad way in this regard: It throws for null handle and thus requires you to check for IsAllocated; but has Interlocked.CompareExchange to protect against people calling it on multiple threads.

I do not have opinion on this yet. Just listing some reasons why it may be a good idea to allow it…

KrzysztofCwalina · 2017-03-15T15:35:20Z

If you don't have the lifetime logic totally under control of a single thread, calling IsAllocated does not help either. It has a race and it would just make finding bugs harder.

And likewise, I don't have a strong opinion on this. Just trying to explore the pros and cons.

Drawaes · 2017-03-15T15:54:32Z

You might have the life time sorted .. it is a struct right? So consider a gchandle which was the Api guideline rather than having to have nullable you can test in a destructor if it is already freed. If so don't call free.

KodrAus · 2017-03-15T22:04:39Z

Both MemoryHandle and its internal GCHandle are pass by copy structs so you'd need to deal with that if you want to catch it.

@benaadams Hmm, that's interesting, so you can't tie the Free method to a particular instance of the handle, since there may be more handles floating around that correspond to the one ref count.

My feeling is that because this is a public API we should make an effort to avoid footguns like multiple calls to Free. Since it's pass-by-copy you can't really store that as state on the handle itself, right?

benaadams · 2017-03-15T22:25:09Z

Since it's pass-by-copy you can't really store that as state on the handle itself, right?

You can solve it for a class member or use within a single function stack space/async; its whether to go further and try to solve it for multiple copies?

Drawaes · 2017-03-15T22:34:23Z

Ahh the footgun known as GCHandle... I am missing some toes due to that very thing
Readonly Structs Copy When Calling a Method
At least it will be consistent with the current GCHandle 😈

KodrAus · 2017-03-16T00:58:44Z

Is this discussion about Free likely to block this PR as it stands? Or can it be moved to a new issue and worked out independently?

KrzysztofCwalina · 2017-03-16T01:16:26Z

I think we should merge with a change to allow multiple calls to Free, or merge as-is and then change to allow multiple calls.
@davidfowl?

mjp41 · 2017-03-16T05:44:54Z

If you merge as is. I can fix free today.

davidfowl · 2017-03-16T05:51:33Z

Sure this is fine. I still don't want to remove TryGetPointer as yet.

use Memory.Pin for uv connection

658cecc

dnfclas added the cla-not-required label Mar 14, 2017

davidfowl reviewed Mar 14, 2017

View reviewed changes

free pinned handle on read

9845b7f

davidfowl reviewed Mar 14, 2017

View reviewed changes

remove and sort usings

54d1968

davidfowl reviewed Mar 14, 2017

View reviewed changes

KodrAus added 2 commits March 14, 2017 18:32

unpin input buffer on each commit

644e59f

remove redundant assingment

55f6265

mjp41 reviewed Mar 14, 2017

View reviewed changes

KodrAus changed the title ~~[WIP] Remove TryGetPointer call from UvTcpConnection~~ Remove TryGetPointer call from UvTcpConnection Mar 15, 2017

davidfowl merged commit c463c97 into dotnet:master Mar 16, 2017

mjp41 mentioned this pull request Mar 16, 2017

Made MemoryHandle Free Idempotent #1322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove TryGetPointer call from UvTcpConnection #1312

Remove TryGetPointer call from UvTcpConnection #1312

KodrAus commented Mar 14, 2017

davidfowl Mar 14, 2017

KodrAus commented Mar 14, 2017

davidfowl Mar 14, 2017

davidfowl commented Mar 14, 2017

davidfowl Mar 14, 2017

KodrAus Mar 14, 2017 •

edited

Loading

davidfowl Mar 14, 2017

KodrAus Mar 14, 2017

davidfowl Mar 14, 2017

KodrAus Mar 14, 2017

KrzysztofCwalina commented Mar 14, 2017

mjp41 Mar 14, 2017

KodrAus Mar 14, 2017

mjp41 Mar 14, 2017

davidfowl Mar 14, 2017

mjp41 Mar 14, 2017

davidfowl Mar 15, 2017

Drawaes commented Mar 14, 2017

Drawaes commented Mar 14, 2017 •

edited

Loading

mjp41 commented Mar 15, 2017

KrzysztofCwalina commented Mar 15, 2017

benaadams commented Mar 15, 2017 •

edited

Loading

jkotas commented Mar 15, 2017

KrzysztofCwalina commented Mar 15, 2017 •

edited

Loading

Drawaes commented Mar 15, 2017

KodrAus commented Mar 15, 2017

benaadams commented Mar 15, 2017

Drawaes commented Mar 15, 2017

KodrAus commented Mar 16, 2017

KrzysztofCwalina commented Mar 16, 2017

mjp41 commented Mar 16, 2017

davidfowl commented Mar 16, 2017

		@@ -179,6 +180,7 @@ private static void ReadCallback(UvStreamHandle handle, int status, object state
		error = new IOException(uvError.Message, uvError);

		_inputBuffer?.Commit();

Remove TryGetPointer call from UvTcpConnection #1312

Remove TryGetPointer call from UvTcpConnection #1312

Conversation

KodrAus commented Mar 14, 2017

Choose a reason for hiding this comment

KodrAus commented Mar 14, 2017

Choose a reason for hiding this comment

davidfowl commented Mar 14, 2017

Choose a reason for hiding this comment

KodrAus Mar 14, 2017 • edited Loading

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

KrzysztofCwalina commented Mar 14, 2017

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

Drawaes commented Mar 14, 2017

Drawaes commented Mar 14, 2017 • edited Loading

mjp41 commented Mar 15, 2017

KrzysztofCwalina commented Mar 15, 2017

benaadams commented Mar 15, 2017 • edited Loading

jkotas commented Mar 15, 2017

KrzysztofCwalina commented Mar 15, 2017 • edited Loading

Drawaes commented Mar 15, 2017

KodrAus commented Mar 15, 2017

benaadams commented Mar 15, 2017

Drawaes commented Mar 15, 2017

KodrAus commented Mar 16, 2017

KrzysztofCwalina commented Mar 16, 2017

mjp41 commented Mar 16, 2017

davidfowl commented Mar 16, 2017

KodrAus Mar 14, 2017 •

edited

Loading

Drawaes commented Mar 14, 2017 •

edited

Loading

benaadams commented Mar 15, 2017 •

edited

Loading

KrzysztofCwalina commented Mar 15, 2017 •

edited

Loading