iox-#14 chunk header design with custom header #403

elBoberido · 2020-12-01T18:08:36Z

Pre-Review Checklist for the PR Author

Notes for Reviewer

This is a test for a workflow with a design document PR before a full implementation.

It's the first PR for a custom extension to the chunk header as requested by @rex-schilasky and also needed for the request response feature #27.

I would like to start implementing this up to the integration into the publisher. For that maybe a PoC is needed. I'm quite confident that the proposed design for the ChunkHeader works regardless of the final decision how to use it in the publisher. So there will be at least two further PRs for this issue.

If the implementation requires design changes. The document will be updated with the corresponding PR.

Checklist for the PR Reviewer

Commits are properly organized and messages are according to the guideline
Code according to our coding style and naming conventions
Unit tests have been written for new behavior
Public API changes are documented via doxygen
Copyright owner are updated in the changed files
PR title describes the changes

Post-review Checklist for the PR Author

All open points are addressed and tracked via issues

Post-review Checklist for the Eclipse Committer

All checkboxes in the PR checklist are checked or crossed out
Merge

References

Partly closes Reserved chunkinfo user payload header #14

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

elBoberido · 2020-12-01T18:11:35Z

oh, if someone doesn't know that feature, you can select Display the rich diff in the Files changed tab, which makes reviewing the markdown more comfortable

elBoberido · 2020-12-01T19:08:57Z

iceoryx_posh/doc/chunk_header.md

+## Considerations
+
+- it's not uncommon to record chunks for a later replay -> detect incompatibilities on replay
+- iceoryx runs on multiple platforms -> endianness of recorded chunks might differ


btw. we could store an endianness indicator in the ChunkHeader, e.g. by replacing m_reserved2 and m_reserved3 with a uint16_t m_endianness = 0xA55A. On big endian platforms the memory would be ... m_chunkHeaderVersion m_reserved1 0xA5 0x5A and on little endian ... m_chunkHeaderVersion m_reserved1 0x5A 0xA5. Alternatively, one byte only could be used as flag.
I just don't know if it makes sense to store it in the ChunkHeader or shift the responsibility to the record framework

Good point @sculpordwarf @frank-schmidtner What do you think, should this be take care of by the recording framework? Any preferences?

iceoryx_posh/doc/chunk_header.md

budrus · 2020-12-02T09:03:43Z

iceoryx_posh/doc/chunk_header.md

+
+### Integration Into Publisher/Subscriber API
+
+The `Publisher` has additional template parameters for the custom header and payload alignment. Alternatively this could also be done with the `allocate` method, but that increases the risk of using it wrong by accident.


For me this is fixed for a specific publisher. So no need to have it in the allocate call

@budrus I would disagree here.
Isn't the idea of the untyped publisher to acquire memory for any arbitrary type and the type and amount of memory can change with every call? With this also the alignment can change with every call!
For me it seems quite natural to have then a second argument in allocate the define the alignment.
untypedPublisher.allocate(memSize, alignment)

In the case of the typed publisher I totally agree with you but here we just call loan.

I think elfenpiff is right. After thinking a bit about this, it might be helpful to have this in allocate. Maybe we have to write down some use cases for the untyped publisher.

Puuh, You really see a use case where one publisher is used with different alignments for different samples? Quite academic IMHO. If you need a special alignment that is > 32 byte, I would assume that every sample would have the same alignment as there is always the same processing for each sample and this has a specific alignment requirement

Let's say you would like to send samples which are containing data for vector operations implemented with AVX512 or SSE2/3/4. Then you need an alignment of 64 bytes. And currently we explicitly allow the user to send a bool as next sample where an alignment of 1 would suffice.

I think we should go all in in this case and support really the stuff the untyped publisher promises the expert user.

Like elfenpiff said, we can already specify the payload size when a sample is loaned, therefore it makes sense to also be able to specify the alignment. I would specify the custom header with the publisher/subscriber, though.

The untyped subscriber is very likely used when iceoryx is integrated in a bigger framework like eCAL. At least I would go for a default as for most of them it will be don't care

yes, default alignment would be 8 bytes

OK, go for it

iceoryx_posh/doc/chunk_header.md

budrus · 2020-12-02T09:17:52Z

iceoryx_posh/doc/chunk_header.md

+- untyped API and C bindings
+    - PoC is needed
+- user defined sequence number
+    - this can probably be done by a `ChunkHeaderHook`


Then the user defined sequence number would be part of the custom chunk header. Maybe this is the best for now. If iceoryx is used in a framework that wants to have additional features based on the sequence number having an additional custom chunk header there is quite likely (e.g. there are timestamps etc.)

either that or the ChunkHeaderHookcould also be used without a custom header, to overwrite the sequence number in ChunkHeader. I'm not yet sure if this is a good idea though. maybe we don't want to give the user write access to the ChunkHeader

I would only allow access to the custom chunk header in the hook. The chunk header is then a header only iceoryx writes. Having these separation of concerns makes sense for me when we have a custom header

iceoryx_posh/doc/chunk_header.md

budrus · 2020-12-02T09:43:46Z

iceoryx_posh/doc/chunk_header.md

+### ChunkHeader Methods
+
+- `void* payload()` returns a pointer to the payload
+- `template <typename T> T* customHeader()` returns a pointer to the custom header


what is returned if there is no custom header?

I suppose we could use SFINAE (enable_if meta programming magic) to prevent the user calling this method in case there is no custom header.

if we store a flag, a nullptr. Maybe an optional<T*> would be better

iceoryx_posh/doc/chunk_header.md

elfenpiff · 2020-12-02T10:04:21Z

iceoryx_posh/doc/chunk_header.md

+    - currently we specify the payload size and the size of the `ChunkHeader` is added automatically
+    - with the new approach, part of the specified payload size might be used for the custom header
+    - part of the specified payload might also be used as padding for the payload alignment
+    - shall we change this back to just specifying the full chunk size?


I would keep it as simple as possible and would just specify the full chunk size.

iceoryx_posh/doc/chunk_header.md

mossmaurice

Great work and a good example for our future design documents!

mossmaurice · 2020-12-02T10:21:46Z

iceoryx_posh/doc/chunk_header.md

+## Considerations
+
+- it's not uncommon to record chunks for a later replay -> detect incompatibilities on replay
+- iceoryx runs on multiple platforms -> endianness of recorded chunks might differ


Good point @sculpordwarf @frank-schmidtner What do you think, should this be take care of by the recording framework? Any preferences?

mossmaurice · 2020-12-02T10:26:27Z

iceoryx_posh/doc/chunk_header.md

+- iceoryx runs on multiple platforms -> endianness of recorded chunks might differ
+- for tracing, a chunk should be uniquely identifiable -> store origin and sequence number
+- the chunk is located in the shared memory, which will be mapped to arbitrary positions in the address space of various processes -> no absolute pointer are allowed
+- aligning the `ChunkHeader` to 32 bytes will ensure that all member are on the same cache line and will improve performance


Why 32 byte?

historical reasons. Up until now the chunks are aligned to 32 bytes to ensure that we also support an alignment of up to 32 bytes for the payload. With this change, the alignment of the payload can be specified, so it's not necessary anymore. It might help with performance if the ChunkHeader is not split at two cache lines and 32 is currently the size of the ChunkHeader. We might want to trade this for more memory efficiency in case we need to increase the ChunkHeader to e.g. 40 bytes, then the alignment would enforce a size of 64 bytes with 24 padding bytes. In this case it might be better to have an alignment of 8 bytes with a potential cost of distributing the ChunkHeader on multiple cache lines. This could also be a compile time option to shift the decision to the user.

mossmaurice · 2020-12-02T10:33:26Z

iceoryx_posh/doc/chunk_header.md

+```
+
+- **m_chunkSize** is the size of the whole chunk
+- **m_chunkHeaderVersion** is used to detect incompatibilities for record&replay functionality


Couldn't this be a compile time thing? How are we dealing with this currently? Using iceoryx_posh/include/iceoryx_posh/version/version_info.hpp?

yes, it would be ca compile time thing, so this should probably be a const uint8_t m_chunkHeaderVersion. Have to think about the implications about this, e.g. the ChunkHeader wouldn't be copy asignable anymore

mossmaurice · 2020-12-02T10:43:50Z

iceoryx_posh/doc/chunk_header.md

+### ChunkHeader Methods
+
+- `void* payload()` returns a pointer to the payload
+- `template <typename T> T* customHeader()` returns a pointer to the custom header


I suppose we could use SFINAE (enable_if meta programming magic) to prevent the user calling this method in case there is no custom header.

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

elBoberido · 2020-12-03T11:30:55Z

iceoryx_posh/doc/chunk_header.md

+```
+// publisher option 1
+auto pub = iox::popo::UntypedPublisher<MyHeader>(serviceDescription);
+
+// publisher option 2
+auto customHeaderSize = sizeOf(MyHeader);
+auto pub = iox::popo::UntypedPublisher(serviceDescription, customHeaderSize);
+
+auto payloadSize = sizeof(MyPayload);
+auto payloadAlignment = alignof(MyPayload);
+pub.loan(payloadSize, payloadAlignment)
+    .and_then([&](auto& sample) {
+        sample.getHeader()->customHeader<MyHeader>().data = 42;
+        auto payload = new (sample.get()) MyPayload();
+        payload->data = 73;
+        sample.publish();
+    })
+    .or_else([](iox::popo::AllocationError) {
+        // Do something with error.
+    });
+
+// subscriber option 1
+auto pub = iox::popo::UntypedPublisher<MyHeader>(serviceDescription);
+
+// subscriber option 2
+auto customHeaderSize = sizeOf(MyHeader);
+auto pub = iox::popo::UntypedSubscriber(serviceDescription, customHeaderSize);
+sub->take()
+    .and_then([](auto& sample){
+        std::cout << "Custom header data: " << sample.getHeader()->customHeader<MyHeader>().data << std::endl;
+        std::cout << "Payload data: " << static_cast<const MyPayload*>(sample->get()).data << std::endl;
+    });
+```
+    - option 1 has the benefit to catch a wrong alignment of the custom header and would be necessary if we make the `Sample` aware of the custom header


@rex-schilasky
this is an adjustment of our current untyped API. The payload size is accessible by the ChunkHeader.

iceoryx_posh/doc/chunk_header.md

budrus · 2020-12-03T11:56:52Z

iceoryx_posh/doc/chunk_header.md

+
+// subscriber option 2
+auto customHeaderSize = sizeOf(MyHeader);
+auto pub = iox::popo::UntypedSubscriber(serviceDescription, customHeaderSize);


Maybe avoiding a template here makes sense. And default for customHeaderSize would be zero. Then there is no change in the API if the custom header is not used

I would still lean towards option 1, since it's more save and I can't think of a use case to change the header between samples. The small change would be to use UntypedPublisher<>(serviceDescription) if the custom header is not used

iceoryx_posh/doc/chunk_header.md

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

elBoberido · 2020-12-03T13:20:15Z

iceoryx_posh/doc/chunk_header.md

+- is it necessary to store a flag in `ChunkHeader` if a custom extension is used?
+    - we could maintain a list of known header IDs or ranges of IDs similar to `IANA` https://tools.ietf.org/id/draft-cotton-tsvwg-iana-ports-00.html#privateports
+    - the IDs could be stored in the `ChunkHeader` and everything with an ID larger than `0xC000` is free to use
+    - to make this somewhat save, the `ChunkHeaderHook` must be mandatory with e.g. a `virtual uint16_t getId() = 0;` method which will be called in the `ChunkSender`
+- do we want to store the version of the custom extension in the `ChunkHeader`, similarly to `m_chunkHeaderVersion`
+- for record&replay the custom header is totally opaque, which might lead to problems if e.g. a timestamp is stored in the custom header and needs to be updated for the replay
+    - if we maintain a list of known custom header IDs and also store the custom header version, a record&replay framework could implement the required conversions


what do you think of this? Could nicely solve some issues

budrus

For me fine for now. The remaining questions and options are to be addressed in the implementation and integration

orecham

Concept looks sound.
Good write-up!

iox-#14 chunk header design with custom header

85bdd58

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

elBoberido added the documentation Improvements or additions to documentation label Dec 1, 2020

elBoberido requested review from orecham, mossmaurice, marthtz, dkroenke, budrus, elfenpiff and MatthiasKillat December 1, 2020 18:08

elBoberido commented Dec 1, 2020

View reviewed changes

budrus requested changes Dec 2, 2020

View reviewed changes

budrus reviewed Dec 2, 2020

View reviewed changes

elfenpiff reviewed Dec 2, 2020

View reviewed changes

iceoryx_posh/doc/chunk_header.md Show resolved Hide resolved

elfenpiff reviewed Dec 2, 2020

View reviewed changes

iceoryx_posh/doc/chunk_header.md Show resolved Hide resolved

elfenpiff reviewed Dec 2, 2020

View reviewed changes

iceoryx_posh/doc/chunk_header.md Show resolved Hide resolved

mossmaurice reviewed Dec 2, 2020

View reviewed changes

budrus removed request for orecham, marthtz, dkroenke and MatthiasKillat December 2, 2020 15:42

elBoberido added 2 commits December 3, 2020 11:19

iox-#14 improve ascii diagrams

30e884b

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

iox-#14 add proposal for publisher/subscriber API

6a1450e

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

elBoberido commented Dec 3, 2020

View reviewed changes

elBoberido mentioned this pull request Dec 3, 2020

Reserved chunkinfo user payload header #14

Closed

11 tasks

budrus reviewed Dec 3, 2020

View reviewed changes

elBoberido commented Dec 3, 2020

View reviewed changes

iceoryx_posh/doc/chunk_header.md Outdated Show resolved Hide resolved

iox-#14 update proposal for maintaining a list of custom header IDs

69469bd

Signed-off-by: Mathias Kraus <mathias.kraus@apex.ai>

elBoberido commented Dec 3, 2020

View reviewed changes

budrus self-requested a review December 7, 2020 11:01

budrus approved these changes Dec 7, 2020

View reviewed changes

orecham approved these changes Dec 7, 2020

View reviewed changes

elBoberido mentioned this pull request Dec 7, 2020

Refine modern pub/sub API #408

Closed

5 tasks

elBoberido merged commit 07626f9 into eclipse-iceoryx:master Dec 7, 2020

elBoberido deleted the iox-#14-chunk-header-design branch December 7, 2020 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iox-#14 chunk header design with custom header #403

iox-#14 chunk header design with custom header #403

elBoberido commented Dec 1, 2020 •

edited

Loading

elBoberido commented Dec 1, 2020

elBoberido Dec 1, 2020

mossmaurice Dec 2, 2020

budrus Dec 2, 2020

elfenpiff Dec 2, 2020

elBoberido Dec 2, 2020

budrus Dec 2, 2020

elfenpiff Dec 3, 2020

elBoberido Dec 3, 2020

budrus Dec 3, 2020

elBoberido Dec 3, 2020

budrus Dec 7, 2020

budrus Dec 2, 2020

elBoberido Dec 2, 2020

budrus Dec 7, 2020

budrus Dec 2, 2020

mossmaurice Dec 2, 2020

elBoberido Dec 2, 2020

elfenpiff Dec 2, 2020

mossmaurice left a comment

mossmaurice Dec 2, 2020

mossmaurice Dec 2, 2020

elBoberido Dec 2, 2020

mossmaurice Dec 2, 2020

elBoberido Dec 2, 2020

mossmaurice Dec 2, 2020

elBoberido Dec 3, 2020

budrus Dec 3, 2020

elBoberido Dec 3, 2020

elBoberido Dec 3, 2020

budrus left a comment

orecham left a comment


		### Integration Into Publisher/Subscriber API

		The `Publisher` has additional template parameters for the custom header and payload alignment. Alternatively this could also be done with the `allocate` method, but that increases the risk of using it wrong by accident.

iox-#14 chunk header design with custom header #403

iox-#14 chunk header design with custom header #403

Conversation

elBoberido commented Dec 1, 2020 • edited Loading

Pre-Review Checklist for the PR Author

Notes for Reviewer

Checklist for the PR Reviewer

Post-review Checklist for the PR Author

Post-review Checklist for the Eclipse Committer

References

elBoberido commented Dec 1, 2020

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

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

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

mossmaurice left a 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

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

budrus left a comment

Choose a reason for hiding this comment

orecham left a comment

Choose a reason for hiding this comment

elBoberido commented Dec 1, 2020 •

edited

Loading