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

base_n: speedup push_str #86214

Closed
wants to merge 1 commit into from
Closed

base_n: speedup push_str #86214

wants to merge 1 commit into from

Conversation

klensy
Copy link
Contributor

@klensy klensy commented Jun 11, 2021

This speedup base_n::push_str by removing few range bounds and utf8 check.

Unsafe is ugly here, but better version is welcome.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2021
@rust-log-analyzer

This comment has been minimized.

@klensy klensy marked this pull request as draft June 11, 2021 11:59
@davidtwco
Copy link
Member

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 16, 2021
@bors
Copy link
Contributor

bors commented Jun 16, 2021

⌛ Trying commit 168111d8134207b4c72284f0ca4954eac1a6040d with merge dce87a54a1682189e15b6dfb02c488edfd7223ff...

@bors
Copy link
Contributor

bors commented Jun 16, 2021

☀️ Try build successful - checks-actions
Build commit: dce87a54a1682189e15b6dfb02c488edfd7223ff (dce87a54a1682189e15b6dfb02c488edfd7223ff)

@rust-timer
Copy link
Collaborator

Queued dce87a54a1682189e15b6dfb02c488edfd7223ff with parent 2336406, future comparison URL.

@rust-timer
Copy link
Collaborator

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 rollup- to bors.

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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 16, 2021
@klensy klensy marked this pull request as ready for review June 16, 2021 14:40
@klensy
Copy link
Contributor Author

klensy commented Jun 16, 2021

No perf change, but at least manual bench run faster.

@davidtwco
Copy link
Member

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.

No perf change, but at least manual bench run faster.

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 davidtwco closed this Jun 16, 2021
@klensy
Copy link
Contributor Author

klensy commented Jun 19, 2021

@davidtwco Faster in synthetic tests, plus it maybe help with #85530, as it used in mangling v0.

@davidtwco davidtwco reopened this Jun 21, 2021
@davidtwco
Copy link
Member

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.

@davidtwco davidtwco added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2021
@JohnCSimon
Copy link
Member

Ping from triage:
@klensy - can you please address the comment from davidtwco?

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) };
Copy link
Member

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.

@crlf0710
Copy link
Member

@klensy Ping from triage! Any updates on this?

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2021
@JohnCSimon JohnCSimon added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Aug 15, 2021
@JohnCSimon
Copy link
Member

JohnCSimon commented Aug 15, 2021

Triage:
@klensy
I'm closing this as inactive as it hasn't seen any movement since June.
Please feel free to open if you're going to continue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants