`FixedBufPool::next` #199

mzabaluev · 2022-12-06T01:28:46Z

Add an async API to get buffers out of FixedBufPool. This is another way in which FixedBufPool will be more useful than FixedBufRegistry.

Also reorganize the shared data structures of FixedBufPool and FixedBufRegistry into separate modules for better readability and future code reuse.

Move the Inner types of FixedBufRegistry and FixedBufPool into a plumbing module and expose them within buf::fixed, to allow code reuse with the upcoming thread-safe collections.

Enable waiting for fixed buffers to become available asynchronously.

The async-friendly wrapper over FixedBufPool::poll_next. Also document the method and the functionality it enables for FixedBufPool. buf::fixed::pool is now a public module and hosts the Next future.

Noah-Kennedy · 2022-12-06T01:36:05Z

ugh, I did not mean to remove the draft status

src/buf/fixed/pool.rs

FrankReh

Nice.

src/buf/fixed/plumbing/pool.rs

FrankReh

The direction is great. Please consider my two outstanding questions. Thank you.

src/buf/fixed/plumbing/pool.rs

The wakeup mechanism is now based on tokio::sync::Notify and wakes up a single task pending for the checked-in capacity buffer every time. Remove FixedBufPool::poll_next and the named Next future. These could not be rewritten with the Notified future without replicating some of the unsafe magic. In the future, TAIT can resurrect the Next named type.

Thanks clippy.

mzabaluev · 2022-12-15T23:36:37Z

src/buf/fixed/plumbing/pool.rs

+        })
+    }
+
+    pub(crate) fn notify_on_next(&mut self, cap: usize) -> Arc<Notify> {


The Arc is not necessary for the single-threaded pool, but I'm going to reuse this for the thread-safe implementation. Notify contains a mutex and uses atomic pointers for tracking waiters, so it's going to be slow regardless when we hit the waiting branch in next. Performance of next in the depleted pool case is not high on my current priority list for micro-optimizations.

But we are left without a simple/fast pool, that doesn't allocate, for the current-thread case. The case I care about. But all is not lost, once the API allows us to provide our own pool.

I'd just like to clarify my understanding here: once a notify has been registered for a capacity, then every time a buffer of that capacity is commited, notify_one() is called (because the notify_next_by_cap entry is never removed)?

I think this could be improved by keeping a (usize, Notify) instead, to explicitly keep a count for the number of waiting tasks, and removing the entry from the hashset once count == zero. That way, you only pay the SeqCst cost of calling notify() when you need to.

Not saying this is needed, I will let @mzabaluev set the path on this one. But I would prefer something that had a little hysteresis to it. Like removing the Notify if the pool size reached something like 10 buffers again. (But I have no idea why I would pick 10. Just something larger than 0.) But I haven't tried to design anything around this idea so feel free to disregard entirely.

My concern is that with the current design, once try_next() has failed once in the next() method. every checking from then on for that buffer size is going to issue a SeqCst memory barrier with the call to notify_one(). Thats quite a penalty. Its unavoidable with the surrent Notify, which is designed for Send operations, but we could at least limit it to when there is actually a consumer waiting.

@ollie-etl With your idea, I don't see how the usize field is decremented properly? In the case of a future being cancelled, there is no future to do the work of updating the count (from my understanding).

And along my idea of not keeping a count but removing once there are some number of buffers back in the pool, I don't see an elegant way of knowing whether the waiters linked list is empty or not. There is no public API.

You'd need to return a struct Next(usize, Notify) wrapper, implment Future,and decrement on drop

Oh, that's much more like @mzabaluev 's original idea I think, of returning a future that does work when it is polled, and does work when it is dropped. But using Notify.

mzabaluev · 2022-12-16T22:26:38Z

tests/fixed_buf.rs

+            let cloned_file = file.try_clone().unwrap();
+
+            let handle = tokio_uring::spawn(async move {
+                let file = File::from_std(cloned_file);


There should be a way to share File and socket objects between tasks, since the descriptor is already in a SharedFd internally.

FrankReh

I'm not going to check the unit tests tonight. But the use of Notify looks very nice. That is one powerful waiting mechanism Tokio has to offer. I ended up liking this implementation more than I thought I would because the Notify does all the heavy lifting of avoiding the pitfall of dropping an awake call. And as result, all the waiters don't have to be awoken to be on the safe side. The Notify implementation makes it safe to begin with, hence notify_one can be used very effectively.

FrankReh · 2022-12-19T23:35:26Z

src/buf/fixed/plumbing/pool.rs

+        })
+    }
+
+    pub(crate) fn notify_on_next(&mut self, cap: usize) -> Arc<Notify> {


But we are left without a simple/fast pool, that doesn't allocate, for the current-thread case. The case I care about. But all is not lost, once the API allows us to provide our own pool.

src/buf/fixed/plumbing/pool.rs

src/buf/fixed/pool.rs

src/buf/fixed/plumbing/pool.rs

src/buf/fixed/pool.rs

mzabaluev · 2022-12-22T22:13:38Z

@FrankReh

But we are left without a simple/fast pool, that doesn't allocate, for the current-thread case.

While the pool is not exhausted, tasks call next at near zero cost with no allocations.

FrankReh · 2022-12-23T00:29:47Z

@mzabaluev

While the pool is not exhausted, tasks call next at near zero cost with no allocations.

That's a very good point. I think I overlooked that while digging into the weeds of the implementation.

ollie-etl · 2023-01-19T14:13:31Z

@Noah-Kennedy @mzabaluev @FrankReh Is something blocking this? It looks like all points have been resolved

Noah-Kennedy · 2023-01-21T21:06:06Z

You don't need my approval here. I'm alright with @FrankReh signing off on this and merging it whenever he feels it is ready.

oliverbunting · 2023-01-25T19:21:20Z

@mzabaluev

While the pool is not exhausted, tasks call next at near zero cost with no allocations.

That's a very good point. I think I overlooked that while digging into the weeds of the implementation.

Is this not a common case though. I.e, to keep some pipeline of tasks fully utilized, do work whenever there is a buffer available?

mzabaluev · 2023-01-26T06:15:26Z

Is this not a common case though. I.e, to keep some pipeline of tasks fully utilized, do work whenever there is a buffer available?

Depends if the task creation is only throttled by availability of the buffers.

FrankReh · 2023-01-26T12:31:22Z

Is this not a common case though. I.e, to keep some pipeline of tasks fully utilized, do work whenever there is a buffer available?

Depends if the task creation is only throttled by availability of the buffers.

Having a limit to the number of buffers that the app would use seems like a good feature and I like the ability to await a next one but I hadn't thought it would be the primary throttler. I also like that a tokio scheduling feature was found to solve this problem because it's more likely to find acceptance by the tokio group.

I had thought I would try my hackier approach of using the copy of the tokio linked list once we have an API for substituting another pool but that was so long ago, I'm not sure it covered an important corner case I appreciated later. The current design even covers the case of a task being made ready to run but then being canceled before the resource is used and I think when select! blocks with timers are used, there will be many cases where a task is cancelled in its tracks.

FrankReh · 2023-02-15T15:07:39Z

I'm going to merge later tonight so it makes it into the next release. This is some really great work that we want in the repo but I had to do a manual merge on four files because code moved and code changed at the same time.

Actually the condensed diff, for the last commit, that this page makes available isn't too bad to read. Most of the changes are straight forward.

I guess the drop code, and the intra-doc links are where there could be problems so another pair of eyes or two or three wouldn't hurt. It compiles cleanly, and there is a nice unit test that passes. But rarely do our unit tests check that things were dropped properly so that's why looking at the drop again would make sense.

But also, nothing we can't change yet again with more iterations.

FrankReh · 2023-02-16T04:34:06Z

Thank you @mzabaluev .

mzabaluev added 3 commits December 5, 2022 12:45

buf/fixed: make collections inner structs reusable

e4a7f68

Move the Inner types of FixedBufRegistry and FixedBufPool into a plumbing module and expose them within buf::fixed, to allow code reuse with the upcoming thread-safe collections.

buf/fixed: FixedBufPool::poll_next

86c92f3

Enable waiting for fixed buffers to become available asynchronously.

buf/fixed: add FixedBufPool::next

e780e64

The async-friendly wrapper over FixedBufPool::poll_next. Also document the method and the functionality it enables for FixedBufPool. buf::fixed::pool is now a public module and hosts the Next future.

Noah-Kennedy approved these changes Dec 6, 2022

View reviewed changes

Noah-Kennedy marked this pull request as ready for review December 6, 2022 01:35

Noah-Kennedy marked this pull request as draft December 6, 2022 01:36

FrankReh reviewed Dec 6, 2022

View reviewed changes

src/buf/fixed/pool.rs Outdated Show resolved Hide resolved

FrankReh reviewed Dec 6, 2022

View reviewed changes

src/buf/fixed/plumbing/pool.rs Outdated Show resolved Hide resolved

src/buf/fixed/plumbing/pool.rs Outdated Show resolved Hide resolved

Merge branch 'master' into fixed-buf-pool-poll-next

6d0af6b

mzabaluev marked this pull request as ready for review December 6, 2022 08:41

FrankReh requested changes Dec 6, 2022

View reviewed changes

src/buf/fixed/plumbing/pool.rs Outdated Show resolved Hide resolved

mzabaluev added 3 commits December 12, 2022 02:21

Merge branch 'master' into fixed-buf-pool-poll-next

09cf69a

Don't carry a RefCell reference across await

2a42972

Thanks clippy.

mzabaluev commented Dec 15, 2022

View reviewed changes

mzabaluev added 2 commits December 16, 2022 07:44

Fixed reference to a Next future in comments

ea8f3bc

tests: test FixedBufPool::next

988046d

mzabaluev commented Dec 16, 2022

View reviewed changes

FrankReh approved these changes Dec 20, 2022

View reviewed changes

mzabaluev added 3 commits December 27, 2022 05:36

buf/fixed: renamed a local var for clarity

27e98f8

buf/fixed: comment on try_next returning None

d83eacb

buf/fixed: comment on Pool::notify_on_next

091cb4d

Merge branch 'master' into mzabaluev/fixed-buf-pool-poll-next

c414ba0

FrankReh merged commit 1205af9 into tokio-rs:master Feb 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`FixedBufPool::next` #199

`FixedBufPool::next` #199

mzabaluev commented Dec 6, 2022

Noah-Kennedy commented Dec 6, 2022

FrankReh left a comment

FrankReh left a comment

mzabaluev Dec 15, 2022 •

edited

Loading

FrankReh Dec 19, 2022

ollie-etl Jan 25, 2023

ollie-etl Jan 26, 2023

FrankReh Jan 26, 2023

ollie-etl Jan 27, 2023

FrankReh Jan 27, 2023 •

edited

Loading

ollie-etl Jan 30, 2023 •

edited

Loading

FrankReh Jan 30, 2023

mzabaluev Dec 16, 2022

FrankReh left a comment

FrankReh Dec 19, 2022

mzabaluev commented Dec 22, 2022

FrankReh commented Dec 23, 2022 •

edited

Loading

ollie-etl commented Jan 19, 2023 •

edited

Loading

Noah-Kennedy commented Jan 21, 2023

oliverbunting commented Jan 25, 2023

mzabaluev commented Jan 26, 2023

FrankReh commented Jan 26, 2023

FrankReh commented Feb 15, 2023 •

edited

Loading

FrankReh commented Feb 16, 2023

FixedBufPool::next #199

FixedBufPool::next #199

Conversation

mzabaluev commented Dec 6, 2022

Noah-Kennedy commented Dec 6, 2022

FrankReh left a comment

Choose a reason for hiding this comment

FrankReh left a comment

Choose a reason for hiding this comment

mzabaluev Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankReh Jan 27, 2023 • edited Loading

Choose a reason for hiding this comment

ollie-etl Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankReh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev commented Dec 22, 2022

FrankReh commented Dec 23, 2022 • edited Loading

ollie-etl commented Jan 19, 2023 • edited Loading

Noah-Kennedy commented Jan 21, 2023

oliverbunting commented Jan 25, 2023

mzabaluev commented Jan 26, 2023

FrankReh commented Jan 26, 2023

FrankReh commented Feb 15, 2023 • edited Loading

FrankReh commented Feb 16, 2023

`FixedBufPool::next` #199

`FixedBufPool::next` #199

mzabaluev Dec 15, 2022 •

edited

Loading

FrankReh Jan 27, 2023 •

edited

Loading

ollie-etl Jan 30, 2023 •

edited

Loading

FrankReh commented Dec 23, 2022 •

edited

Loading

ollie-etl commented Jan 19, 2023 •

edited

Loading

FrankReh commented Feb 15, 2023 •

edited

Loading