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

Core / ZeroCopy: Added Doxygen API documentation and 2 new c++ samples demonstrating usage of low level zero copy API #1169

Merged
merged 4 commits into from
Jul 31, 2023

Conversation

rex-schilasky
Copy link
Contributor

@rex-schilasky rex-schilasky commented Jul 29, 2023

Pull request type

Please check the type of change your PR introduces:

  • Feature
  • Documentation content changes

What is the current behavior?
We missed samples to demonstrate low level zero copy API and the CPayloadWriter class was not documented (Doxygen).

Issue Number: #1145

What is the new behavior?
Two new binary send / rec zero copy samples added, CPayloadWriter class documented.

Does this introduce a breaking change?

  • Yes
  • No

Cherry-Pick to

  • 5.12

…ry_zero_copy_rec demonstrating the usage of the low level zero copy API

documented CPayloadWriter (#1145)
@FlorianReimold
Copy link
Member

If I see correctly, this PR basically contains 2 changes:

  1. It adds the samples
  2. More importantly: It adds doxygen documentation to the payload writer (public API)

The latter change requires this commit to be cherry-picked to 5.12.

@rex-schilasky can you confirm this?

@rex-schilasky
Copy link
Contributor Author

If I see correctly, this PR basically contains 2 changes:

  1. It adds the samples
  2. More importantly: It adds doxygen documentation to the payload writer (public API)

The latter change requires this commit to be cherry-picked to 5.12.

@rex-schilasky can you confirm this?

This is all correct. The doxygen documented CPayloadWriter class and both samples demonstrating the usage of this class should go to 5.12.

@FlorianReimold FlorianReimold changed the title added 2 new c++ samples demonstrating usage of low level zero copy API Core / ZeroCopy: Added Doxygen API documentation and 2 new c++ samples demonstrating usage of low level zero copy API Jul 31, 2023
Copy link
Member

@FlorianReimold FlorianReimold left a comment

Choose a reason for hiding this comment

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

After reviewing the Doxygen I don't really get what "data to be written" means here. I assume that this term refers to the size of the underlying memory file?

const void* m_buffer = nullptr;
size_t m_size = 0;
const void* m_buffer = nullptr; ///< Pointer to the buffer containing the data to be written.
size_t m_size = 0; ///< Size of the data to be written.
Copy link
Member

Choose a reason for hiding this comment

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

Is this really the "data to be written"? If so, why isn't this just a temporary locally scoped variable? Or is this capacity of the memory file, as the GetSize() function says?

Either way, I guess either the documentation of the variable or the GetSize() function is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CBufferPayloadWriter is a kind of primitive wrapper of the classic CBufferPayloadWriter(const void* const buffer_, size_t size_) Send API. Basically it stores the arguments buffer_ and size_ into the internals of CBufferPayloadWriter and does the memcpy action in the Write call.
GetSize reports the needed size correctly, Write is just doing the copy.

@rex-schilasky rex-schilasky merged commit 942f01d into master Jul 31, 2023
13 checks passed
@rex-schilasky rex-schilasky deleted the feature/zero-copy-sample branch August 3, 2023 06:17
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