Skip to content

feat(driver, rt): multi buffer pool#954

Open
Sherlock-Holo wants to merge 2 commits into
compio-rs:masterfrom
Sherlock-Holo:multi-buffer-pool
Open

feat(driver, rt): multi buffer pool#954
Sherlock-Holo wants to merge 2 commits into
compio-rs:masterfrom
Sherlock-Holo:multi-buffer-pool

Conversation

@Sherlock-Holo

Copy link
Copy Markdown
Contributor

this PR allow user create multi extra buffer pools, not just use the pre-created buffer pool

@George-Miao George-Miao changed the title Multi buffer pool feat(driver, rt): multi buffer pool Jun 17, 2026
@George-Miao
George-Miao self-requested a review June 17, 2026 07:06
@github-actions github-actions Bot added enhancement New feature or request package: driver Related to compio-driver package: runtime Related to compio-runtime labels Jun 17, 2026
@Berrysoft

Copy link
Copy Markdown
Member

How do you want to use with this new API? There's no API to specify which buffer pool to use in the IO types.

@Sherlock-Holo

Copy link
Copy Markdown
Contributor Author

How do you want to use with this new API? There's no API to specify which buffer pool to use in the IO types.

currently user can use driver op to submit a operation with the extra buffer pool

after this PR, we can modify or extend the managed io trait to allow user use the extra buffer pool directly

@Berrysoft

Copy link
Copy Markdown
Member

Our discussion focuses on two major points:

  • It's ugly to expose create_buffer_pool and release_buffer_pool. This PR exposes the former and put the latter in the drop method, making the buffer pool keeping a weak ref to the driver. We don't like this design.
  • It's inconvenient to accept an argument buffer_poll: &BufferPool in *Managed traits. It makes associated type unavoidable.

Here's my design to solve the problems.

If we don't want to play with reference cycles, we have to keep the buffer pools inside the driver / runtime. The exposed API should be a handle to the buffer pool:

impl Runtime {
    pub fn buffer_manager(&self) -> BufferManager;
}

struct BufferManager { /* ... */ }

impl BufferManager {
    pub fn create_pool(&self) -> BufferPoolHandle;
    pub fn get_pool(&self, index: usize) -> Option<BufferPoolHandle>;
    pub fn release_pool(&self, index: usize);
}

struct BufferPoolHandle(usize);

impl BufferPoolHandle {
    pub fn index(&self) -> usize;
    pub async fn with<F: Future>(&self, f: impl FnOnce() -> F) -> F::Output;
}

The managed APIs are not modified, but all managed methods should be called inside one BufferPoolHandle::with. For example,

handle.with(|| async {
    file.read_managed_at(0, 0).await.unwrap();
}).await;

Would like advice from @Sherlock-Holo and @George-Miao .

@George-Miao

Copy link
Copy Markdown
Member

@Berrysoft I don't think it's necessary to force all managed API to be called inside with. Maybe add this to FutureExt and if user doesn't say which pool to use, provide a default one like what we have right now.

@Berrysoft

Berrysoft commented Jul 8, 2026

Copy link
Copy Markdown
Member

Oh, fine. Then it might be like

file.read_managed_at(0, 0).with_buffer_pool(&handle).await;

@George-Miao

Copy link
Copy Markdown
Member

@Berrysoft Yeah LGTM

@Berrysoft

Copy link
Copy Markdown
Member

@Sherlock-Holo Could you remove the changes to compio-runtime in this PR? We could modify it later.

And I wonder if it's possible to avoid such a release queue with Weak<RefCell<VecDeque<u16>>>...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request package: driver Related to compio-driver package: runtime Related to compio-runtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants