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-#1693 support for iox::string in NamedPipe #2173

Merged

Conversation

lucabart97
Copy link
Contributor

@lucabart97 lucabart97 commented Jan 30, 2024

By default, the NamedPipe 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

Copy link

codecov bot commented Jan 30, 2024

Codecov Report

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

Comparison is base (7135c57) 80.12% compared to head (da6cc54) 80.04%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2173      +/-   ##
==========================================
- Coverage   80.12%   80.04%   -0.09%     
==========================================
  Files         420      421       +1     
  Lines       16332    16361      +29     
  Branches     2267     2270       +3     
==========================================
+ Hits        13086    13096      +10     
- Misses       2411     2423      +12     
- Partials      835      842       +7     
Flag Coverage Δ
unittests 79.83% <34.66%> (-0.09%) ⬇️
unittests_timing 15.48% <0.00%> (-0.04%) ⬇️

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

Files Coverage Δ
iceoryx_hoofs/posix/ipc/include/iox/named_pipe.hpp 100.00% <ø> (ø)
iceoryx_hoofs/posix/ipc/source/named_pipe.cpp 55.17% <50.00%> (+4.88%) ⬆️
..._hoofs/posix/ipc/include/iox/detail/named_pipe.inl 29.82% <29.82%> (ø)

@elBoberido
Copy link
Member

@lucabart97 sorry for letting you wait so long. Not sure if I manage to do the review today but your PR is on the top of my todo list

@lucabart97 lucabart97 force-pushed the iox-1693-iox-string-namedpipe branch from e92b3dc to 6c017e9 Compare February 4, 2024 10:54
@lucabart97 lucabart97 changed the title iox-eclipse-iceoryx#1963 support for iox::string in NamedPipe iox-eclipse-iceoryx#1693 support for iox::string in NamedPipe Feb 4, 2024
@lucabart97 lucabart97 force-pushed the iox-1693-iox-string-namedpipe branch from 6c017e9 to bf251fa Compare February 4, 2024 11:00
elBoberido
elBoberido previously approved these changes Feb 4, 2024
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

@elBoberido elBoberido dismissed their stale review February 4, 2024 17:06

The commit message is wrong. Please use only iox-# and not iox-eclipse-iceoryx#. The eclipse-iceoryx is added only by the github web frontend. I would also encourage to use the git hooks since they take care of this automatically.

@lucabart97 lucabart97 force-pushed the iox-1693-iox-string-namedpipe branch from bf251fa to dbfa133 Compare February 4, 2024 19:47
@lucabart97 lucabart97 changed the title iox-eclipse-iceoryx#1693 support for iox::string in NamedPipe iox-#1693 support for iox::string in NamedPipe Feb 4, 2024
@lucabart97
Copy link
Contributor Author

@elBoberido Done!

By default, the NamedPipe 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-1693-iox-string-namedpipe branch from dbfa133 to da6cc54 Compare February 4, 2024 19:48
@elBoberido elBoberido merged commit 80bbdf9 into eclipse-iceoryx:master Feb 4, 2024
26 checks passed
@elBoberido
Copy link
Member

@lucabart97 thanks. Next step would be using the new API

@lucabart97
Copy link
Contributor Author

lucabart97 commented Feb 5, 2024

@elBoberido
What should would I do? Starting from which part of the code?
I could start finding and replacing all the send methods of those classes, using the new API.
But if I remember correctly, there is another issue in the message serialization, right?
iceoryx_dust/include/iceoryx_dust/cxx/serialization.hpp

@elBoberido
Copy link
Member

@lucabart97 yes, the serialization would be the next step. There are essentially two options. One short term solution would be to make the current serialization work with iox::string. This should not be too hard and also not take too much time, although it might not be the most performant solution it should be better than the current one. In the long run we would like to improve the serialization. See also #503. Since the proposal from the comment in #503 would require quite a big refactoring, a stepping stone would be to start with the current concept and make the de-serialization work with iox::string first and as second step the serialization. For the serialization itself, I think some of the iox::convert functions need to be adapted to work with an iox::string. Here some of the building blocks from the logger could be re-used.

@lucabart97
Copy link
Contributor Author

lucabart97 commented Feb 6, 2024

@elBoberido
So if I understood correctly I should start to support iox::string in the serialization method and later move forward to use the new posix/ipc/* API?

@elBoberido
Copy link
Member

@lucabart97 yes starting to support iox::string in the serialization functions. From my gut feelings I think the de-serialization part might be simpler but it's up to you to choose :)

@lucabart97
Copy link
Contributor Author

@elBoberido
Yesterday I tried to participate in the meeting, I joined it, but we had some connection issues, maybe next time!

Ok, I start to understand the serialization part and try to support also iox::string!

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.

Use Zero-Copy receive in all IPC-Channel types and the posh IPC channel
2 participants