eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.62k stars 383 forks source link

Reserved chunkinfo user payload header #14

Closed rex-schilasky closed 3 years ago

rex-schilasky commented 4 years ago

Brief feature description

When sending a payload chunk via publishers sendChunk method a user can pass the payload memory pointer and it's size to transfer it with zero copy behavior to the subscribers side. If there is any additional content that needs to be transferred too (like counter, user timestamp, hash values ..) there is no option to apply this information additionally into the chunk header. So the only way to transport the payload data and some additional header information as single chunk is to do an extra memory copy in a new payload memory block before sending it as one chunk. So for this use case the amazing zero memory concept is broken.

Possible solution

Provide a reserved space in the ChunkInfo header to hold / transfer a user specific payload header that is not in the same linear memory like the payload data.


edited by @elBoberido

Progress Tracking

budrus commented 4 years ago

Thanks for the request @rex-schilasky

Having a user defined extra space in the chunk header would be possible. But I'm wondering if this could also be solved in your middleware code that uses iceoryx as transport layer. I.e. when calling allocateChunk() you provide "your payload size + your specific header size". Then you do some pointer magic to provide to your next API layer a pointer to chunk pointer address + sizeof(your_header). You leave some extra space at the beginning of the iceoryx payload that you could fill when sending.

This would be similar to what we are doing under the hood. We also provide a pointer to the payload to the user but have in front of that memory space for our chunk header.

What do you think?

rex-schilasky commented 4 years ago

This is exactly how I use iceoryx right now. The only disadvantage from my point of view is that I'm a kind of specific here in that way that when I want to communicate between an eCAL task that send out some payload buffer (maybe encoded in google protobuf) and I want to receive that buffer in a 'basic' iceoryx task, I need to know that there is a specific header and cut it off before processing the payload.

If there were a "transport specific" reserved header that is only used for additional internal data between a pub / sub connection (maybe internal clock, hashes, id ..) then this information would be transparent for the higher user level API. Not a must have but a nice to have one ..

budrus commented 4 years ago

Got your point. With the next 0.16.0 release we introduce a ChunkHeader (this requires also an update on your side if you are using the current ..ChunkWithInfo.. methods). In this ChunkHeader we have the old ChunkInfo structure and also something like a specific reserved member https://github.com/eclipse/iceoryx/blob/77c84ae339db8aaedd3b34962474a13272cd59e7/iceoryx_posh/include/iceoryx_posh/mepoo/chunk_header.hpp#L34

Maybe we could make this more flexible for being able to introduce a variable size byte array. @elfenpiff What do you think?

rex-schilasky commented 4 years ago

Is this still planned to have a "byte array" like flexible ChunkHeader member ? This would qualify eCAL to have real zero copy local message transport.

elBoberido commented 4 years ago

@rex-schilasky, how many bytes do you need for your header?

There is another possibility, we could partition the payload further and add userHeader and userPayload functions. By default, userHeader would return a nullptr and the userPayload function would return the pointer to the actual payload. If a user header is used, the size of this header would be stored in the ChunkHeader and userPayload() would return the calculated address of the user payload.

current framing:
[[ChunkHeader][Payload]]
proposed framing
[[ChunkHeader][[UserHeader][UserPayload]]]
UserHeader can be 0.

What do you think? Aside from better naming ;)

rex-schilasky commented 4 years ago

@elBoberido looks perfect for me. Would be really great to get this feature and UserHeader is not that bad ;)

elBoberido commented 4 years ago

okay, it's on my TODO but I don't know when I have time for this, since there are some topics with higher priority, like mac and windows support. Is this something which causes you much pain at the moment?

rex-schilasky commented 4 years ago

Pain is a hard word. My projects is using iceoryx successfully now and this change will only improve performance.

elBoberido commented 3 years ago

@rex-schilasky, I'm currently working on this and need to clarify one thing. Do you need to be able to set a custom header per sender port or would it be sufficient for you if you could specify it at compile time

rex-schilasky commented 3 years ago

@elBoberido to specify it at compile time would be sufficient in the meaning of specifying it's fixed, maximum size in advance.

budrus commented 3 years ago

@elBoberido what do you think, how much work is left and is this a topic for 1.0 release. As this could be an API breaker

elBoberido commented 3 years ago

@budrus @rex-schilasky sorry, this completely slipped my attention. I have a branch with some ideas. There are some open points, but I think there is work for 1-2 weeks left.

elBoberido commented 3 years ago

@rex-schilasky, please have a look at the proposed design

rex-schilasky commented 3 years ago

@elBoberido thank you for investing so much effort in the proposal and the clear documentation. I have to admit that this custom header is increasing the complexity of your API significantly. Iceoryx is providing an ultra fast zero copy ipc mechanism. My question is .. would it really matter to reserve some kind of fixed size "reserved customer bytes". Most operating systems can not reserve memory files/chunks smaller than 4kB. So relative to that size .. if we would reserve 64 or 128 bytes custom data space it shouldn't matter from performance and memory perspective that much but keeps the simplicity of your API.

Maybe this size can be configured as a global #define to adapt it to customers needs. Even if that will break compatibility between differently configured iceoryx implementations it doesn't matter from my perspective because I don't see any kind of existing interoperability between different ipc frameworks using iceoryx.

budrus commented 3 years ago

@rex-schilasky We now have several use cases where we see the benefit of a flexible custom header (e.g. header for request/response). The goal is to have no increase in API complexity if the custom header is not used. If you want to use one, best would be to only have to define the type once when creating publishers and subscribers. I will discuss with @elBoberido if things would become much easier with your proposal. If yes, this should be considered as a possible solution.

rex-schilasky commented 3 years ago

@budrus This sounds perfect to me and to the existing customers too. eCAL has a very minimalistic interface to iceoryx and it's easy for us to test a new version with custom header if available. I will do some tests the next days related to my evaluation of the rmw's performance and then we can compare this "real life" use case with eCAL + current iceoryx and the new one.

elBoberido commented 3 years ago

@rex-schilasky @budrus, the public API could be used quite simple and similar to what we have now, e.g.

struct MyPayload {
    int a = 0;
    int b = 0;
};
struct MyHeader {
    int h = 0;
};
auto pub = iox::popo::TypedPublisher<MyPayload, MyHeader>(serviceDescription);
pub.loan()
    .and_then([&](auto& sample) {
        sample.getHeader()->customHeader<MyHeader>().h = 73;
        sample->a = 42;
        sample->b = 13;
        sample.publish();
    })
    .or_else([](iox::popo::AllocationError) {
        // Do something with error.
    });

You could also derive from ChunkHeaderHook and pass that to the publisher if you don't want to access the header at the loan. For the untyped publisher something similar would be done, the size of the custom header and the payloand alignment could be specified at the allocate call. But that is not yet decided.

rex-schilasky commented 3 years ago

For untyped publishers I would expect some kind of interface providing the address and size of the customer header and the same for the payload, right ? Currently the iceoryx interface looks like this

size_t CDataWriterSHM::Send(const SWriterData& data_)
{
    // allocate chunk for header and payload
    auto header_data_len = sizeof(SWriterData) + data_.len;
    auto ch = m_publisher->allocateChunkWithHeader(header_data_len, true);   // here we allocate the memory for payload header and payload data

    // copy payload header
    std::memcpy(ch->payload(), &data_, sizeof(SWriterData));
    // copy payload data         WE DONT WANT TO DO THIS
    std::memcpy(static_cast<char*>(ch->payload()) + sizeof(SWriterData), data_.buf, data_.len);

    // send the complete chunk
    m_publisher->sendChunk(ch);
}

I would like to copy the content of ´´´SWriterData´´´ into the new custom header and then provide only the pointer to the existing memory data_.buf, data_.len to the iceoryx publisher.

elBoberido commented 3 years ago

@rex-schilasky I'm not sure if you get notified when I mention you in the PR, since you are not in the suggestion box. Please have a look at https://github.com/eclipse/iceoryx/pull/403#discussion_r535135651

elBoberido commented 3 years ago

@rex-schilasky finally you should be able to use this feature. There are still a few internal cleanups and some extensions needed to make things like record&replay more robust but nothing on the public API.

rex-schilasky commented 3 years ago

@elBoberido thank you so much for all the effort you invest in that feature. We (eCAL) are waiting for the final release 1.0 and then we reintegrate the new API into our middleware. What do you mean with features that will make record&replay more robust ?

elBoberido commented 3 years ago

@rex-schilasky depending on what you put on the user-header, it might be necessary to update the content on replay, e.g. if there are some timestamps. With the chunk-header we know the layout and can put everything in place as necessary, but the user-header is opaque. The idea was to have a similar approach to the ICANN and have a known list of user-header. So 0x0000 to 0xC000 would be reserved and one can ask for a unique user-header ID and 0xC000 to 0xCFFF can be used at a whim and therefore can be anything. A record&replay framework could use the information of known user-header to update the content if necessary. This will probably be a 2.0 topic.

Furthermore, there is already a field in the chunk-header for the current version and the subscriber has to check if the received chunk-header version is the one it is compiled with to prevent weird behavior just because some old data is replayed which was recorded when the chunk-header fields had a different layout or semantic meaning. This is not much work, but time run out before the feature freeze.

The last thing is for payloads with a larger alignment than the chunk-header alignment. In this case, the record framework also needs to know the alignment in order to get the correct chunk. This is also important for gateways and will be in 1.0. It's nothing which changes the public user facing API though.

elBoberido commented 3 years ago

@budrus I will create a an example next week, then this issue could be closed.

I have two enhancement for what I'd like to create separate issues

rex-schilasky commented 3 years ago

Congratulation to Almond !! eCAL will integrate the new API soon.

rex-schilasky commented 3 years ago

@elBoberido I started to prepare everything for integrating the 1.0.0 into eCAL but it would make things easier if you could share a simple example like mentioned. I have a Custom Header structure with fixed size but flexible content for sure (timestamps, counters ..) and I have a binary payload blob with flexible size. These two information I would like to publish using a low level C++ interface.

Thank you in advance, ReX

elBoberido commented 3 years ago

@rex-schilasky I'll prepare a simple example this week. Basically its

iox::popo::Publisher<PayloadType, HeaderType> publisher(...);
publisher.loan().and_then([] (auto& sample) {
    sample.getUserHeader()->timestamp = ...;
});

or with the untyped API

iox::popo::UntypedPublisher publisher(...);
publisher.loan(userPayloadSize, userPayloadAlignment, userHeaderSize, userHeaderAlignment).and_then([] (auto userPayload) {
    static_cast<UserHeaderType*>(iox::mepoo::ChunkHeader::fromUserPayload(userPayload)->userHeader())->timestamp = ...;
});
rex-schilasky commented 3 years ago

Thank you ! I will wait for the example. Looking forward to make new performance tests, the machine is well prepared now. Building Iceoryx was working perfectly, the new documentation is great (except a simple low level pub/sub sample ;-)).

elBoberido commented 3 years ago

@rex-schilasky which API do you use? I'll start the example with that one and create a draft PR so that you have an early access to to code which is relevant to you.

rex-schilasky commented 3 years ago

Hi none of both API's exists, in my current iceoryx integration. See

eCAL iceoryx writer eCAL iceoryx reader

I would like to use the typed API but I'm open for recommendations. My goal is to avoid these 2 copies, where I copy the header and payload data in a single target blob that is finally published by iceoryx:

    // copy payload header
    std::memcpy(ch->payload(), &data_, sizeof(SWriterData));
    // copy payload data
    std::memcpy(static_cast<char*>(ch->payload()) + sizeof(SWriterData), data_.buf, data_.len);
elBoberido commented 3 years ago

Okay, then I start with the typed C++ API. From the code you posted it seems the untyped C++ API would make more sense, but maybe there is something in the upper layers which makes the typed C++ API more suitable.

rex-schilasky commented 3 years ago

Like I mentioned, I will trust in your experience here. I do not prefer one of these variants, but the typed one seemed to me more compact / better readable.

elBoberido commented 3 years ago

If you use the typed API, you would need to add a template parameter for the type you want to send to CDataWriterSHM. If that's okay, then you can use the typed API, if not, you need to use the untyped API.

rex-schilasky commented 3 years ago

Then I would go for typed :-). Thank you in advance !

elBoberido commented 3 years ago

I did a small mistake with the typed API. We have a Publisher and Subscriber alias for PublisherImpl and SubscriberImpl. The latter have an additional template parameter for testing so the former ones should help prevent doing mistakes. Unfortunately I forgot to add the template parameter for the user-header to the Publisher and Subscriber alias. It's not a big problem, but you have to use PublisherImpl and SubscriberImpl like in the example.