Skip to content

Reduce code size overhead from core::panicking::panic #22948

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

Merged
merged 1 commit into from
Mar 14, 2015

Conversation

rprichard
Copy link
Contributor

Reduce code size overhead from core::panicking::panic

core::panicking::panic currently creates an Arguments structure using
format_args!("{}", expr), which formats the expr str using the Display::fmt.
Display::fmt pulls in Formatter::pad, which then also pulls in string-related
code for truncation and padding.

If core::panicking::panic instead creates an Arguments structure with a string
piece, it is possible that the Display::fmt function for str can be optimized
out of the program.

In my testing with a 32-bit x86 bare metal program, the change tended to save
between ~100 bytes and ~5500 bytes, depending on what other panic* functions
the program invokes and whether the panic_fmt lang item uses the Arguments
value.

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@brson
Copy link
Contributor

brson commented Mar 9, 2015

LGTM, but I think this test case doesn't capture anything new, so please remove it. @alexcrichton are you ok adding an unstable method to the ArgumentsV1 trait here?

@alexcrichton
Copy link
Member

Couldn't this call new_v1? They look like they take the same arguments, you could just use an empty args array.

Either way, can you add documentation to panic explaining why format_args! is not being used?

@rprichard
Copy link
Contributor Author

new_v1_single_string enforces that pieces is either the same size as args or one item greater. It isn't necessary, though -- I can just call Arguments::new_v1 with an empty args.

I had added this short comment inside panic. Should I elaborate more?

// To reduce formatting code size overhead, create an Arguments structure
// that does not reference a formatting function.

I'll remove the test.

@alexcrichton
Copy link
Member

Yeah that's fine, thanks!

@rprichard
Copy link
Contributor Author

Let me know if I should squash and/or rebase these changes.

@alexcrichton
Copy link
Member

Ah yes, a squash would be nice, other than that r=me, this looks great, thanks!

Display::fmt for str calls into Formatter::pad, which is modest in size
and also pulls in string-related functions for its truncation and padding
abilities.  For size-critical programs (e.g. embedded), this call site
may be the only reason Formatter::pad is linked into the output.
@rprichard
Copy link
Contributor Author

@alexcrichton I squashed the commits, so this PR should be ready now.

@alexcrichton
Copy link
Member

@bors: r+ 9c0057d

bors added a commit that referenced this pull request Mar 14, 2015
Reduce code size overhead from core::panicking::panic

core::panicking::panic currently creates an Arguments structure using
format_args!("{}", expr), which formats the expr str using the Display::fmt.
Display::fmt pulls in Formatter::pad, which then also pulls in string-related
code for truncation and padding.

If core::panicking::panic instead creates an Arguments structure with a string
piece, it is possible that the Display::fmt function for str can be optimized
out of the program.

In my testing with a 32-bit x86 bare metal program, the change tended to save
between ~100 bytes and ~5500 bytes, depending on what other panic* functions
the program invokes and whether the panic_fmt lang item uses the Arguments
value.
@bors
Copy link
Collaborator

bors commented Mar 14, 2015

⌛ Testing commit 9c0057d with merge f7453f9...

@bors bors merged commit 9c0057d into rust-lang:master Mar 14, 2015
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.

5 participants