Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reserved chunkinfo user payload header #14

Closed
11 tasks done
rex-schilasky opened this issue Dec 5, 2019 · 34 comments · Fixed by #403, #426, #438, #462 or #577
Closed
11 tasks done

Reserved chunkinfo user payload header #14

rex-schilasky opened this issue Dec 5, 2019 · 34 comments · Fixed by #403, #426, #438, #462 or #577
Assignees
Labels
enhancement New feature
Milestone

Comments

@rex-schilasky
Copy link

rex-schilasky commented Dec 5, 2019

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

  • Design Document
  • Extend the current ChunkHeader and write test for the new functionality
  • remove ChunkInfo and use the refactored ChunkHeader in the code base
  • extend the API to access the new functionality
    • typed API
    • untyped API
    • C API
  • restrict access to ChunkHeader members and hand out references instead of pointer
  • documentation and examples
  • cleanup
    • remove all TODOs
    • remove chunk_mock_dds.hpp and user chunk_mock.hpp instead
    • in order to reduce ambiguity don't use just payload but chunkPayload and userPayload. The chunkPayload consists of the customHeader/userHeader and userPayload
@budrus
Copy link
Contributor

budrus commented Dec 10, 2019

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
Copy link
Author

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
Copy link
Contributor

budrus commented Dec 14, 2019

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?

@mossmaurice mossmaurice added the feature request Feature request under evaluation label Dec 16, 2019
@rex-schilasky
Copy link
Author

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
Copy link
Member

@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
Copy link
Author

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

@elBoberido
Copy link
Member

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
Copy link
Author

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

@elBoberido
Copy link
Member

@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

elBoberido added a commit to elBoberido/iceoryx that referenced this issue Sep 24, 2020
@rex-schilasky
Copy link
Author

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

@mossmaurice mossmaurice added enhancement New feature and removed feature request Feature request under evaluation labels Nov 4, 2020
@mossmaurice mossmaurice modified the milestones: v1.x, v2.0 Nov 4, 2020
@budrus
Copy link
Contributor

budrus commented Nov 4, 2020

@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
Copy link
Member

@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 added a commit to elBoberido/iceoryx that referenced this issue Nov 27, 2020
elBoberido referenced this issue in ApexAI/iceoryx Dec 1, 2020
elBoberido referenced this issue in ApexAI/iceoryx Dec 1, 2020
@elBoberido
Copy link
Member

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

marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
… call on the untyped C++ API

Signed-off-by: Mathias Kraus <[email protected]>
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
…ons and check if iox_pub_init works

Signed-off-by: Mathias Kraus <[email protected]>
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
…ccess-to-chunk-header

Iox eclipse-iceoryx#14 restrict access to chunk header
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
…yped-user-header-API

Iox eclipse-iceoryx#14 adjust untyped user header api
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
marthtz pushed a commit to boschglobal/iceoryx that referenced this issue May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment