search

yoshuawuyts / futures-concurrency

Structured concurrency operations for async Rust
https://docs.rs/futures-concurrency
Apache License 2.0
413 stars 33 forks source link

Better capacity and growth rate for `{Stream,Future}Group` #172

Closed matheus-consoli closed 7 months ago

matheus-consoli commented 8 months ago

Fixes the same problem as #168 by keeping up with the capacity and growth rate of the group.

When a group tries to grow over its capacity, we double the previous capacity.

FutureGroup and StreamGroup are structurally very similar, they only differ on the Stream implementation, so I isolated the common structure to reduce the duplicated code.

Performance

Compared to the main branch, we gained 99,775% using this benchmark (with 1_000_000 futures).

Before:

group/futures_concurrency::FutureGroup
                        time:   [27.222 s 28.561 s 29.720 s]

After:

group/futures_concurrency::FutureGroup
                        time:   [68.507 ms 68.598 ms 68.752 ms]
                        change: [-99.769% -99.760% -99.748%] (p = 0.00 < 0.05)
                        Performance has improved.

todos

Closes #169

yoshuawuyts commented 7 months ago

@matheus-consoli apologies for taking a little while to respond; I was out all of last week. I love that you've implemented a more robust version of #168, and I'm 100% on board with solving #169 the way you've done it. However, I'm unsure whether using the InnerGroup implementation you've provided here is the way to do it. Let me try and explain.

In the current implementation we have two structs:

In the implementation provided by this PR we have a hierarchy of structs, at the top level there is:

Which internally uses a shared impl:

PollBehavior here abstracts over the differences between Stream and Future, and is in turn implemented on two additional structs:

My worry is that this creates a funnel-shaped abstraction: we're specializing behavior at the top of the abstraction, going through a single shared type, and then specializing again at the bottom of the abstraction. I worry that this may work because poll_next is almost identical to poll, but if we wanted to add something more left-field such as AsyncIteratorGroup this layering would break down.

I think for that reason we might be better served with just duplicating the strategy between both impls. And perhaps we can explore ways to share more logic between impls without specializing at the lower levels in future PRs? Would that be ok with you?

matheus-consoli commented 7 months ago

sure, i agree with you

my initial idea was to write a single type InnerGroup and have {Future, Stream}Group be type aliases (that would, more or less, break the funnel-shaped problem), but i also didn't want to expose those in the public API... and i wasn't considering an AsyncIteratorGroup.

I will close this PR and open a simpler one.