Skip to content

wip remove future overhead when tracing is off. #15679

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ashdnazg
Copy link
Contributor

@ashdnazg ashdnazg commented Apr 10, 2025

Rationale for this change

The current tracing implementation adds an overhead of boxing all spawned futures and closures, even if tracing is off.

What changes are included in this PR?

The tracing API is changed to use a Spawner abstraction over JoinSet and the global tokio functions, allowing to spawn futures and closures unchanged if tracing is off.

Are these changes tested?

The tracing tests pass.

Are there any user-facing changes?

I don't think so.

@geoffreyclaude can you take a look? I'd like to know if this is desired before I make this PR nicer.

@github-actions github-actions bot added the common Related to common crate label Apr 10, 2025
@geoffreyclaude
Copy link
Contributor

@ashdnazg: I didn't go too much into detail yet, but the overall idea looks pretty sensible! Although it does seem to add complexity to some already non-obvious code.

Did you measure the overhead of the boxing though? I was expecting/hoping it to be negligible compared to the cost of the actual tasks.

I'm also not particularly attached to the current implementation, so if you find a way to simplify the interface as well as remove the wrapping when tracing is off, go for it! Since it hasn't yet been integrated in a release, the interface is completely open to change. Just as long as the tracing.rs (both the test and the example files) run with the expected results.

Otherwise I think it's a balance to find between code complexity and performance.

@ashdnazg
Copy link
Contributor Author

ashdnazg commented Apr 10, 2025

I admit I haven't benchmarked yet.
It's more out of principle, which admittedly isn't the best reason 😆

The way I see it, even if now that's not a significant overhead, now - as you also said - is the time to change stuff, as later it will be harder. If in the future (pun unintended) it'll be an issue, it won't be fun.

On that notion - do trace_future and trace_block need to be public?
If I understand correctly, it's enough to use them in SpawnedTask and JoinSet and the tests don't use them either.

@geoffreyclaude
Copy link
Contributor

geoffreyclaude commented Apr 11, 2025

On that notion - do trace_future and trace_block need to be public?
If I understand correctly, it's enough to use them in SpawnedTask and JoinSet and the tests don't use them either.

They're only public within the crate: the trace_utils mod is not exposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants