-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Avoid unnecessary allocations in float_lit
and integer_lit
.
#55384
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -494,8 +494,17 @@ fn float_lit(s: &str, suffix: Option<Symbol>, diag: Option<(Span, &Handler)>) | |
-> Option<ast::LitKind> { | ||
debug!("float_lit: {:?}, {:?}", s, suffix); | ||
// FIXME #2252: bounds checking float literals is deferred until trans | ||
let s = s.chars().filter(|&c| c != '_').collect::<String>(); | ||
filtered_float_lit(Symbol::intern(&s), suffix, diag) | ||
|
||
// Strip underscores without allocating a new String unless necessary. | ||
let s2; | ||
let s = if s.chars().any(|c| c == '_') { | ||
s2 = s.chars().filter(|&c| c != '_').collect::<String>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, since underscores are in ASCII, couldn't we into_bytes, retain, and then String::from_utf8, maybe unchecked? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would work, but it's not clear to me that it would be any faster; it might even be slower because there are two allocations involved? (Because it converts to a vector, and then back to a new String?) Besides, this is the cold path, so I'm satisfied with reusing the existing code, which until now had been considered good enough for the hot path :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just FYI, String::from_utf8 consumes the vector given to it, so no additional allocation would happen. |
||
&s2 | ||
} else { | ||
s | ||
}; | ||
|
||
filtered_float_lit(Symbol::intern(s), suffix, diag) | ||
} | ||
|
||
/// Parse a string representing a byte literal into its final form. Similar to `char_lit` | ||
|
@@ -591,8 +600,14 @@ fn integer_lit(s: &str, suffix: Option<Symbol>, diag: Option<(Span, &Handler)>) | |
-> Option<ast::LitKind> { | ||
// s can only be ascii, byte indexing is fine | ||
|
||
let s2 = s.chars().filter(|&c| c != '_').collect::<String>(); | ||
let mut s = &s2[..]; | ||
// Strip underscores without allocating a new String unless necessary. | ||
let s2; | ||
let mut s = if s.chars().any(|c| c == '_') { | ||
s2 = s.chars().filter(|&c| c != '_').collect::<String>(); | ||
&s2 | ||
} else { | ||
s | ||
}; | ||
|
||
debug!("integer_lit: {}, {:?}", s, suffix); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be interesting to see if
std::slice::memchr::memchr(b'_', s.as_bytes()).is_some()
is faster thans.chars().any()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it. Cachegrind says it's marginally more instructions: 22,526,559,505 up from 22,516,683,388. So I think I'll stick with the original.