-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
&str.slice is bloated #16625
Comments
This is the cost of the |
It's also the cost of doing significantly more work than necessary:
It should be possible to at least reduce it to 2 unwinding (one for the character boundaries, one for |
The intricate details of the error handling aren't really important, because there's no reason for it to be inlined everywhere. It should be a function call passing a string literal. Using macros to report error locations doesn't work anyway because you don't get any context beyond the fact that a very low-level function like |
I think the problem is not the error location (that can be fixed if it's not already) but the fact that there's string formatting going on. Asserts should never have string formatting. |
Do |
|
These are somewhat stop-gap solutions to address #16625 core: Separate failure formatting in str methods slice, slice_to, slice_from Use a separate inline-never function to format failure message for str::slice() errors. Using strcat's idea, this makes sure no formatting code from failure is inlined when str::slice() is inlined. The number of `unreachable` being inlined when usingi `.slice()` drops from 5 to just 1. The testcase: ``` #![crate_type = "lib"] pub fn slice(x: &str, a: uint, b: uint) -> &str { x.slice(a, b) } ``` shrinks from 16.9 kB to 3.3 kB llvm IR, and the number of `unreachable` drops from 5 to 1.
With no custom message, we should just use concat! + stringify! for `assert!(expr)` to avoid the string formatting code path. Inspired by issue #16625
This issue has been resolved by resolving the example given, but what is the follow up, what is the more general issue? Would allowing a format-free assert! code path help (issue #16700)? Should we extend the pattern of using a "cold" function to perform elaborate error reporting? |
I would not like to propagate the This specific issue has been addressed, however, so I'm going to close this. More targeted follow up issues can be opened and addressed separately. |
with
-O
becomesThe worst part is there's five separate sites that unwind (each
unreachable
is just after an unwinding function) due to various assertions and bounds-checks. (&[].slice
is mildly better.)The text was updated successfully, but these errors were encountered: