Skip to content
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

Move tracing::warn call in spawn_try_task to separate function #3581

Closed
wants to merge 2 commits into from

Conversation

dullbananas
Copy link
Collaborator

@dullbananas dullbananas commented Jul 11, 2023

I ran cargo-llvm-lines, and over 1% of the LLVM code comes from calls to spawn_try_task.

Generic functions cause separate LLVM code generation for each unique combination of generic arguments (each implementation of Future for the task parameter in this case). Moving part of the function to a separate non-generic function will reduce the amount of code generation, which can help with compilation speed. https://nnethercote.github.io/perf-book/compile-times.html#llvm-ir

The complexity of the output of the tracing::warn macro can be seen here by selecting "expand macros" under "tools": playground link

Here's the output of cargo-llvm-lines on this commit using dev profile and CARGO_PROFILE_DEV_LTO=fat:
lineslib-lto.zip

I will make similar improvements in Tokio

Copy link
Collaborator

@phiresky phiresky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting.Change seems ok.

But spawn_try_task is only called with three different arguments, so it should only be monomorphized three times. It seems like that should not make a significant difference? (with the current use of the function)

@Nutomic
Copy link
Member

Nutomic commented Jul 11, 2023

Can you check if this actually makes a noticable difference in build time? cargo build --timings is ideal for this. Make sure to set RUSTC_WRAPPER='' if you use sccache.

@dullbananas
Copy link
Collaborator Author

But spawn_try_task is only called with three different arguments, so it should only be monomorphized three times. It seems like that should not make a significant difference? (with the current use of the function)

cargo-llvm-lines says it's 80 times. This is because it's used by perform and perform_crud in api_routes_http, which are called with different types for Data.

@dullbananas
Copy link
Collaborator Author

Can you check if this actually makes a noticable difference in build time? cargo build --timings is ideal for this. Make sure to set RUSTC_WRAPPER='' if you use sccache.

The amount of LLVM lines reduced by 1.2% (2151193 to 2124873).

I don't want to directly measure the change in build time. It takes a while to build even without cargo clear, and I don't expect to get precise enough results without rebuilding so many times that it would take many hours. And for accurate results, the CPU can't be running anything else during that time.

lineslib-lto-with-change.zip

@Nutomic
Copy link
Member

Nutomic commented Jul 12, 2023

cargo-llvm-lines says it's 80 times. This is because it's used by perform and perform_crud in api_routes_http, which are called with different types for Data.

Im currently getting rid of those traits, see #3596

@Nutomic
Copy link
Member

Nutomic commented Jul 14, 2023

Measured the compile time of this a few times (cherry-picked on top of main), and Im not seeing any improvement.

@Nutomic Nutomic closed this Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants