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

Using &v instead of v arg in format! does not inline, causing ~6% perf hit #112156

Open
nyurik opened this issue Jun 1, 2023 · 4 comments
Open
Labels
A-codegen Area: Code generation A-fmt Area: `std::fmt` C-bug Category: This is a bug. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@nyurik
Copy link
Contributor

nyurik commented Jun 1, 2023

I tried this code:

pub fn func(i: i32) -> String {
    format!("{}", &i)
}

I expected &i to be inlined and be equivalent to format!("{}", i)

Instead, in my benchmarking tests I see about 6% performance hit, and assembly has a non-inlined function.

image

Relevant links

Meta

rustc --version --verbose:

rustc 1.69.0 (84c898d65 2023-04-16)
binary: rustc
commit-hash: 84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc
commit-date: 2023-04-16
host: x86_64-unknown-linux-gnu
release: 1.69.0
LLVM version: 15.0.7
@nyurik nyurik added the C-bug Category: This is a bug. label Jun 1, 2023
@Jules-Bertholet
Copy link
Contributor

@rustbot label A-codegen I-slow

@rustbot rustbot added A-codegen Area: Code generation I-slow Issue: Problems and improvements with respect to performance of generated code. labels Jun 1, 2023
@nyurik
Copy link
Contributor Author

nyurik commented Jun 1, 2023

cc: @m-ou-se because it might be related to all the great format-AST related work

@m-ou-se
Copy link
Member

m-ou-se commented Jun 1, 2023

Right now, every argument to format_args!() turns into a Argument, which is basically a &dyn Display but a bit different:

/// This struct represents the generic "argument" which is taken by format_args!().
/// It contains a function to format the given value. At compile time it is ensured that the
/// function and the value have the correct types, and then this struct is used to canonicalize
/// arguments to one type.
///
/// Argument is essentially an optimized partially applied formatting function,
/// equivalent to `exists T.(&T, fn(&T, &mut Formatter<'_>) -> Result`.
#[lang = "format_argument"]
#[derive(Copy, Clone)]
pub struct Argument<'a> {
value: &'a Opaque,
formatter: fn(&Opaque, &mut Formatter<'_>) -> Result,
}

I don't expect llvm to be able to optimize that in any meaningful way.

So I don't think this is something we can immediately solve. Longer term, a new implementation could do this very differently and perhaps tackle this issue.

related to all the great format-AST related work

I don't think the AST is enough to tell if a & can be dropped, because for !Sized things, the & must stay.

Perhaps we can special case &identifier only, if we're sure that in that case there are no subtle edge cases where the & must stay. (But in that case it should maybe just be a warning (from clippy or rustc).)

@jyn514 jyn514 added T-libs Relevant to the library team, which will review and decide on the PR/issue. A-fmt Area: `std::fmt` labels Jun 1, 2023
@nyurik
Copy link
Contributor Author

nyurik commented Jun 1, 2023

Thanks @m-ou-se for the explanation. I was hoping that while argument is represented as a &dyn Display-like structure, code generation and optimization would inline the fmt() call, removing the extra complexity.

nyurik added a commit to nyurik/rust that referenced this issue Jul 24, 2023
Per rust-lang#112156, using `&` in `format!` may cause a small perf delay, so I tried to clean up one module at a time format usage. This PR includes a few removals of the ref in format (they do compile locally without the ref), as well as a few format inlining for consistency.
nyurik added a commit to nyurik/rust that referenced this issue Jul 24, 2023
Per rust-lang#112156, using `&` in `format!` may cause a small perf delay, so I tried to clean up one module at a time format usage. This PR includes a few removals of the ref in format (they do compile locally without the ref), as well as a few format inlining for consistency.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 24, 2023
…Lapkin

Optimize format usage

Per rust-lang#112156, using `&` in `format!` may cause a small perf delay, so I tried to clean up one module at a time format usage. This PR includes a few removals of the ref in format (they do compile locally without the ref), as well as a few format inlining for consistency.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jul 24, 2023
Optimize format usage

Per rust-lang#112156, using `&` in `format!` may cause a small perf delay, so I tried to clean up one module at a time format usage. This PR includes a few removals of the ref in format (they do compile locally without the ref), as well as a few format inlining for consistency.
@workingjubilee workingjubilee added the C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such label Oct 8, 2023
bors added a commit to rust-lang/rust-clippy that referenced this issue Jul 26, 2024
Avoid ref when using format!

Clean up a few minor refs in `format!` macro, as it has a performance cost. Apparently the compiler is unable to inline `format!("{}", &variable)`, and does a run-time double-reference instead (format macro already does one level referencing).

Inlining format args prevents accidental `&` misuse.

See also rust-lang/rust#112156

changelog: none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation A-fmt Area: `std::fmt` C-bug Category: This is a bug. C-optimization Category: An issue highlighting optimization opportunities or PRs implementing such I-slow Issue: Problems and improvements with respect to performance of generated code. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants