Hi, HttpWorkType looks good.
But please leave load generation functions original.
Your changes are bad for performance, and it's not working on my machine.

cargo run -- -z 10s http://127.0.0.1:3000
# stop after 50 requests ... 

Hi, I fixed the performance bug for HTTP1 with 'deadline' enabled. Good catch sorry. As far as performance impact, these changes should have minimal effect. I've done a few runs comparing 'before' and after' and found no difference at all.
The only part I've split out into functions is the setup the initial loop for each of the load generation patterns. These only get called once per load generation run, so the impact is essentially nothing.
Do you want me to perform some more benchmarking and come back with data?

Would it be better if I split this into smaller changes?

Would it be better if I split this into smaller changes?

Sorry for delay.
Yeah it's very helpful if you split to smoller PRs.

The only part I've split out into functions is the setup the initial loop for each of the load generation patterns. These only get called once per load generation run, so the impact is essentially nothing.

It's not true for fn work(). It is now using channel instead of AtomicUsize and it introduces some overheads.

In my environment, the difference are noticeable.

❯ cargo run --profile release-ci -- -n 10000000 -c 1000 http://localhost:3000
    Finished `release-ci` profile [optimized] target(s) in 0.15s
     Running `target/release-ci/oha -n 10000000 -c 1000 'http://localhost:3000'`
Summary:
  Success rate: 100.00%
  Total:        16.9223 secs
  Slowest:      0.1381 secs
  Fastest:      0.0000 secs
  Average:      0.0017 secs
  Requests/sec: 590935.7094

❯ oha -n 10000000 -c 1000 http://localhost:3000 # v1.8.0
Summary:
  Success rate: 100.00%
  Total:        16.3692 secs
  Slowest:      0.1259 secs
  Fastest:      0.0000 secs
  Average:      0.0016 secs
  Requests/sec: 610903.7964

Off course, the performance gap is small, but I care it.

Could you make this PR only contain HttpWorkType stuffs?
We can discuss about refactoring on other PRs. I think general idea of refactoring is good. Thank you.

Hi, so I removed the refactoring for the work_until function that used the endless_emitter, because that had more of a slowdown than the other paths What are your thoughts on the latest changes?
I think having a 1% performance difference for the 'slow' path is not so bad, for the amount of code that is reduced. The 'fast path' should still be just as fast.
Also what server are you using to test oha against? I want to use the same one so I can make sure we're measuring the same thing

Could you just contain HttpWorkType for this PR?
I really care about the performance for all path.

For this code,

async fn parallel_work_http1(
    n_connections: usize,
    rx: AsyncReceiver<Option<Instant>>,
    report_tx: kanal::Sender<Result<RequestResult, ClientError>>,
    client: Arc<Client>,
    deadline: Option<std::time::Instant>,
) -> Vec<tokio::task::JoinHandle<()>> {

The rx: AsyncReceiver<Option<Instant>> adds some overheads for non latency correction workers.
Because they always send None but the size of it isn't zero.

I test against https://github.com/hatoo/sandbag

Ok I can do that. One thought to discuss though - is it a good thing for oha to have different performance depending on the way a user invokes it? For example, all of the qps paths will perform say 1% worse because they use the channels. So the user might see a different performance of their application depending on whether they use -q or not, or even perhaps between the -n and -z flags. So is it better to have the same function used for all of these paths so that the user gets consistent performance? Also then if you performance optimise that one function, then all of the load tests automatically get faster.

is it a good thing for oha to have different performance depending on the way a user invokes it? For example, all of the qps paths will perform say 1% worse because they use the channels. So the user might see a different performance of their application depending on whether they use -q or not, or even perhaps between the -n and -z flags. So is it better to have the same function used for all of these paths so that the user gets consistent performance?

Yeah, performance variation depending on command line flags is not good.
But keeping codes optimal is more important to me.

Also then if you performance optimise that one function, then all of the load tests automatically get faster.

I'm not saying I'll refuse all refactoring. I think we can refactoring work* to use common function while keeping the current performance by using generics.

Done at #746