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

Inefficient code generated from ToString::to_string() for bool #106611

Closed
Kmeakin opened this issue Jan 9, 2023 · 3 comments · Fixed by #106662
Closed

Inefficient code generated from ToString::to_string() for bool #106611

Kmeakin opened this issue Jan 9, 2023 · 3 comments · Fixed by #106662
Assignees
Labels
C-bug Category: This is a bug.

Comments

@Kmeakin
Copy link
Contributor

Kmeakin commented Jan 9, 2023

The ToString::to_string() implementation for bool generates over 700 lines of LLVM IR and 500 lines of AArch64 assembly!
Using format! with Display or Debug is also unnecessarily inefficient.
https://godbolt.org/z/3M4jGYE9d

// Worst
pub fn to_string(b: bool) -> String {
    b.to_string()
}

// Better
pub fn format_display(b: bool) -> String {
    format!("{b}")
}

pub fn format_debug(b: bool) -> String {
    format!("{b:?}")
}

// Best
pub fn if_then_else(b: bool) -> String {
    if b {
        "true".into()
    } else {
        "false".into()
    }
}

Version

rustc 1.68.0-nightly (ee0412d1e 2023-01-07)
binary: rustc
commit-hash: ee0412d1ef81efcfabe7f66cd21476ca85d618b1
commit-date: 2023-01-07
host: x86_64-unknown-linux-gnu
release: 1.68.0-nightly
LLVM version: 15.0.6
@Kmeakin Kmeakin added the C-bug Category: This is a bug. label Jan 9, 2023
@Ezrashaw
Copy link
Contributor

Ezrashaw commented Jan 9, 2023

Hmm, internally Display and Debug for bool are just an if/else statement, ToString is then implemented for any types implementing those. I wonder if this overhead:

#[inline]
default fn to_string(&self) -> String {
    let mut buf = String::new();
    let mut formatter = core::fmt::Formatter::new(&mut buf);
    // Bypass format_args!() to avoid write_str with zero-length strs
    fmt::Display::fmt(self, &mut formatter)
        .expect("a Display implementation returned an error unexpectedly");
    buf
}

causes that issue. Perhaps bool can be special-cased in ToString (like many other types)?

I'm happy to do a PR for this, just want confirmation this is sane.

@Noratrieb
Copy link
Member

Going through the formatting machinery causes lots of overhead since there's a lot of dynamic dispatch going on. We could specialize ToString on bool to return the string directly here.

@Ezrashaw
Copy link
Contributor

Ezrashaw commented Jan 9, 2023

Cool, I assume I can copy from bool's Display impl.

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants