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

Avoid allocating when parsing \u{...} literals. #50052

Merged
merged 1 commit into from
Apr 20, 2018

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Apr 18, 2018

char_lit uses an allocation in order to ignore '_' chars in \u{...}
literals. This patch changes it to not do that by processing the chars
more directly.

This improves various rustc-perf benchmark measurements by up to 6%,
particularly regex, futures, clap, coercions, hyper, and encoding.

rustc-perf results, on a stage 2 build with jemalloc disabled:

regex-check
	avg: -5.4%	min: -6.5%	max: -2.7%
futures-check
	avg: -3.5%	min: -5.3%	max: -1.7%
regex-opt
	avg: -2.0%	min: -5.1%	max: -0.2%
regex
	avg: -2.3%	min: -5.0%	max: -0.6%
futures-opt
	avg: -3.0%	min: -4.8%	max: -1.1%
futures
	avg: -3.1%	min: -4.8%	max: -1.3%
clap-rs-check
	avg: -1.8%	min: -3.5%	max: -0.9%
coercions-check
	avg: -2.0%	min: -3.3%	max: -1.0%
hyper-check
	avg: -2.2%	min: -3.1%	max: -1.3%
hyper
	avg: -1.3%	min: -2.4%	max: -0.3%
hyper-opt
	avg: -0.9%	min: -2.3%	max: -0.1%
coercions
	avg: -1.1%	min: -2.2%	max: -0.4%
encoding-check
	avg: -1.7%	min: -2.2%	max: -0.9%
clap-rs-opt
	avg: -0.7%	min: -2.2%	max: 0.0%
coercions-opt
	avg: -1.2%	min: -2.1%	max: -0.3%
clap-rs
	avg: -0.8%	min: -1.9%	max: -0.4%
encoding-opt
	avg: -1.0%	min: -1.9%	max: -0.3%
encoding
	avg: -1.1%	min: -1.9%	max: -0.4%
piston-image-check
	avg: -0.7%	min: -1.3%	max: -0.3%
inflate-opt
	avg: -0.3%	min: -0.9%	max: -0.0%
piston-image
	avg: -0.3%	min: -0.8%	max: -0.1%
piston-image-opt
	avg: -0.3%	min: -0.7%	max: -0.1%
syn-check
	avg: -0.3%	min: -0.6%	max: -0.1%
deep-vector
	avg: 0.1%	min: -0.1%	max: 0.5%
syn-opt
	avg: -0.1%	min: -0.4%	max: 0.0%
html5ever
	avg: -0.2%	min: -0.4%	max: -0.0%
deep-vector-check
	avg: 0.0%	min: -0.3%	max: 0.3%
syn
	avg: -0.2%	min: -0.3%	max: -0.1%
html5ever-check
	avg: -0.3%	min: -0.3%	max: -0.2%
issue-46449-check
	avg: -0.1%	min: -0.2%	max: 0.2%
html5ever-opt
	avg: -0.0%	min: -0.2%	max: 0.1%
deep-vector-opt
	avg: -0.0%	min: -0.2%	max: 0.1%
issue-46449-opt
	avg: -0.0%	min: -0.2%	max: 0.1%
unify-linearly-check
	avg: -0.0%	min: -0.2%	max: 0.1%
helloworld-check
	avg: 0.0%	min: -0.0%	max: 0.2%
parser-check
	avg: -0.0%	min: -0.2%	max: 0.0%
inflate
	avg: 0.0%	min: -0.0%	max: 0.1%
tokio-webpush-simple-check
	avg: -0.1%	min: -0.1%	max: -0.0%
regression-31157-check
	avg: 0.0%	min: -0.1%	max: 0.1%
issue-46449
	avg: 0.0%	min: -0.1%	max: 0.1%
tuple-stress-opt
	avg: 0.0%	min: -0.0%	max: 0.1%
tuple-stress-check
	avg: -0.0%	min: -0.1%	max: 0.1%
tuple-stress
	avg: 0.0%	min: -0.0%	max: 0.1%
deeply-nested-check
	avg: 0.0%	min: -0.0%	max: 0.1%
regression-31157
	avg: -0.0%	min: -0.1%	max: 0.1%
deeply-nested-opt
	avg: -0.0%	min: -0.1%	max: 0.1%
parser-opt
	avg: -0.0%	min: -0.1%	max: 0.0%
parser
	avg: 0.1%	min: 0.0%	max: 0.1%
tokio-webpush-simple
	avg: -0.0%	min: -0.1%	max: 0.1%
regression-31157-opt
	avg: -0.0%	min: -0.1%	max: 0.1%
helloworld-opt
	avg: 0.0%	min: -0.0%	max: 0.1%
unify-linearly-opt
	avg: 0.0%	min: -0.0%	max: 0.1%
unused-warnings-check
	avg: 0.0%	min: 0.0%	max: 0.1%
tokio-webpush-simple-opt
	avg: -0.0%	min: -0.1%	max: 0.0%
helloworld
	avg: -0.0%	min: -0.0%	max: 0.1%
unused-warnings
	avg: 0.0%	min: -0.0%	max: 0.0%
deeply-nested
	avg: -0.0%	min: -0.0%	max: -0.0%
unused-warnings-opt
	avg: 0.0%	min: -0.0%	max: 0.0%
unify-linearly
	avg: 0.0%	min: -0.0%	max: 0.0%
inflate-check
	avg: 0.0%	min: -0.0%	max: 0.0%

@kennytm
Copy link
Member

kennytm commented Apr 18, 2018

Could we do the same for the integer_lit function too 🤔

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 18, 2018
@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Apr 18, 2018

⌛ Trying commit ca47fdc00059278c920df9743d84758dac533a32 with merge 1d4dcc78c0dfae72827cd04a442c6bd9e9c89ce2...

@bors
Copy link
Contributor

bors commented Apr 18, 2018

☀️ Test successful - status-travis
State: approved= try=True


// All digits and '_' are ascii, so treat each byte as a char.
let mut v: u32 = 0;
for &c in lit[3..idx].as_bytes().iter() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just lit[3..idx].bytes() {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

// All digits and '_' are ascii, so treat each byte as a char.
let mut v: u32 = 0;
for &c in lit[3..idx].as_bytes().iter() {
let c = c as char;
Copy link

@leonardo-m leonardo-m Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you replace that hard cast "as" with:

let c = char::from(c);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@nnethercote
Copy link
Contributor Author

Could we do the same for the integer_lit function too thinking

char_lit is much hotter, but I see in my profiles that integer_lit is moderately hot for a couple of benchmarks. So I'll do that too.

@nnethercote
Copy link
Contributor Author

Actually,integer_lit is a bit harder, so I'd prefer to do that in a follow-up.

`char_lit` uses an allocation in order to ignore '_' chars in \u{...}
literals. This patch changes it to not do that by processing the chars
more directly.

This improves various rustc-perf benchmark measurements by up to 6%,
particularly regex, futures, clap, coercions, hyper, and encoding.
@nnethercote
Copy link
Contributor Author

The new patch addresses @leonardo-m's comments.

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 18, 2018

📌 Commit 9f14502 has been approved by Mark-Simulacrum

@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 Apr 18, 2018
@bors
Copy link
Contributor

bors commented Apr 20, 2018

⌛ Testing commit 9f14502 with merge f6e532b8b41298b6adb04d6254e660cd7a5abb38...

@bors
Copy link
Contributor

bors commented Apr 20, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 20, 2018
@rust-highfive

This comment has been minimized.

@nnethercote
Copy link
Contributor Author

Could this be an infrastructure issue? I don't see anything else obviously wrong...

@kennytm
Copy link
Member

kennytm commented Apr 20, 2018

@bors retry

[00:01:44]    Compiling syn v0.13.1


No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.

@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 Apr 20, 2018
@bors
Copy link
Contributor

bors commented Apr 20, 2018

⌛ Testing commit 9f14502 with merge 85f5dd4...

bors added a commit that referenced this pull request Apr 20, 2018
Avoid allocating when parsing \u{...} literals.

`char_lit` uses an allocation in order to ignore '_' chars in \u{...}
literals. This patch changes it to not do that by processing the chars
more directly.

This improves various rustc-perf benchmark measurements by up to 6%,
particularly regex, futures, clap, coercions, hyper, and encoding.

rustc-perf results, on a stage 2 build with jemalloc disabled:

<details>

```
regex-check
	avg: -5.4%	min: -6.5%	max: -2.7%
futures-check
	avg: -3.5%	min: -5.3%	max: -1.7%
regex-opt
	avg: -2.0%	min: -5.1%	max: -0.2%
regex
	avg: -2.3%	min: -5.0%	max: -0.6%
futures-opt
	avg: -3.0%	min: -4.8%	max: -1.1%
futures
	avg: -3.1%	min: -4.8%	max: -1.3%
clap-rs-check
	avg: -1.8%	min: -3.5%	max: -0.9%
coercions-check
	avg: -2.0%	min: -3.3%	max: -1.0%
hyper-check
	avg: -2.2%	min: -3.1%	max: -1.3%
hyper
	avg: -1.3%	min: -2.4%	max: -0.3%
hyper-opt
	avg: -0.9%	min: -2.3%	max: -0.1%
coercions
	avg: -1.1%	min: -2.2%	max: -0.4%
encoding-check
	avg: -1.7%	min: -2.2%	max: -0.9%
clap-rs-opt
	avg: -0.7%	min: -2.2%	max: 0.0%
coercions-opt
	avg: -1.2%	min: -2.1%	max: -0.3%
clap-rs
	avg: -0.8%	min: -1.9%	max: -0.4%
encoding-opt
	avg: -1.0%	min: -1.9%	max: -0.3%
encoding
	avg: -1.1%	min: -1.9%	max: -0.4%
piston-image-check
	avg: -0.7%	min: -1.3%	max: -0.3%
inflate-opt
	avg: -0.3%	min: -0.9%	max: -0.0%
piston-image
	avg: -0.3%	min: -0.8%	max: -0.1%
piston-image-opt
	avg: -0.3%	min: -0.7%	max: -0.1%
syn-check
	avg: -0.3%	min: -0.6%	max: -0.1%
deep-vector
	avg: 0.1%	min: -0.1%	max: 0.5%
syn-opt
	avg: -0.1%	min: -0.4%	max: 0.0%
html5ever
	avg: -0.2%	min: -0.4%	max: -0.0%
deep-vector-check
	avg: 0.0%	min: -0.3%	max: 0.3%
syn
	avg: -0.2%	min: -0.3%	max: -0.1%
html5ever-check
	avg: -0.3%	min: -0.3%	max: -0.2%
issue-46449-check
	avg: -0.1%	min: -0.2%	max: 0.2%
html5ever-opt
	avg: -0.0%	min: -0.2%	max: 0.1%
deep-vector-opt
	avg: -0.0%	min: -0.2%	max: 0.1%
issue-46449-opt
	avg: -0.0%	min: -0.2%	max: 0.1%
unify-linearly-check
	avg: -0.0%	min: -0.2%	max: 0.1%
helloworld-check
	avg: 0.0%	min: -0.0%	max: 0.2%
parser-check
	avg: -0.0%	min: -0.2%	max: 0.0%
inflate
	avg: 0.0%	min: -0.0%	max: 0.1%
tokio-webpush-simple-check
	avg: -0.1%	min: -0.1%	max: -0.0%
regression-31157-check
	avg: 0.0%	min: -0.1%	max: 0.1%
issue-46449
	avg: 0.0%	min: -0.1%	max: 0.1%
tuple-stress-opt
	avg: 0.0%	min: -0.0%	max: 0.1%
tuple-stress-check
	avg: -0.0%	min: -0.1%	max: 0.1%
tuple-stress
	avg: 0.0%	min: -0.0%	max: 0.1%
deeply-nested-check
	avg: 0.0%	min: -0.0%	max: 0.1%
regression-31157
	avg: -0.0%	min: -0.1%	max: 0.1%
deeply-nested-opt
	avg: -0.0%	min: -0.1%	max: 0.1%
parser-opt
	avg: -0.0%	min: -0.1%	max: 0.0%
parser
	avg: 0.1%	min: 0.0%	max: 0.1%
tokio-webpush-simple
	avg: -0.0%	min: -0.1%	max: 0.1%
regression-31157-opt
	avg: -0.0%	min: -0.1%	max: 0.1%
helloworld-opt
	avg: 0.0%	min: -0.0%	max: 0.1%
unify-linearly-opt
	avg: 0.0%	min: -0.0%	max: 0.1%
unused-warnings-check
	avg: 0.0%	min: 0.0%	max: 0.1%
tokio-webpush-simple-opt
	avg: -0.0%	min: -0.1%	max: 0.0%
helloworld
	avg: -0.0%	min: -0.0%	max: 0.1%
unused-warnings
	avg: 0.0%	min: -0.0%	max: 0.0%
deeply-nested
	avg: -0.0%	min: -0.0%	max: -0.0%
unused-warnings-opt
	avg: 0.0%	min: -0.0%	max: 0.0%
unify-linearly
	avg: 0.0%	min: -0.0%	max: 0.0%
inflate-check
	avg: 0.0%	min: -0.0%	max: 0.0%
```

</details>
@bors
Copy link
Contributor

bors commented Apr 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 85f5dd4 to master...

@bors bors merged commit 9f14502 into rust-lang:master Apr 20, 2018
@nnethercote nnethercote deleted the char_lit branch April 23, 2018 05:10
@nnethercote
Copy link
Contributor Author

Actually,integer_lit is a bit harder, so I'd prefer to do that in a follow-up.

I finally did integer_lit (and float_lit) in #55384.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants