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 unnecessary allocations in `float_lit` and `integer_lit`. #55384

Merged
merged 1 commit into from Oct 29, 2018

Conversation

@nnethercote
Copy link
Contributor

commented Oct 26, 2018

This commit avoids an allocation when parsing any float and integer
literals that don't involved underscores.

This reduces the number of allocations done for the tuple-stress
benchmark by 10%, reducing its instruction count by just under 1%.

Avoid unnecessary allocations in `float_lit` and `integer_lit`.
This commit avoids an allocation when parsing any float and integer
literals that don't involved underscores.

This reduces the number of allocations done for the `tuple-stress`
benchmark by 10%, reducing its instruction count by just under 1%.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Oct 26, 2018

r? @michaelwoerister

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

// 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>();

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Oct 26, 2018

Member

Hm, since underscores are in ASCII, couldn't we into_bytes, retain, and then String::from_utf8, maybe unchecked?

This comment has been minimized.

Copy link
@nnethercote

nnethercote Oct 28, 2018

Author Contributor

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 :)

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Oct 29, 2018

Contributor

Just FYI, String::from_utf8 consumes the vector given to it, so no additional allocation would happen.


// Strip underscores without allocating a new String unless necessary.
let s2;
let s = if s.chars().any(|c| c == '_') {

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Oct 26, 2018

Contributor

It would be interesting to see if std::slice::memchr::memchr(b'_', s.as_bytes()).is_some() is faster than s.chars().any().

This comment has been minimized.

Copy link
@nnethercote

nnethercote Oct 29, 2018

Author Contributor

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.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Oct 29, 2018

Although I haven't changed the code, IMO I've addressed the comments above, so this is ready for re-review.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

Thanks, @nnethercote!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

📌 Commit eb637d2 has been approved by michaelwoerister

pietroalbini added a commit to pietroalbini/rust that referenced this pull request Oct 29, 2018

Rollup merge of rust-lang#55384 - nnethercote:better-integer_lit-floa…
…t_lit, r=michaelwoerister

Avoid unnecessary allocations in `float_lit` and `integer_lit`.

This commit avoids an allocation when parsing any float and integer
literals that don't involved underscores.

This reduces the number of allocations done for the `tuple-stress`
benchmark by 10%, reducing its instruction count by just under 1%.

bors added a commit that referenced this pull request Oct 29, 2018

Auto merge of #55462 - pietroalbini:rollup, r=pietroalbini
Rollup of 9 pull requests

Successful merges:

 - #54965 (update tcp stream documentation)
 - #55269 (fix typos in various places)
 - #55384 (Avoid unnecessary allocations in `float_lit` and `integer_lit`.)
 - #55423 (back out bogus `Ok`-wrapping suggestion on `?` arm type mismatch)
 - #55426 (Make a bunch of trivial methods of NonNull be `#[inline]`)
 - #55438 (Avoid directly catching BaseException in bootstrap configure script)
 - #55439 (Remove unused sys import from generate-deriving-span-tests)
 - #55440 (Remove unreachable code in hasClass function in Rustdoc)
 - #55447 (Fix invalid path in generate-deriving-span-tests.py.)

Failed merges:

r? @ghost

@bors bors merged commit eb637d2 into rust-lang:master Oct 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nnethercote nnethercote deleted the nnethercote:better-integer_lit-float_lit branch Oct 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.