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

iox-eclipse-iceoryx#2040 support for iox::string in UnixDomainSocket #2122

Merged

Conversation

lucabart97
Copy link
Contributor

@lucabart97 lucabart97 commented Nov 28, 2023

By default, the UnixDomainSocket class relies on std::string objects to send and receive data, as defined by its interface. However, this approach can potentially lead to dynamic memory allocation when handling larger data payloads that exceed the stack space optimization (SSO) limit.

To address this issue, an alternative API has been introduced that enables data transmission and reception using iox::string. This approach allows for direct data manipulation within stack memory, effectively eliminating the need for dynamic memory allocation.

Pre-Review Checklist for the PR Author

  1. Add a second reviewer for complex new features or larger refactorings
  2. Code follows the coding style of CONTRIBUTING.md
  3. Tests follow the best practice for testing
  4. Changelog updated in the unreleased section including API breaking changes
  5. Branch follows the naming format (iox-123-this-is-a-branch)
  6. Commits messages are according to this guideline
  7. Update the PR title
    • Follow the same conventions as for commit messages
    • Link to the relevant issue
  8. Relevant issues are linked
  9. Add sensible notes for the reviewer
  10. All checks have passed (except task-list-completed)
  11. All touched (C/C++) source code files from iceoryx_hoofs are added to ./clang-tidy-diff-scans.txt
  12. Assign PR to reviewer

Notes for Reviewer

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
  • All touched (C/C++) source code files from iceoryx_hoofs have been added to ./clang-tidy-diff-scans.txt
  • PR title describes the changes

Post-review Checklist for the PR Author

  1. All open points are addressed and tracked via issues

References

@lucabart97 lucabart97 force-pushed the iox-2040-iox-string-unix-socket branch from c333659 to dfba380 Compare November 28, 2023 21:05
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (fe19a9f) 80.20% compared to head (641d81d) 80.22%.
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2122      +/-   ##
==========================================
+ Coverage   80.20%   80.22%   +0.01%     
==========================================
  Files         419      420       +1     
  Lines       16276    16306      +30     
  Branches     2252     2257       +5     
==========================================
+ Hits        13054    13081      +27     
+ Misses       2424     2422       -2     
- Partials      798      803       +5     
Flag Coverage Δ
unittests 80.01% <86.11%> (+0.01%) ⬆️
unittests_timing 15.64% <69.44%> (+0.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...hoofs/posix/ipc/include/iox/unix_domain_socket.hpp 100.00% <ø> (ø)
...oryx_hoofs/posix/ipc/source/unix_domain_socket.cpp 56.66% <100.00%> (-3.59%) ⬇️
...osix/ipc/include/iox/detail/unix_domain_socket.inl 83.05% <83.05%> (ø)

... and 3 files with indirect coverage changes

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did not yet look at the tests.

Unfortunately there are some merge conflicts due to the ongoing restructuring of iceoryx hoofs. I hope they are not too severe.

Could you also refer to #1693 in the PR description.

@lucabart97 lucabart97 force-pushed the iox-2040-iox-string-unix-socket branch 3 times, most recently from 1dbde0f to 0f61db8 Compare December 2, 2023 15:47
@lucabart97 lucabart97 force-pushed the iox-2040-iox-string-unix-socket branch 2 times, most recently from d857ba0 to 0a46ac7 Compare December 9, 2023 18:57
@elBoberido
Copy link
Member

@lucabart97 sorry for letting you wait so long. I was quite busy this week and will have a look at the PR tomorrow.

@lucabart97
Copy link
Contributor Author

@elBoberido no problem, take your time!

Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just skimmed through the tests but with the new behavior of having buffers which are potentially smaller than the message, new tests need to check this behavior.

I think a test with a iox::string full of e.g. a will unveil an off by one error in the receive method.

@lucabart97 lucabart97 force-pushed the iox-2040-iox-string-unix-socket branch from 0a46ac7 to 1fb9063 Compare December 23, 2023 13:18
@lucabart97
Copy link
Contributor Author

lucabart97 commented Dec 23, 2023

I just skimmed through the tests but with the new behavior of having buffers which are potentially smaller than the message, > new tests need to check this behavior.

I think a test with a iox::string full of e.g. a will unveil an off by one error in the receive method.

I think the tests starting from this line do what do you suggest to do:

https://github.com/lucabart97/iceoryx/blob/1fb9063e91dc4602e2d3bcc4febc12dc0d37074e/iceoryx_hoofs/test/moduletests/test_posix_ipc_unix_domain_sockets.cpp#L401
(401~509)

@elBoberido
Copy link
Member

I just skimmed through the tests but with the new behavior of having buffers which are potentially smaller than the message, > new tests need to check this behavior.

I think a test with a iox::string full of e.g. a will unveil an off by one error in the receive method.

I think the tests starting from this line do what do you suggest to do:

https://github.com/lucabart97/iceoryx/blob/1fb9063e91dc4602e2d3bcc4febc12dc0d37074e/iceoryx_hoofs/test/moduletests/test_posix_ipc_unix_domain_sockets.cpp#L401 (401~509)

Not quite. Currently the data array in iox::string is zero initialized. This means that an off by one error might slip through.

There should be a test with a message_t msg; which is set to e.g. a for the full length and there also needs to be a tests with an iox::string with a capacity smaller than the one from message_t.

@lucabart97 lucabart97 force-pushed the iox-2040-iox-string-unix-socket branch from 1fb9063 to daa65e1 Compare January 6, 2024 10:42
@lucabart97
Copy link
Contributor Author

lucabart97 commented Jan 6, 2024

I added a test with the iox::string full of a and different tests with different iox::string size in send() and receive()

@elBoberido
Copy link
Member

Will look at it tomorrow more closely. Just skimmed over the changes and found a small stylistic issue. For non type template parameter we usually use upper case for the name.

@lucabart97 lucabart97 force-pushed the iox-2040-iox-string-unix-socket branch from daa65e1 to 14bd023 Compare January 7, 2024 18:13
Copy link
Member

@elBoberido elBoberido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a small nitpick and one additional tests and from my point of view this can be merged. :)

Btw, next Tuesday is the next developer meetup and the week after there is another one to introduce the Rust based iceoryx2. You are welcome to join.

By default, the UnixDomainSocket class relies on std::string objects
to send and receive data, as defined by its interface. However, this
approach can potentially lead to dynamic memory allocation when
handling larger data payloads that exceed the stack space
optimization (SSO) limit.

To address this issue, an alternative API has been introduced that
enables data transmission and reception using iox::string.
This approach allows for direct data manipulation within stack
memory, effectively eliminating the need for dynamic memory
allocation.

Signed-off-by: Luca Bartoli <[email protected]>
@lucabart97 lucabart97 force-pushed the iox-2040-iox-string-unix-socket branch from 14bd023 to 641d81d Compare January 13, 2024 13:22
@lucabart97
Copy link
Contributor Author

lucabart97 commented Jan 13, 2024

@elBoberido
Done!

Sorry for the delay and thank you for the invitation, but last week I was on holiday!

@elBoberido elBoberido merged commit a2a472f into eclipse-iceoryx:master Jan 13, 2024
26 checks passed
@elBoberido
Copy link
Member

Thanks for your patience :)

There is usually one meetup on the first Tuesday each month. Your welcome to join whenever it suits you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants