RUST: RegDescList and XferDescList APIs #828
Conversation
|
👋 Hi evgeny-leksikov! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
evgeny-leksikov
force-pushed
the
rust_dl_apis
branch
from
a0684b8 to
f89cb64
Compare
evgeny-leksikov
changed the title
evgeny-leksikov
force-pushed
the
rust_dl_apis
branch
from
f89cb64 to
aab2365
Compare
evgeny-leksikov
force-pushed
the
rust_dl_apis
branch
from
aab2365 to
7656037
Compare
| inner: NonNull<bindings::nixl_capi_reg_dlist_s>, | ||
| _phantom: PhantomData<&'a dyn NixlDescriptor>, | ||
| // Track descriptors internally for comparison purposes | ||
| descriptors: Vec<RegDescriptor>, |
There was a problem hiding this comment.
Why do we need to maintain duplicate storage, with all the implications?
Instead of simply exposing operators from C++...
Am I missing something?
patch.txt
There was a problem hiding this comment.
Index and IndexMut
There was a problem hiding this comment.
It looks like a nice optimization, but I'm afraid some of those APIs may have (or will have in the future) some side effects and checks that we are skipping that way, IMO RegDescList should call the bindings directly, and maybe have another struct called "CachedRegDescList" or make those implementations interchangeable with a configuration flag, making it less error prone, giving a simple workaround in case some incosistencies happen on users end because of this.
There was a problem hiding this comment.
I added sync_mgr to automate consistency and avoid accidental mutation in future changes
|
/build |
evgeny-leksikov
force-pushed
the
rust_dl_apis
branch
from
caefe51 to
dbbf296
Compare
- equality operators - set/get - get index Signed-off-by: Evgeny Leksikov <[email protected]>
Add SyncManager absraction to replace manual control Signed-off-by: Evgeny Leksikov <[email protected]>
Signed-off-by: Evgeny Leksikov <[email protected]>
evgeny-leksikov
force-pushed
the
rust_dl_apis
branch
from
dbbf296 to
e6755ff
Compare
|
/build |
| } | ||
|
|
||
| /// Mutates the frontend data (marks as dirty) | ||
| pub fn mutate<F, R>(&mut self, f: F) -> R |
There was a problem hiding this comment.
Minor: Maybe we should rename this function to modify, as semantically mutate may be associated mistakenly with a modification of a replica/copy.
Although we can think of it as a copy of the C++ object, it may not be clear to the next developer working on this
| // Test XferDescList equality scenarios (same tests as RegDescList) | ||
| { | ||
| // 1. Empty lists of same type should be equal | ||
| let xfer_list1 = XferDescList::new(MemType::Dram).unwrap(); | ||
| let xfer_list2 = XferDescList::new(MemType::Dram).unwrap(); | ||
| assert_eq!(xfer_list1, xfer_list2); | ||
| assert!(!(xfer_list1 != xfer_list2)); | ||
|
|
||
| // 2. Lists with different memory types should not be equal | ||
| let xfer_list_vram = XferDescList::new(MemType::Vram).unwrap(); | ||
| assert_ne!(xfer_list1, xfer_list_vram); | ||
| assert!(!(xfer_list1 == xfer_list_vram)); | ||
|
|
||
| // 3. Lists with same descriptors should be equal | ||
| let mut xfer_list3 = XferDescList::new(MemType::Dram).unwrap(); | ||
| let mut xfer_list4 = XferDescList::new(MemType::Dram).unwrap(); | ||
|
|
||
| xfer_list3.add_desc(0x1000, 0x100, 0).unwrap(); | ||
| xfer_list3.add_desc(0x2000, 0x200, 1).unwrap(); | ||
|
|
||
| xfer_list4.add_desc(0x1000, 0x100, 0).unwrap(); | ||
| xfer_list4.add_desc(0x2000, 0x200, 1).unwrap(); | ||
|
|
||
| assert_eq!(xfer_list3, xfer_list4); | ||
| assert!(!(xfer_list3 != xfer_list4)); | ||
|
|
||
| // 4. Lists with different descriptors should not be equal | ||
| let mut xfer_list5 = XferDescList::new(MemType::Dram).unwrap(); | ||
| let mut xfer_list6 = XferDescList::new(MemType::Dram).unwrap(); | ||
|
|
||
| xfer_list5.add_desc(0x1000, 0x100, 0).unwrap(); | ||
| xfer_list6.add_desc(0x2000, 0x200, 1).unwrap(); | ||
|
|
||
| assert_ne!(xfer_list5, xfer_list6); | ||
| assert!(!(xfer_list5 == xfer_list6)); | ||
|
|
||
| // 5. Lists with different lengths should not be equal | ||
| let mut xfer_list7 = XferDescList::new(MemType::Dram).unwrap(); | ||
| let xfer_list8 = XferDescList::new(MemType::Dram).unwrap(); | ||
|
|
||
| xfer_list7.add_desc(0x1000, 0x100, 0).unwrap(); | ||
|
|
||
| assert_ne!(xfer_list7, xfer_list8); | ||
| assert!(!(xfer_list7 == xfer_list8)); | ||
|
|
||
| // 6. Lists with same descriptors but different order should not be equal | ||
| let mut xfer_list9 = XferDescList::new(MemType::Dram).unwrap(); | ||
| let mut xfer_list10 = XferDescList::new(MemType::Dram).unwrap(); | ||
|
|
||
| xfer_list9.add_desc(0x1000, 0x100, 0).unwrap(); | ||
| xfer_list9.add_desc(0x2000, 0x200, 1).unwrap(); | ||
|
|
||
| xfer_list10.add_desc(0x2000, 0x200, 1).unwrap(); | ||
| xfer_list10.add_desc(0x1000, 0x100, 0).unwrap(); | ||
|
|
||
| assert_ne!(xfer_list9, xfer_list10); | ||
| assert!(!(xfer_list9 == xfer_list10)); | ||
| } |
There was a problem hiding this comment.
This is a separate test; it would be better to move it to a separate test function
| // Test RegDescList equality scenarios | ||
| { | ||
| // 1. Empty lists of same type should be equal | ||
| let reg_list1 = RegDescList::new(MemType::Dram).unwrap(); | ||
| let reg_list2 = RegDescList::new(MemType::Dram).unwrap(); | ||
| assert_eq!(reg_list1, reg_list2); | ||
| assert!(!(reg_list1 != reg_list2)); | ||
|
|
||
| // 2. Lists with different memory types should not be equal | ||
| let reg_list_vram = RegDescList::new(MemType::Vram).unwrap(); | ||
| assert_ne!(reg_list1, reg_list_vram); | ||
| assert!(!(reg_list1 == reg_list_vram)); | ||
|
|
||
| // 3. Lists with same descriptors should be equal | ||
| let mut reg_list3 = RegDescList::new(MemType::Dram).unwrap(); | ||
| let mut reg_list4 = RegDescList::new(MemType::Dram).unwrap(); | ||
|
|
||
| reg_list3.add_desc(0x1000, 0x100, 0).unwrap(); | ||
| reg_list3.add_desc(0x2000, 0x200, 1).unwrap(); | ||
|
|
||
| reg_list4.add_desc(0x1000, 0x100, 0).unwrap(); | ||
| reg_list4.add_desc(0x2000, 0x200, 1).unwrap(); |
There was a problem hiding this comment.
It feels like 1, 2, 3, 4, 5, 6, 7 can each be a test on its own. It would also be much easier to look at and understand what failed once one of those scenarios fails.
Also, there are a few lines here and there that look like code duplication that we can avoid
|
|
||
| // Tests for equality operators on RegDescList and XferDescList | ||
| #[test] | ||
| fn test_descriptor_list_equality() { |
There was a problem hiding this comment.
Maybe we should add some tests that trigger the synchronization manager and make sure it works properly. Maybe we can do it directly by creating a class that implements the trait just to assert that the synchronization happened
4 participants
What?
RegDescList and XferDescList missed APIs: