New N-WriteStream, N-ReadStream and Device Example

[metadings]

NanomsgReadStream now uses the UnmanagedMemoryStream class.

NanomsgWriteStream now writes into a MemoryStream; with Flush it prepares a nn_iovec struct and copies the current MemoryStream buffer with Marshal.Copy into an nn_allocmsg allocated buffer. The entire message is built in NanomsgSocketBase.SendStreamImpl. The inproc protocol works as expected.

I've added a Device example, which has multiple REP workers answering to multiple REQ messages in parallel. Also try it with inproc protocol, by commenting-out the const string host for tcp.

I am submitting this patch under the MIT license.

[metadings]

Sorry I'm not a gitpro, I don't get it how to solve the conflicts, can you help with these please?

[kwpatrick]

Rejecting this. Please try and understand the zero-copy design of this library.

There's already a set of changes which address the MsgPack issue.

[metadings]

I believe this change actually enables the zero-copy, whereas before messages were not allocated using nn_allocmsg. Now you can see that also the inproc protocol works, and the data is no more fragmented.

It's just built up using MemoryStream, then the Buffer is copied onto a nn_allocated buffer. What about pinning the MemoryStream's buffer (dismissing the MemoryStream itself) and then using that buffer for the iovec_base? However also that would not have been nn_allocated.

[kwpatrick]

From your edit it sounds like you're starting to understand the issue with your approach. Follow that chain of reasoning and you'll understand the design of the current code.

There are two use cases when it comes to sending messages: a known message size and an unknown message size. nn_allocmsg is only useful in the former condition. Unfortunately, most practical serialization scenarios involve an unknown final message size (and it would involve running the serialization twice in order to pre-compute that final size). There are changes that could be made to nanomsg which would work around this restriction (i.e. allow multiple NN_MSG lengthed items in nn_msghdr's scatter array), but that is a separate topic.

Your code actually adds multiple copies on top of the Marshal.Copy. MemoryStream reallocates and copies its contents on writes. You also add a large number of managed allocations, which a design goal of the library is to avoid in the critical path as much as possible.

We're happy to have more contributors, and I hope you continue helping us. It is really important that you try to understand the existing code base, though. Ask questions if you need help, as that will probably save everyone a lot of time.

Another point of github etiquette would be to not include a large number of unrelated changes in a single pull request. I'm sure you have some beneficial updates here, but the unacceptable parts mean that it has to be rejected as a whole. Split this up and we'll have another look.

[metadings]

I'm sorry that I messed up your stream classes, now that it works I actually see the benefits ;) This is now allocating using Marshal.AllocHGlobal, what about nn_allocmsg? Currently is that completely unused by NNanomsg.

I'll post a new NanomsgSymbol class, and how to use the NanomsgException class. There will also be changes over multiple methods in NanomsgSocketBase, for example my SendImpl now looks like

        protected int SendImpl(byte[] buffer)
        {
            int sentBytes = Interop.nn_send(_socket, buffer, buffer.Length, (int)SendRecvFlags.NONE);
            if (sentBytes < 0)
            {
                var error = new NanomsgException("nn_send");
                if (error.ErrorCode == NNSymbol.EAGAIN)
                    return sentBytes;
                else if (error.ErrorCode == NNSymbol.ETERM)
                    return sentBytes;
                else
                    throw error;
            }
            Debug.Assert(sentBytes == buffer.Length);
            return sentBytes;
        }

This way the error checking becomes a little bit more comfortable, but more specifically this also drastically improves the performance of the library. I discovered that the library is often going to throw EINVAL, EAGAIN and ETERM unnecessarily as Exception, instead of just returning null or 0. With this change my example makes instead of ~340 op/s now around ~1000 op/s!

I'm going to post it tomorrow, hope you'll give it a try!

P.S: Don't you think we should name NanomsgSocket just a Socket class in Nanomsg namespace?

[metadings]

ReceiveStream often returns EINVAL, although it is in the Listener.ReceiveMessage event...

[kwpatrick]

Only two conditions in nn_recvmsg for that, and neither should happen (null msg header or multiple NN_MSG gather buffers).

Could be missing something, but the EINVALs could be coming from the device code. Maybe check it before the receive as well.

On Sat, Jul 5, 2014 at 5:00 PM, metadings notifications@github.com wrote:

ReceiveStream often returns EINVAL, although it is in the Listener.ReceiveMessage event...

— Reply to this email directly or view it on GitHub https://github.com/mhowlett/NNanomsg/pull/16#issuecomment-48096366.

[kwpatrick]

Scratch that, more than a few ways to get -EINVAL from the socket code. You see this in the current examples or some other scenario?

On Wed, Jul 9, 2014 at 2:00 AM, Kyle Patrick kwpatrick@gmail.com wrote:

Only two conditions in nn_recvmsg for that, and neither should happen (null msg header or multiple NN_MSG gather buffers).

Could be missing something, but the EINVALs could be coming from the device code. Maybe check it before the receive as well.

On Sat, Jul 5, 2014 at 5:00 PM, metadings notifications@github.com wrote:

ReceiveStream often returns EINVAL, although it is in the Listener.ReceiveMessage event...

— Reply to this email directly or view it on GitHub https://github.com/mhowlett/NNanomsg/pull/16#issuecomment-48096366.

[metadings]

Well it is (basically the same example) after nn_term - a REP worker's Listener gets a POLLIN event, then it tries to nn_recv, which fails with EINVAL. The next worker trying nn_poll fails with EPERM. Also described it in this issue on nanomsg, it looks more like an issue in the native lib.

Sorry I'll need some more days to make fine-graned pull requests.

Would you agree when I change the name of NanomsgSymbols to NNSymbol? My current version of NNSymbol also takes non-declared symbols, without Debug.Fail and also caches and allows to lookup the symbol name, symbol number and optionally a message.

The example from above (and other error handling places) now looks (for me) like

protected int SendImpl(byte[] buffer)
{
    int sentBytes = Interop.nn_send(_socket, buffer, buffer.Length, (int)SendRecvFlags.NONE);
    if (sentBytes < 0)
    {
        int error = NN.Errno();
        if (error == NNSymbol.EAGAIN) return sentBytes;
        if (error == NNSymbol.ETERM) return sentBytes;
        throw new NanomsgException("nn_send");
    }
    Debug.Assert(sentBytes == buffer.Length);
    return sentBytes;
}

It looks now like before, but not handling these errors makes the lib way slower (because it threw exceptions for EAGAIN and ETERM instead of just returning 0/null or -number). Also a NNSymbol is now not just an int but a more expressive class, like the native library's struct for a symbol.

Another little change in NanomsgSocket is an additional constructor

        public NanomsgSocket(Protocol protocol) : this(Domain.SP, protocol) { }

which allows to create a default full-blown socket with

var sock = new NanomsgSocket(Protocol.REQ);

Could you make that change, so I don't need to make a pull request for that?