Allow AM receive callback to control data allocation/copy#479
Open
pentschev wants to merge 15 commits intorapidsai:branch-0.46from
Open
Allow AM receive callback to control data allocation/copy#479pentschev wants to merge 15 commits intorapidsai:branch-0.46from
pentschev wants to merge 15 commits intorapidsai:branch-0.46from
Conversation
pentschev
added
feature request
This reverts commit d9a04ee.
wence-
requested changes
Oct 9, 2025
Contributor
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
I think this looks broadly good though I am slightly worried about the reinterpret_cast usage.
Comment on lines
+61
to
+62
| * @param[in] receiverCallbackInfo receiver callback info to execute when request completes, | ||
| * including user header. |
Contributor
There was a problem hiding this comment.
I don't understand this docstring addition. AFAICT the receiverCallbackInfo is not a function, so how can it be executed?
Comment on lines
+306
to
+307
| : _data(reinterpret_cast<const uint8_t*>(&value), | ||
| reinterpret_cast<const uint8_t*>(&value) + sizeof(T)) |
Contributor
There was a problem hiding this comment.
This is UB due to aliasing I think, you need to std::memcpy. The compiler will turn it in to the obvious thing.
| reinterpret_cast<const uint8_t*>(&value) + sizeof(T)) | ||
| { | ||
| static_assert(std::is_trivially_copyable_v<T>, "Type must be trivially copyable"); | ||
| static_assert(sizeof(T) > 0, "Type size must be greater than zero"); |
Contributor
There was a problem hiding this comment.
nit: No C++ type has zero size (using Empty = T[0] for some T is UB)
| * @returns String representation of the data. | ||
| */ | ||
| AmData(void* data, size_t length) : data(data), length(length) {} | ||
| [[nodiscard]] std::string asString() const { return std::string(_data.begin(), _data.end()); } |
Contributor
There was a problem hiding this comment.
This copies the values, is that what you want or would a std::string_view do?
| throw std::runtime_error("AmUserHeader size mismatch: expected " + std::to_string(sizeof(T)) + | ||
| " bytes, got " + std::to_string(_data.size())); | ||
| } | ||
| return *reinterpret_cast<const T*>(_data.data()); |
Contributor
There was a problem hiding this comment.
This needs to be std::memcpy as well, I think, due to aliasing issues.
Comment on lines
+175
to
+177
| * This method is used for delayed receive operations where `delayReceive`` was enabled. | ||
| * It takes a user-provided buffer and internally calls `ucp_am_recv_data_nbx`` to receive | ||
| * the AM data that was stored when the message first arrived. Returns a `Request`` object |
Contributor
There was a problem hiding this comment.
Suggested change
| * This method is used for delayed receive operations where `delayReceive`` was enabled. | |
| * It takes a user-provided buffer and internally calls `ucp_am_recv_data_nbx`` to receive | |
| * the AM data that was stored when the message first arrived. Returns a `Request`` object | |
| * This method is used for delayed receive operations where `delayReceive` was enabled. | |
| * It takes a user-provided buffer and internally calls `ucp_am_recv_data_nbx` to receive | |
| * the AM data that was stored when the message first arrived. Returns a `Request` object |
Start in markdown, finish in rst :)
| /** | ||
| * @brief Receive delayed Active Message data into a user-provided buffer pointer. | ||
| * | ||
| * This method is used for delayed receive operations where `delayReceive`` was enabled. |
Contributor
There was a problem hiding this comment.
Suggested change
| * This method is used for delayed receive operations where `delayReceive`` was enabled. | |
| * This method is used for delayed receive operations where `delayReceive` was enabled. |
| */ | ||
| class AmUserHeader { | ||
| private: | ||
| std::vector<uint8_t> _data; |
Contributor
There was a problem hiding this comment.
suggestion: use std::byte (these aren't characters...).
Also, do you need this to be a resizeable thing, or is it sufficient for this object to manage a std::byte *?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP