-
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
base_n: speedup push_str #86214
base_n: speedup push_str #86214
Conversation
r? @davidtwco (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 168111d8134207b4c72284f0ca4954eac1a6040d with merge dce87a54a1682189e15b6dfb02c488edfd7223ff... |
☀️ Try build successful - checks-actions |
Queued dce87a54a1682189e15b6dfb02c488edfd7223ff with parent 2336406, future comparison URL. |
Finished benchmarking try commit (dce87a54a1682189e15b6dfb02c488edfd7223ff): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
No perf change, but at least manual bench run faster. |
I don't think this is worth landing if it doesn't have a measurable effect on perf. If there's a specific case that this improves, or you have further ideas that you'd like to try, then we can re-open this.
Do you mean manual benchmarking of this function in isolation, or a test case that rustc is noticably faster at compiling with these changes applied? |
@davidtwco Faster in synthetic tests, plus it maybe help with #85530, as it used in mangling v0. |
Could you add a commit to this MR that enables the new symbol mangling? We'll drop it after, but we can use it to run perf with the new mangling on and see how this impacts the performance of that. |
Ping from triage: |
index += 1; | ||
for idx in (0..128).rev() { | ||
// SAFETY: given `base <= MAX_BASE`, so `n % base < MAX_BASE` | ||
s[idx] = unsafe { *BASE_64.get_unchecked((n % base) as usize) }; |
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.
This probably doesn't need to be unsafe. LLVM will eliminate the bounds check because the assertion above already allows it to proof the condition given in the comment.
@klensy Ping from triage! Any updates on this? |
Triage: |
This speedup base_n::push_str by removing few range bounds and utf8 check.
Unsafe is ugly here, but better version is welcome.