-
Notifications
You must be signed in to change notification settings - Fork 389
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
iox-1803 Expand cmake configuration options #1804
iox-1803 Expand cmake configuration options #1804
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakubSosnovec I have no other words than: This is awesome!
Thank you so much for this great work we truly appreciate this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add the small_memory
example as an exception to tools/iceoryx_build_test.sh
(line 318 ff) since it does not contain any actual source code.
@elfenpiff @elBoberido There are a few things to get your opinion on:
|
Codecov Report
@@ Coverage Diff @@
## master #1804 +/- ##
==========================================
- Coverage 75.53% 75.48% -0.06%
==========================================
Files 375 375
Lines 14594 14596 +2
Branches 2070 2071 +1
==========================================
- Hits 11024 11018 -6
- Misses 2941 2949 +8
Partials 629 629
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
||
Please note that it might be impossible to build some other iceoryx examples, | ||
since the decreased maximum length of service and runtime name strings might | ||
cause some static assertion failures. This problem demonstrates that the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakubSosnovec These assertions are because the strings used in the other examples are too long then? Would it make sense to ensure all examples are compatible with the small_memory setup? Compiling maybe would be possible but we would get runtime problem as the resource limits are too small, I guess. So if we would like to ensure that all examples work, this would lead to bad compromises, e.g. having to increase the numbers again a bit because mempools are too small or not enough subscribers are possible. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, some examples will fail to even compile, because iox::cxx::string
constructors have static asserts for checking the length of string literals passed in -- and some demos have quite long strings for either runtime names or service description names.
Furthermore, even if we get through compilation, the memory pool in this demo is extremely small (10x512b), so I expect that wouldnt be enough for some other demos.
Of course, we could increase the resource limits, but then the demo memory usage wouldnt fit into 100 kB. So I thought it is better to keep it as is, and add a warning that decreasing the resource limits of course has some consequences, such as incompatibility with demos other than the hello world example.
Is that okey with you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakubSosnovec Yes, I wouldn't make bad compromises that increase the memory footprint again. If we have the disclaimer then it is also OK if we get runtime errors when executing other examples that need larger mempools, more subscribers etc. The only question for me would then be the compile time one. I.e. should we maybe change the runtime and service ID names of the other examples to be compatible with your setup. I think you have max string length of 30 from what I saw which should be manageable.
Pinging @elfenpiff as he already reviewed to see what he thinks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@budrus I do not think it is a good idea to adapt all examples to minimal configuration. Runtime errors are OK, if stated that only specific examples work with the minimal configuration.
From the perspective of compile time, as already mentioned the maximum string length is the main reason why a build can fail. This is a side effect of the way the static strings work, there has to be a limit (in the static case) and failing early (e.g. already at compilation) is preferable.
String sizes affect service description sizes, which affect port sizes etc. which contribute significantly to the memory footprint of the management segment, so there is a trade-off.
Maybe it is worth considering to have a minimal with minimal string size that builds only one example?
Tests might still be a problem, as they also use strings and may also need to excluded from the build for a minimal configuration. String size should probably be a power of two, to reduce wasted memory due to alignment in some cases, say 32.
There is no simple answer here to make everything work with really low string sizes.
Luckily the user memory segment has less issues and is fairly flexible regarding its size. It still reminds me that the internal allocation is wasteful and fairly inflexible though (to be changed when we provide advanced allocator support).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is worth considering to have a minimal with minimal string size that builds only one example?
Thats exactly what I did. The small_memory
example that I added is basically just a guide on how to configure iceoryx just enough to be able to build the icehello
example. I do know that other examples will fail, since some of them use very long strings.
@JakubSosnovec I can just repeat what elfenpiff said. Thanks very much for the PR. Unfortunately I did not yet have time to do a review but it's at the top of my todo list for next week :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and makes some previously non-configurable options configurable now. I did not try to build and run it yet, will do so on Monday. If we can solve the (potential) build issue with potential problems due to reduced string length we can merge it IMO.
|
||
Please note that it might be impossible to build some other iceoryx examples, | ||
since the decreased maximum length of service and runtime name strings might | ||
cause some static assertion failures. This problem demonstrates that the user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@budrus I do not think it is a good idea to adapt all examples to minimal configuration. Runtime errors are OK, if stated that only specific examples work with the minimal configuration.
From the perspective of compile time, as already mentioned the maximum string length is the main reason why a build can fail. This is a side effect of the way the static strings work, there has to be a limit (in the static case) and failing early (e.g. already at compilation) is preferable.
String sizes affect service description sizes, which affect port sizes etc. which contribute significantly to the memory footprint of the management segment, so there is a trade-off.
Maybe it is worth considering to have a minimal with minimal string size that builds only one example?
Tests might still be a problem, as they also use strings and may also need to excluded from the build for a minimal configuration. String size should probably be a power of two, to reduce wasted memory due to alignment in some cases, say 32.
There is no simple answer here to make everything work with really low string sizes.
Luckily the user memory segment has less issues and is fairly flexible regarding its size. It still reminds me that the internal allocation is wasteful and fairly inflexible though (to be changed when we provide advanced allocator support).
set(IOX_MAX_SHM_SEGMENTS 2 CACHE STRING "") | ||
set(IOX_MAX_NUMBER_OF_MEMPOOLS 3 CACHE STRING "") | ||
set(IOX_MAX_NUMBER_OF_CONDITION_VARIABLES 8 CACHE STRING "") | ||
set(IOX_MAX_NODE_NAME_LENGTH 30 CACHE STRING "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could maybe be like 32 and still not use much more memory due to alignment. But likely will use a little more, so there is a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatthiasKillat 32 would lead to 7 wasted bytes due to the string capacity being length + 1
. What would work would be 31 but that looks ugly and I think keeping it at 30 is a good aesthetic compromise.
[[segment]] | ||
|
||
[[segment.mempool]] | ||
size = 512 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the size can be reduced even more (like 256), depending on the needs of the example. For count I am also a big fan of powers of two (8 or 16 maybe?) but of course it is not mandatory.
Overall the data segment organization memory consumption is fairly easy to change, unlike the management memory consumption.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, it is a runtime option, not a compile time one.
/// @NOLINTBEGIN(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers) | ||
template <> | ||
inline bool convert::fromString<string<100>>(const char* v, string<100>& dest) noexcept | ||
template <uint64_t Capacity> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Why was there a leftover magic number specialization in the first place... :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*cough* CString100
*cough*
set(IOX_MAX_CHUNKS_HELD_PER_SUBSCRIBER_SIMULTANEOUSLY 256) | ||
endif() | ||
message(STATUS "[i] IOX_MAX_CHUNKS_HELD_PER_SUBSCRIBER_SIMULTANEOUSLY:" ${IOX_MAX_CHUNKS_HELD_PER_SUBSCRIBER_SIMULTANEOUSLY}) | ||
configure_option( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we keep the defaults from before here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these values are the defaults that were hard-coded before.
@@ -46,6 +47,21 @@ constexpr uint64_t IOX_MAX_PUBLISHER_HISTORY = static_cast<uint32_t>(@IOX_MAX_PU | |||
constexpr uint32_t IOX_MAX_CHUNKS_HELD_PER_SUBSCRIBER_SIMULTANEOUSLY = | |||
static_cast<uint32_t>(@IOX_MAX_CHUNKS_HELD_PER_SUBSCRIBER_SIMULTANEOUSLY@); | |||
constexpr uint32_t IOX_MAX_NUMBER_OF_NOTIFIERS = static_cast<uint32_t>(@IOX_INTERNAL_MAX_NUMBER_OF_NOTIFIERS@); | |||
constexpr uint32_t IOX_MAX_PROCESS_NUMBER = static_cast<uint32_t>(@IOX_MAX_PROCESS_NUMBER@); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I am wrong, but these appear all to be new options according to the diff. But I was sure we e.g. already had some of these for say request-response in sme way (e.g. IOX_MAX_CLIENTS_PER_SERVER
).
Can you confirm that these are new parameters?
Edit: Answered myself (I think) but still leave the comment for confirmation.
@@ -86,20 +86,20 @@ constexpr uint32_t MAX_GATEWAY_SERVICES = 2 * MAX_CHANNEL_NUMBER; | |||
// Client | |||
constexpr uint32_t MAX_CLIENTS = build::IOX_MAX_SUBSCRIBERS; | |||
constexpr uint32_t MAX_REQUESTS_ALLOCATED_SIMULTANEOUSLY = 4U; | |||
constexpr uint32_t MAX_RESPONSES_PROCESSED_SIMULTANEOUSLY = 16U; | |||
constexpr uint32_t MAX_RESPONSE_QUEUE_CAPACITY = 16U; | |||
constexpr uint32_t MAX_RESPONSES_PROCESSED_SIMULTANEOUSLY = build::IOX_MAX_RESPONSES_PROCESSED_SIMULTANEOUSLY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I can answer my own question now. We had the options but they were hard-coded here.
However, it is somewhat inconsistent as some of them are still hard-coded. No need for changes, just an observation. We do not want some of them configurable, but the distinction may look arbitrary and not cleanly separated. Something for another issue, maybe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, I was also unsure about this inconsistency here . We can easily add at least the obvious ones, such as MAX_REQUESTS_PROCESSED_SIMULTANEOUSLY
. It would make sense to me, I guess.
/// @param[in] dest destination to which the value should be written | ||
/// @return false = if the conversion fails otherwise true | ||
template <uint64_t Capacity> | ||
static bool fromString(const char* v, string<Capacity>& dest) noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this tested? Maybe indirectly by passing a string<Capacity>
to the previously existing template but then why do we need this overload in the first place?
If it is not tested we should add at least three tests, one with an empty string, one with a non-empty one and one with maximum length Capacity
.
template <> | ||
inline bool convert::fromString<string<100>>(const char* v, string<100>& dest) noexcept | ||
template <uint64_t Capacity> | ||
inline bool convert::fromString(const char* v, string<Capacity>& dest) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see now why we would need to add this overload. So in theory it should be tested in the way I requested with a string<100>
. Did not check, but nothing breaks in the CI so it works or is untested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a simple test for convert::fromString
overload. Also I actually modified the implementation to return false
if the source size exceeds the cxx::string
capacity.
@@ -0,0 +1,76 @@ | |||
# icehello with minimal memory usage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked the example and it worked well thanks to the good description. Will continue reviewing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakubSosnovec To make this example visible on the website you can add a simple redirection with a title in https://github.com/eclipse-iceoryx/iceoryx/tree/master/doc/website/examples to link to this markdown file.
Additionally please add an entry in https://github.com/eclipse-iceoryx/iceoryx/blob/master/doc/website/examples/.pages
Output looks like this: https://iceoryx.io/v2.0.2/examples/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What to do if a new example is created is also documented here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkroenke @mossmaurice I have now added the example documentation and links according to https://github.com/eclipse-iceoryx/iceoryx/blob/master/CONTRIBUTING.md#how-to-add-a-new-example, including the asciinema recording (sed
ing the user to hypnotoad
screwed up the recording a bit, because of the different lengths of my username and hypnotoad
, so I used htoad
instead).
Please take a look :)
c252858
to
f22d406
Compare
I have rebased to latest master, to fix some merge conflicts in copyright comment lines. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to myself: Don't give estimates for PR reviews 😅
Looks good. Could be merged from my point of view.
set(IOX_MAX_SHM_SEGMENTS 2 CACHE STRING "") | ||
set(IOX_MAX_NUMBER_OF_MEMPOOLS 3 CACHE STRING "") | ||
set(IOX_MAX_NUMBER_OF_CONDITION_VARIABLES 8 CACHE STRING "") | ||
set(IOX_MAX_NODE_NAME_LENGTH 30 CACHE STRING "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatthiasKillat 32 would lead to 7 wasted bytes due to the string capacity being length + 1
. What would work would be 31 but that looks ugly and I think keeping it at 30 is a good aesthetic compromise.
[[segment]] | ||
|
||
[[segment.mempool]] | ||
size = 512 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, it is a runtime option, not a compile time one.
macro(configure_option) | ||
set(ONE_VALUE_ARGS NAME DEFAULT_VALUE) | ||
cmake_parse_arguments(CONFIGURE_OPTION "" "${ONE_VALUE_ARGS}" "" ${ARGN}) | ||
|
||
if(NOT ${CONFIGURE_OPTION_NAME}) | ||
set(${CONFIGURE_OPTION_NAME} ${CONFIGURE_OPTION_DEFAULT_VALUE}) | ||
endif() | ||
message(STATUS "[i] ${CONFIGURE_OPTION_NAME}: " ${${CONFIGURE_OPTION_NAME}}) | ||
endmacro() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
# note: don't change IOX_INTERNAL_MAX_NUMBER_OF_NOTIFIERS value because it could break the C-Binding | ||
#if(NOT IOX_MAX_NUMBER_OF_NOTIFIERS) | ||
#configure_option( | ||
# NAME IOX_MAX_NUMBER_OF_NOTIFIERS | ||
# DEFAULT_VALUE 256 | ||
#) | ||
set(IOX_INTERNAL_MAX_NUMBER_OF_NOTIFIERS 256) | ||
#endif() | ||
#message(STATUS "[i] IOX_MAX_NUMBER_OF_NOTIFIERS:" ${IOX_MAX_NUMBER_OF_NOTIFIERS}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a remnant of the v2 release crunch time. AFAIK the note is not true any more and we could remove INTERNAL
. @elfenpiff @dkroenke please correct me if I'm wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elBoberido @JakubSosnovec Yes this can be removed (but not necessarily in this PR to delay this not further here).
/// @NOLINTBEGIN(cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers) | ||
template <> | ||
inline bool convert::fromString<string<100>>(const char* v, string<100>& dest) noexcept | ||
template <uint64_t Capacity> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*cough* CString100
*cough*
ce3dddd
f22d406
to
ce3dddd
Compare
Rebased to latest master, to fix conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for the contribution.
Pre-Review Checklist for the PR Author
iox-123-this-is-a-branch
)iox-#123 commit text
)task-list-completed
)iceoryx_hoofs
are added to./clang-tidy-diff-scans.txt
Notes for Reviewer
Based on this discussion: #1770
Checklist for the PR Reviewer
iceoryx_hoofs
have been added to./clang-tidy-diff-scans.txt
Post-review Checklist for the PR Author
References