chore: reduce noise in benchmarks #151
Conversation
|
|
||
| [package.metadata.docs.rs] | ||
| all-features = true | ||
|
|
||
| [profile.bench] |
There was a problem hiding this comment.
This is for flamegraph generation.
There was a problem hiding this comment.
Is this obvious to most people looking at the Cargo.toml? If so, then fine. If not, maybe a line comment?
|
@ollie-etl I'm gating this on your approval. |
|
Where do you see the FuturesUnordered name in the flame graph? Just out of curiosity ... |
|
@FrankReh See here: Notice how much time is spent within various |
|
At the very least this cleans up the flamegraphs by moving everything into tokio codepaths. |
|
Here is a flamegraph with these changes. |
|
Granted, this would all be much cleaner if I didn't run it in a bench, but still... |
|
Ha. I guess you know what you're looking for. I still barely see core::future in those flames. Maybe I'm blind today. But great you are having a look. I can learn a lot by seeing what you end up finding and changing. I'm just happy I can reproduce the other problem. I'll work on it tomorrow. |
| let mut js = JoinSet::new(); | ||
|
|
||
| for _ in 0..opts.iterations { | ||
| js.spawn_local(tokio_uring::no_op()); | ||
| } | ||
|
|
||
| while let Some(res) = js.join_next().await { | ||
| res.unwrap().unwrap(); | ||
| } |
There was a problem hiding this comment.
This is fine - everything in this method is timed.
|
@Noah-Kennedy Please cherry-pick That commit cleans up the use of criterion now the runtime is exposed. It also exposes a bug introduced (I think) in #148. I'm not all that sure about the runtime stuff yet - could you take a look? The commit also limits the flame graph to the profiled region. |
|
Sure! @ollie-etl could you file an issue for the new bug? |
Will do. I'm out of office until next week |
|
|
@Noah-Kennedy Would you mind pulling cb676c8 into this PR? I think it improves the benchmarking code significantly |
|
@Noah-Kennedy can you get this in? I use the benchmarks a lot to check what I've done is sane, and tidying them up would be nice |
|
@oliverbunting I'll fix this up and get this in shortly then. |
|
@Noah-Kennedy Can you have a look at this again? I would like to get some benchmarks, especially now that 6.1 is out. |
Removes FuturesUnordered from benchmarks and alters timing to ignore task spawning, as to hone in on specifically the processing of ops and driver performance.
e44a2c9 to
8726a26
Compare
|
@FrankReh yeah I'll fix this up |
|
@FrankReh could you take a look at this, I think it's fine as is |
|
|
||
| [package.metadata.docs.rs] | ||
| all-features = true | ||
|
|
||
| [profile.bench] |
There was a problem hiding this comment.
Is this obvious to most people looking at the Cargo.toml? If so, then fine. If not, maybe a line comment?
|
@Noah-Kennedy Will let you squash and merge. |
A number of errors got overlooked in #151 and earlier PRs targeting the criterion benchmarks. In particular, the throughput, concurrency, and iteration count values were all wrong. This change addresses those issues.
Follow up to tokio-rs#151.
Removes FuturesUnordered from benchmarks and alters timing to ignore task spawning, as to hone in on specifically the processing of ops and driver performance.
Removes FuturesUnordered from benchmarks and alters timing to ignore task spawning, as to hone in on specifically the processing of ops and driver performance.