Skip to content

Conversation

@kingcrimsontianyu
Copy link
Contributor

@kingcrimsontianyu kingcrimsontianyu commented Jan 13, 2026

Currently bounce buffer is non-copyable and non-movable. Creating a vector of bounce buffers requires the use of unique pointers as a workaround (std::vector<std::unique_ptr<Buffer>>). This PR lifts this limitation by making it move constructible and move assignable.

This PR makes the following additional improvements:

  • Add unit test for the bounce buffer pool class
  • Add public methods for buffer pool class used to query the state (number of free buffers held, size of the buffer) and facilitate testing
  • Mark some methods const

@copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 13, 2026

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@kingcrimsontianyu kingcrimsontianyu added breaking Introduces a breaking change improvement Improves an existing functionality c++ Affects the C++ API of KvikIO labels Jan 13, 2026
@kingcrimsontianyu kingcrimsontianyu marked this pull request as ready for review January 13, 2026 18:57
@kingcrimsontianyu kingcrimsontianyu requested review from a team as code owners January 13, 2026 18:57
@kingcrimsontianyu
Copy link
Contributor Author

Well. For a WIP PR, I no longer need the bounce buffer to be moveable. Is there any disadvantage of making it so? I'm not aware of any. But if there is, I can pivot this PR to just adding the unit test. Let me know what you think @madsbk. Thanks!

@madsbk
Copy link
Member

madsbk commented Jan 14, 2026

Well. For a WIP PR, I no longer need the bounce buffer to be moveable. Is there any disadvantage of making it so? I'm not aware of any. But if there is, I can pivot this PR to just adding the unit test. Let me know what you think @madsbk. Thanks!

Not that I’m aware of. The buffers themselves are allocated on the heap, so making the bounce buffer movable should not make a difference. That said, we could wait with this feature until it is actually needed. I don’t have a strong opinion either way.

@vuule
Copy link
Contributor

vuule commented Jan 15, 2026

might be better to merge this now if it's ready
no negative impact AFAICT

@kingcrimsontianyu
Copy link
Contributor Author

Thanks for the discussion.
This PR is now a dependency of #896, which uses a vector of bounce buffers to implement a k-way bounce buffer to reduce the occurrence of stream synchronization. So this can be a legitimate reason for making the bounce buffer moveable.

Copy link
Member

@madsbk madsbk left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @kingcrimsontianyu

PS: pushed a minor update to the docs

@kingcrimsontianyu
Copy link
Contributor Author

Thanks all for your reviews.

@kingcrimsontianyu
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 2c273a9 into rapidsai:main Jan 28, 2026
315 of 324 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Introduces a breaking change c++ Affects the C++ API of KvikIO improvement Improves an existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants