Repository navigation
Conversation
|
👋 Hi iyastreb! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
|
/build |
ColinNV
left a comment
ColinNV
left a comment
There was a problem hiding this comment.
And as discussed I believe it would be cleaner and less error-prone to keep the two cases (owning, non-owning) somehow separate, however that's now possible at this point.
| * Can be called only in owner mode (not shallow copy), after vector is modified | ||
| */ | ||
| inline void | ||
| syncView() { |
There was a problem hiding this comment.
The inline is superfluous, a noexcept could be added.
| * @param size Size of the descriptor list | ||
| * @return Shallow copy of the descriptor list | ||
| */ | ||
| static const nixlDescList<T> |
There was a problem hiding this comment.
const does nothing when returning by value.
| /** @var Vector for storing nixlDescs */ | ||
| std::vector<T> descs; | ||
| /** @var Whether the descriptor list is a shallow copy */ | ||
| bool isShallowCopy_; |
There was a problem hiding this comment.
I wonder whether "shallow copy" is the right term, how about something like non-owning view, or read-only view?
| return size_; | ||
| } | ||
|
|
||
| inline size_t |
There was a problem hiding this comment.
inline does nothing here, noexcept can be added.
| inline bool | ||
| isEmpty() const { | ||
| return (descs.size() == 0); | ||
| return (size_ == 0); |
There was a problem hiding this comment.
Brackets are superfluous (as is the inline while noexcept is missing).
| if (size <= 1) return (size == 1); | ||
| for (int i = 0; i < size - 1; ++i) { | ||
| if (vec[i + 1] < vec[i]) return false; | ||
| size_t count = size(); |
| if (vec[i + 1] < vec[i]) return false; | ||
| size_t count = size(); | ||
| if (count <= 1) { | ||
| return (count == 1); |
There was a problem hiding this comment.
Brackets are superfluous, and why is an empty list considered not sorted?
| if (count <= 1) { | ||
| return (count == 1); | ||
| } | ||
| for (size_t i = 0; i < count - 1; ++i) { |
There was a problem hiding this comment.
Could probably be implemented with std::is_sorted() or std::is_sorted_until().
| if (itr == this->descs.end()) return NIXL_ERR_NOT_FOUND; | ||
| if (static_cast<const nixlBasicDesc &>(*itr) == query) | ||
| return static_cast<int>(itr - this->descs.begin()); | ||
| auto itr = std::lower_bound(begin(), end(), query); |
| auto itr = std::lower_bound(this->descs.begin(), this->descs.end(), query); | ||
| if (itr != this->descs.end() && itr->covers(query)) | ||
| return static_cast<int>(itr - this->descs.begin()); | ||
| auto itr = std::lower_bound(begin(), end(), query); |
| void | ||
| nixlDescList<T>::remDesc(size_t index) { | ||
| checkModifiable(); | ||
| if ((index >= descs.size()) || (index < 0)) { |
There was a problem hiding this comment.
| if ((index >= descs.size()) || (index < 0)) { | |
| if (index >= descs.size()) { |
index < 0 is always false for size_t
| /** @var Whether the descriptor list is a shallow copy */ | ||
| bool isShallowCopy_; | ||
| /** @var Common view of the descriptor list, both for shallow copy and own data */ | ||
| const T *view_; |
There was a problem hiding this comment.
Could we also use std::variant to hold either the "view" type (data + size) or a std::vector<T>?
It might not be as simple but it would avoid any bugs related to using descs while _isShallowCopy is true
| nixlDescList<nixlBasicDesc> trimmed(type); | ||
| nixlBasicDesc* p; | ||
|
|
||
| for (auto & elm: descs) { |
There was a problem hiding this comment.
Iterating over descs will not work here when it's a shallow copy
| std::string(reinterpret_cast<const char *>(begin()), n_desc * sizeof(nixlBasicDesc))); | ||
| if (ret) return ret; | ||
| } else { // already checked it can be only nixlBlobDesc or nixlSectionDesc | ||
| for (auto & elm : descs) { |
There was a problem hiding this comment.
Iterating over descs will not work here when it's a shallow copy
| for (size_t i=0; i<lhs.descs.size(); ++i) | ||
| if (lhs.descs[i] != rhs.descs[i]) |
There was a problem hiding this comment.
Accesing descs directly will not work here when it's a shallow copy.
📝 WalkthroughWalkthroughThe changes introduce shallow-copy semantics to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/infra/nixl_descriptors.cpp (1)
189-243:⚠️ Potential issue | 🟡 MinorDeserializer calls
clear()beforetypeis initialized.Line 197 calls
clear(), which callscheckModifiable(). However, at this pointtypehasn't been deserialized yet (happens at line 207). WhileisShallowCopy_is initialized tofalsein the member initializer list (line 191), callingclear()before full initialization seems fragile. Consider movingclear()after basic validation or initializingdescsempty by default.Additionally,
view_andsize_are initialized tonullptrand0butsyncView()is only called after deserialization succeeds, leaving them inconsistent during parsing.Suggested improvement
template<class T> nixlDescList<T>::nixlDescList(nixlSerDes *deserializer) : isShallowCopy_(false), view_(nullptr), size_(0) { size_t n_desc; std::string str; - clear(); + // descs is default-constructed empty, no need to clear + // syncView will be called after successful parsing str = deserializer->getStr("nixlDList"); // Object type🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infra/nixl_descriptors.cpp` around lines 189 - 243, The constructor nixlDescList<T>::nixlDescList currently calls clear() before 'type' is deserialized which triggers checkModifiable() and can be fragile; move the clear() call to after the initial validation that reads "nixlDList" and successfully deserializes 'type' and 'n' (the getBuf("t", &type, ...) and getBuf("n", &n_desc, ... ) calls) so clear() only runs once the object has a valid type, or alternatively ensure descs is default-initialized empty and clear() does not rely on an initialized 'type' (update clear() to be safe when type is uninitialized); also ensure view_ and size_ remain consistent by calling syncView() only after deserialization succeeds (leave view_/size_ as nullptr/0 until then) and reference the functions/members: clear(), getStr(), getBuf("t", &type...), getBuf("n", &n_desc...), descs, isShallowCopy_, view_, size_, and syncView() when making changes.src/api/cpp/nixl_descriptors.h (1)
17-18: 🧹 Nitpick | 🔵 TrivialHeader guard doesn't follow full-path convention.
Per repository conventions, header guards should include the full repository-relative path in uppercase with
NIXL_prefix. Consider updating toNIXL_SRC_API_CPP_NIXL_DESCRIPTORS_H.Suggested fix
-#ifndef _NIXL_DESCRIPTORS_H -#define _NIXL_DESCRIPTORS_H +#ifndef NIXL_SRC_API_CPP_NIXL_DESCRIPTORS_H +#define NIXL_SRC_API_CPP_NIXL_DESCRIPTORS_HAnd at the end of the file:
-#endif +#endif // NIXL_SRC_API_CPP_NIXL_DESCRIPTORS_HBased on learnings: "Enforce header guards to include the full repository-relative path in uppercase, with slashes replaced by underscores, prefixed by NIXL_, and suffixed with _H."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/cpp/nixl_descriptors.h` around lines 17 - 18, The header guard currently uses _NIXL_DESCRIPTORS_H; update it to follow repo convention by replacing both occurrences of the guard with NIXL_SRC_API_CPP_NIXL_DESCRIPTORS_H (and update the trailing `#endif` comment if present) so the guard includes the full repository-relative path in uppercase with underscores and the NIXL_ prefix and _H suffix.
♻️ Duplicate comments (3)
src/infra/nixl_descriptors.cpp (3)
285-295:⚠️ Potential issue | 🟠 Major
trim()iterates overdescsdirectly — broken for shallow copies.When
isShallowCopy_is true,descsis empty and this loop produces an empty result instead of trimming the actual view data.Suggested fix
template <class T> nixlDescList<nixlBasicDesc> nixlDescList<T>::trim() const { if constexpr (std::is_same<nixlBasicDesc, T>::value) { return *this; } else { nixlDescList<nixlBasicDesc> trimmed(type); - nixlBasicDesc* p; - - for (auto & elm: descs) { - p = (nixlBasicDesc*) (&elm); - trimmed.addDesc(*p); + for (const auto &elm : *this) { + trimmed.addDesc(static_cast<const nixlBasicDesc &>(elm)); } // No failure scenario return trimmed; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infra/nixl_descriptors.cpp` around lines 285 - 295, The loop in trim() iterates over descs, which is empty for shallow copies (isShallowCopy_ == true), so change the iteration to use the actual view container when shallow: check isShallowCopy_ and iterate over the view backing (e.g., the container that holds the view elements / descriptors) instead of descs, then cast each element to nixlBasicDesc* and call trimmed.addDesc(*p) as before; implement this by branching on isShallowCopy_ (or by selecting the correct container via a helper like getViewDescs()) so trimmed contains the real descriptors for shallow copies.
387-395:⚠️ Potential issue | 🟠 Major
operator==accessesdescsdirectly — broken for shallow copies.Lines 391-393 iterate over
lhs.descsandrhs.descs, which are empty for shallow copies, causing false equality results.Suggested fix
template <class T> bool operator==(const nixlDescList<T> &lhs, const nixlDescList<T> &rhs) { - if ((lhs.getType() != rhs.getType()) || (lhs.descCount() != rhs.descCount())) return false; + if ((lhs.getType() != rhs.getType()) || (lhs.size() != rhs.size())) return false; - for (size_t i=0; i<lhs.descs.size(); ++i) - if (lhs.descs[i] != rhs.descs[i]) + for (size_t i = 0; i < lhs.size(); ++i) + if (lhs[i] != rhs[i]) return false; return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infra/nixl_descriptors.cpp` around lines 387 - 395, The operator== implementation in nixlDescList<T> directly accesses the private member descs (in operator==(const nixlDescList<T>&,...)) which is empty for shallow copies; instead iterate using descCount() and compare elements via the class's public accessor (e.g., getDesc(i) or descAt(i)) for both lhs and rhs to avoid touching the internal vector; modify the loop in operator== to run i from 0 to lhs.descCount() and compare lhs.getDesc(i) with rhs.getDesc(i) (using the actual accessor name defined on nixlDescList<T>).
341-346:⚠️ Potential issue | 🟠 Major
serialize()iterates overdescsdirectly — broken for shallow copies.The else-branch (lines 343-346) iterates
for (auto & elm : descs)which is empty for shallow copies, producing incorrect serialization output.Suggested fix
} else { // already checked it can be only nixlBlobDesc or nixlSectionDesc - for (auto & elm : descs) { + for (const auto &elm : *this) { ret = serializer->addStr("", elm.serialize()); if (ret) return ret; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/infra/nixl_descriptors.cpp` around lines 341 - 346, serialize() is iterating over the local variable descs which can be an empty shallow copy, causing missing output; change the loop to iterate the actual container (e.g., use this->descs or accept/use a const reference to the original container) so serializer->addStr("", elm.serialize()) runs against the real elements (replace for (auto & elm : descs) with for (auto & elm : this->descs) or adjust the function signature to take const std::vector<...>& descs and iterate that).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/cpp/nixl_descriptors.h`:
- Around line 384-387: The descCount() method currently returns int while
reading size_ (a size_t), risking silent narrowing; change descCount() to return
size_t to match size(), or if int is required for API compatibility, add an
explicit bounds check in descCount() that caps values > INT_MAX (using size_ and
INT_MAX) and handles overflow (e.g., throw or return INT_MAX) to avoid undefined
behavior—update the signature and any callers of descCount() accordingly (refer
to descCount() and size_).
- Around line 480-481: The remDesc(size_t) declaration causes negative ints
(e.g. from getIndex()) to wrap to huge unsigned values and bypass the (index <
0) check; fix by reverting remDesc to accept a signed int (e.g. remDesc(int
index)) or, if you must keep size_t, add explicit validation at the top of the
implementation: checkModifiable(); verify index is within bounds and not
negative by using a signed comparison (cast descs.size() to int or check the
source int before conversion) and throw std::out_of_range on failure before
calling descs.erase or syncView; update call sites (e.g. where getIndex() is
used in src/infra/nixl_memory_section.cpp and the Rust C API wrappers) if you
change the signature.
---
Outside diff comments:
In `@src/api/cpp/nixl_descriptors.h`:
- Around line 17-18: The header guard currently uses _NIXL_DESCRIPTORS_H; update
it to follow repo convention by replacing both occurrences of the guard with
NIXL_SRC_API_CPP_NIXL_DESCRIPTORS_H (and update the trailing `#endif` comment if
present) so the guard includes the full repository-relative path in uppercase
with underscores and the NIXL_ prefix and _H suffix.
In `@src/infra/nixl_descriptors.cpp`:
- Around line 189-243: The constructor nixlDescList<T>::nixlDescList currently
calls clear() before 'type' is deserialized which triggers checkModifiable() and
can be fragile; move the clear() call to after the initial validation that reads
"nixlDList" and successfully deserializes 'type' and 'n' (the getBuf("t", &type,
...) and getBuf("n", &n_desc, ... ) calls) so clear() only runs once the object
has a valid type, or alternatively ensure descs is default-initialized empty and
clear() does not rely on an initialized 'type' (update clear() to be safe when
type is uninitialized); also ensure view_ and size_ remain consistent by calling
syncView() only after deserialization succeeds (leave view_/size_ as nullptr/0
until then) and reference the functions/members: clear(), getStr(), getBuf("t",
&type...), getBuf("n", &n_desc...), descs, isShallowCopy_, view_, size_, and
syncView() when making changes.
---
Duplicate comments:
In `@src/infra/nixl_descriptors.cpp`:
- Around line 285-295: The loop in trim() iterates over descs, which is empty
for shallow copies (isShallowCopy_ == true), so change the iteration to use the
actual view container when shallow: check isShallowCopy_ and iterate over the
view backing (e.g., the container that holds the view elements / descriptors)
instead of descs, then cast each element to nixlBasicDesc* and call
trimmed.addDesc(*p) as before; implement this by branching on isShallowCopy_ (or
by selecting the correct container via a helper like getViewDescs()) so trimmed
contains the real descriptors for shallow copies.
- Around line 387-395: The operator== implementation in nixlDescList<T> directly
accesses the private member descs (in operator==(const nixlDescList<T>&,...))
which is empty for shallow copies; instead iterate using descCount() and compare
elements via the class's public accessor (e.g., getDesc(i) or descAt(i)) for
both lhs and rhs to avoid touching the internal vector; modify the loop in
operator== to run i from 0 to lhs.descCount() and compare lhs.getDesc(i) with
rhs.getDesc(i) (using the actual accessor name defined on nixlDescList<T>).
- Around line 341-346: serialize() is iterating over the local variable descs
which can be an empty shallow copy, causing missing output; change the loop to
iterate the actual container (e.g., use this->descs or accept/use a const
reference to the original container) so serializer->addStr("", elm.serialize())
runs against the real elements (replace for (auto & elm : descs) with for (auto
& elm : this->descs) or adjust the function signature to take const
std::vector<...>& descs and iterate that).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef38289c-51b0-470f-ba7d-c6636e464c30
📒 Files selected for processing (3)
src/api/cpp/nixl_descriptors.hsrc/infra/mem_section.hsrc/infra/nixl_descriptors.cpp
| int | ||
| descCount() const { | ||
| return descs.size(); | ||
| return size_; | ||
| } |
There was a problem hiding this comment.
Potential narrowing conversion from size_t to int.
descCount() returns int but size_ is size_t. For lists exceeding INT_MAX elements, this silently overflows. Consider returning size_t or adding a narrowing check. The size() method correctly returns size_t.
Suggested fix
- int
+ size_t
descCount() const {
return size_;
}Or, if int return is required for API compatibility:
int
descCount() const {
+ assert(size_ <= static_cast<size_t>(std::numeric_limits<int>::max()));
return size_;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int | |
| descCount() const { | |
| return descs.size(); | |
| return size_; | |
| } | |
| size_t | |
| descCount() const { | |
| return size_; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/cpp/nixl_descriptors.h` around lines 384 - 387, The descCount()
method currently returns int while reading size_ (a size_t), risking silent
narrowing; change descCount() to return size_t to match size(), or if int is
required for API compatibility, add an explicit bounds check in descCount() that
caps values > INT_MAX (using size_ and INT_MAX) and handles overflow (e.g.,
throw or return INT_MAX) to avoid undefined behavior—update the signature and
any callers of descCount() accordingly (refer to descCount() and size_).
| void | ||
| remDesc(const int &index); | ||
| remDesc(size_t index); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify callers of remDesc that may pass negative values
echo "=== Checking remDesc callers with int index ==="
rg -n -B2 -A2 'remDesc\(' --type cpp
echo ""
echo "=== Checking getIndex return type and usage ==="
rg -n 'int.*getIndex|getIndex.*int' --type cppRepository: ai-dynamo/nixl
Length of output: 4346
🏁 Script executed:
# Check remDesc implementation
sed -n '259,270p' src/infra/nixl_descriptors.cpp
echo ""
echo "=== Check Rust wrapper signatures ==="
sed -n '1080,1100p' src/bindings/rust/wrapper.cpp
sed -n '1275,1295p' src/bindings/rust/wrapper.cpp
echo ""
echo "=== Check NIXL_ERR_NOT_FOUND definition ==="
rg -n 'NIXL_ERR_NOT_FOUND|define.*NOT_FOUND' --type cpp --type h | head -20
echo ""
echo "=== Check getIndex implementation ==="
sed -n '298,320p' src/infra/nixl_descriptors.cppRepository: ai-dynamo/nixl
Length of output: 4056
🏁 Script executed:
rg -n 'define NIXL_ERR_NOT_FOUND|NIXL_ERR_NOT_FOUND\s*=' --type hRepository: ai-dynamo/nixl
Length of output: 114
Critical: remDesc(size_t) silently converts negative indices to large values.
The signature change from int to size_t introduces a dangerous implicit conversion. Callers pass int values directly from getIndex() (which returns -4 for not-found), including in src/infra/nixl_memory_section.cpp and C API wrappers in src/bindings/rust/wrapper.cpp.
When a negative int is passed, it converts to a very large size_t value, and the bounds check (index < 0) in the implementation is always false for an unsigned type, allowing out-of-bounds access.
Suggested fix: Keep int signature with proper validation
void
- remDesc(size_t index);
+ remDesc(int index);Or add explicit validation in the implementation:
void
nixlDescList<T>::remDesc(int index) {
checkModifiable();
if ((index >= (int)descs.size()) || (index < 0)) {
throw std::out_of_range("Index is out of range");
}
descs.erase(descs.begin() + index);
syncView();
}Note: The current check (index < 0) when index is size_t is always false and does not prevent the conversion of negative values to large unsigned numbers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void | |
| remDesc(const int &index); | |
| remDesc(size_t index); | |
| void | |
| remDesc(int index); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/cpp/nixl_descriptors.h` around lines 480 - 481, The remDesc(size_t)
declaration causes negative ints (e.g. from getIndex()) to wrap to huge unsigned
values and bypass the (index < 0) check; fix by reverting remDesc to accept a
signed int (e.g. remDesc(int index)) or, if you must keep size_t, add explicit
validation at the top of the implementation: checkModifiable(); verify index is
within bounds and not negative by using a signed comparison (cast descs.size()
to int or check the source int before conversion) and throw std::out_of_range on
failure before calling descs.erase or syncView; update call sites (e.g. where
getIndex() is used in src/infra/nixl_memory_section.cpp and the Rust C API
wrappers) if you change the signature.
|
It's not relevant for 2 step API, which is now being used |
What?
Enabled zero-copy for nixlDescList by allowing shallow copy.
Why?
On large desc dimensions copying takes a lot of time. One of the copies is made inside Python binding, where we copy Python array into nixlDescList. This copy takes quite some time while holding the GIL lock. In certain cases we can eliminate this copy by making it shallow copy, when
initialize_xferis called sequentially afterget_xfer_descs.initialize_xfermakes its own copy (nixlMetaList), which can be also eliminated.The proposed solution is to optionally enable zero-copy mode for nixlDescList. Appropriate Python binding modification will be done in a separate PR.
How?
Added a common
view_that points either to owneddescsor to an external data. All the read accesses are done via this view, all the writes modify the view.Python API is in separate PR: #1325
Perf testing:
Summary by CodeRabbit
New Features
Bug Fixes
API Changes