[core][rdt] implement sender side memory pool + perf improvements - #65205
Conversation
Signed-off-by: Joshua Lee <joshlee@anyscale.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the NIXL memory pool management to pack tensors into contiguous pool blocks based on object IDs rather than individual storage pointers, and introduces several microbenchmarks to measure weight-syncing performance. The review feedback highlights critical bugs, including an AttributeError from accessing dtype.itemsize (which is not a valid PyTorch attribute), RuntimeErrors when calling get_device() on CPU tensors, and a potential cleanup failure when handling partially initialized tensor lists during exceptions. Additionally, performance optimizations are suggested to reduce
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit ea81e8b. Configure here.
stephanie-wang
left a comment
There was a problem hiding this comment.
Looks good, just had some comments on simplifying it a bit. Still reviewing tests.
| Consecutive tensors that are adjacent in both the source storage and the | ||
| pool are copied together as one device copy. Weight-sync layouts, where | ||
| the tensors are ordered views of one weight, collapse to a single copy | ||
| per block. Anything else, such as interleaved order or separately | ||
| allocated tensors, simply forms chains of one and copies per tensor. |
There was a problem hiding this comment.
I think this is a bit of an over-optimization, not clear this is a common use case.
There was a problem hiding this comment.
I think this mainly matters if you do a ray.put on a bunch of contiguous views for the memory pool. I found that for the 10k view experiment that I did it added around 200ms to ray.put , and with this optimization it was around 2ms. I could put this in a separate PR if it's a bit easier to review.
| # NIXL requires the local and remote lists to agree on descriptor | ||
| # count and length, so build both together: one descriptor for a | ||
| # whole group when the user's buffers already match the packed | ||
| # layout, otherwise one per tensor. |
There was a problem hiding this comment.
I think the chance of this case where you are using packed tensors and the user's buffers match the packed tensors exactly isn't very high and it makes the code harder to follow, so I would remove this optimization for now.
There was a problem hiding this comment.
hmmm yea that makes sense, I think my only hesitancy would be that in the 10k view case we would then always do 10k NIXL READ calls which would increase the latency by like 3x then doing it all in one pull. It was a bit of an engineered example though. Should I just leave it as a follow up for now?
|
Yes, I think we should leave out the optimizations that I commented on earlier. I'm not sure if it makes sense to merge them at all, even in a follow-up PR. We can sync about that offline. |
Sounds good, removed them |

Rewrite of the RDT NIXL memory pool + a couple performance improvements.
The original RDT NIXL memory pool had a couple key issues, most notably:
1.) It copied the entire storage tensor backing the views
2.) It did not collapse the transfer descriptors resulting in the transfer speed scaling with the number of tensors stored into the pool, even though they were backed by contiguous memory
The new pool solves both of these issues, and only copies the user provided views into the memory pool. It does this in two phases: