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

[rustc_data_structures][base_n][perf] Remove unnecessary utf8 check. #114339

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

ttsugriy
Copy link
Contributor

@ttsugriy ttsugriy commented Aug 1, 2023

Since all output characters taken from BASE_64 are valid UTF8 chars there is no need to waste cycles on validation.

Even though it's obviously a perf win, I've also used a benchmark on M1 MacBook Air with following results:

Running benches/base_n_benchmark.rs (target/release/deps/base_n_benchmark-825fe5895b5c2693)
push_str/old            time:   [14.670 µs 14.852 µs 15.074 µs]
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe
push_str/new            time:   [12.573 µs 12.674 µs 12.801 µs]
Found 11 outliers among 100 measurements (11.00%)
  7 (7.00%) high mild
  4 (4.00%) high severe

Since all output characters taken from `BASE_64` are valid UTF8 chars
there is no need to waste cycles on validation.

Even though it's obviously a perf win, I've also used a [benchmark](https://gist.github.com/ttsugriy/e1e63c07927d8f31e71695a9c617bbf3)
on M1 MacBook Air with following results:
```
Running benches/base_n_benchmark.rs (target/release/deps/base_n_benchmark-825fe5895b5c2693)
push_str/old            time:   [14.670 µs 14.852 µs 15.074 µs]
                        Performance has regressed.
Found 11 outliers among 100 measurements (11.00%)
  4 (4.00%) high mild
  7 (7.00%) high severe
push_str/new            time:   [12.573 µs 12.674 µs 12.801 µs]
                        Performance has regressed.
Found 11 outliers among 100 measurements (11.00%)
  7 (7.00%) high mild
  4 (4.00%) high severe
```
@rustbot
Copy link
Collaborator

rustbot commented Aug 1, 2023

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 1, 2023
@Noratrieb
Copy link
Member

let's see whether this has a perf impact in the big picture
@bors try @rust-timer-queue

@bors
Copy link
Contributor

bors commented Aug 1, 2023

⌛ Trying commit 64dfd10 with merge 54dae55966c4fd1e2fffedc9a1c6a16d6df6f073...

@klensy
Copy link
Contributor

klensy commented Aug 1, 2023

Some years ago i tried something similar #86214

@bors
Copy link
Contributor

bors commented Aug 1, 2023

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

@Noratrieb
Copy link
Member

it seems likely to me that the perf impact will be negligible (actually this code might be off by default and only used in v0 mangling) but even then, I'd expect it to not matter that much. That said, the safety invariant here is trivial to uphold as it always writes ASCII bytes so it's not that problematic.

@Noratrieb
Copy link
Member

oops, wrong command
@rust-timer build 54dae55966c4fd1e2fffedc9a1c6a16d6df6f073

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (54dae55966c4fd1e2fffedc9a1c6a16d6df6f073): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-0.8%, -0.3%] 2
Improvements ✅
(secondary)
-0.3% [-0.4%, -0.3%] 3
All ❌✅ (primary) -0.5% [-0.8%, -0.3%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.7% [3.7%, 3.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.7% [-1.7%, -1.7%] 1
Improvements ✅
(secondary)
-2.0% [-2.0%, -2.0%] 1
All ❌✅ (primary) -1.7% [-1.7%, -1.7%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: missing data

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 8, 2023

📌 Commit 64dfd10 has been approved by davidtwco

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2023
@bors
Copy link
Contributor

bors commented Aug 8, 2023

⌛ Testing commit 64dfd10 with merge 617821a...

@bors
Copy link
Contributor

bors commented Aug 8, 2023

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 617821a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 8, 2023
@bors bors merged commit 617821a into rust-lang:master Aug 8, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 8, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (617821a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.4% [-3.5%, -0.9%] 6
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.5% [-2.5%, -2.5%] 3
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 634.018s -> 633.541s (-0.08%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants