vortex-file: Spawn tasks via runtime instead of directly via tokio - #8711
Conversation
Signed-off-by: Frederic Branczyk <fbranczyk@gmail.com>
e769588 to
4b260ab
Compare
gatesn
left a comment
There was a problem hiding this comment.
This is great, would love to know what checks you're using so we can run them ourselves!
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | rebuild_naive |
91.4 µs | 109 µs | -16.17% |
| ❌ | Simulation | chunked_varbinview_opt_canonical_into[(100, 100)] |
305.4 µs | 340.4 µs | -10.27% |
| ⚡ | Simulation | chunked_varbinview_into_canonical[(1000, 10)] |
205.6 µs | 169.5 µs | +21.32% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[128] |
244.4 ns | 215.3 ns | +13.55% |
| ⚡ | Simulation | bitwise_not_vortex_buffer_mut[1024] |
304.7 ns | 275.6 ns | +10.58% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing polarsignals:worktree-spawn-rm (4b260ab) with develop (a649d75)
Footnotes
-
42 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
…ortex-data#8711) ## Rationale for this change I upgraded to 0.78 and some automation we have that detects when dependencies use raw `tokio::spawn` got triggered. We need to control all spawn points, both for correctness testing and for instrumentation purposes. I realize that these spawns are in test code, but I thought since vortex already has the runtime abstractions, it wouldn't hurt to use them in the tests as well (admittedly our tooling is not crazily sophisticated so it can't distinguish whether it's testing code or not). ## What changes are included in this PR? Use `TokioRuntime` instead of raw `tokio::spawn` in tests. ## What APIs are changed? Are there any user-facing changes? n/a Signed-off-by: Frederic Branczyk <fbranczyk@gmail.com>
Rationale for this change
I upgraded to 0.78 and some automation we have that detects when dependencies use raw
tokio::spawngot triggered. We need to control all spawn points, both for correctness testing and for instrumentation purposes. I realize that these spawns are in test code, but I thought since vortex already has the runtime abstractions, it wouldn't hurt to use them in the tests as well (admittedly our tooling is not crazily sophisticated so it can't distinguish whether it's testing code or not).What changes are included in this PR?
Use
TokioRuntimeinstead of rawtokio::spawnin tests.What APIs are changed? Are there any user-facing changes?
n/a