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 `SmallVec::collect` #64949

Merged
merged 3 commits into from Oct 8, 2019

Conversation

@nnethercote
Copy link
Contributor

nnethercote commented Oct 1, 2019

We can get sizeable speed-ups by avoiding SmallVec::collect when the number of elements is small.

nnethercote added 3 commits Oct 1, 2019
This commit reduces instruction counts for several benchmarks by up to
5%.
This commit reduces instruction counts for several benchmarks by up to
5%.
Also avoid interning when it's not necessary.

This commit reduces instruction counts for a couple of benchmarks by up to 1%.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 1, 2019

r? @zackmdavis

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

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 1, 2019

Some local perf results:

wg-grammar-check
        avg: -5.1%      min: -7.8%      max: -0.3%
serde-check
        avg: -4.1%      min: -6.5%      max: -0.2%
futures-check
        avg: -2.7%      min: -5.7%      max: -0.2%
deeply-nested-check
        avg: -3.3%      min: -5.3%      max: -0.0%
regression-31157-check
        avg: -2.4%      min: -4.9%      max: -0.1%
unify-linearly-check
        avg: -2.3%      min: -4.8%      max: -0.0%
packed-simd-check
        avg: -3.0%      min: -4.6%      max: -0.9%
piston-image-check
        avg: -1.8%      min: -3.7%      max: -0.3%
webr_api-check
        avg: -1.9%      min: -3.0%      max: -0.1%
clap-rs-check
        avg: -2.3%      min: -2.9%      max: -1.1%
issue-46449-check
        avg: -2.1%      min: -2.8%      max: -0.0%
cargo-check
        avg: -1.3%      min: -2.6%      max: -0.2%
syn-check
        avg: -1.1%      min: -2.2%      max: -0.1%
ripgrep-check
        avg: -1.0%      min: -2.1%      max: -0.1%
style-servo-check
        avg: -1.2%      min: -1.9%      max: -0.1%
webrender-check
        avg: -1.0%      min: -1.9%      max: -0.2%
regex-check
        avg: -0.6%      min: -1.8%      max: -0.1%
cranelift-codegen-check
        avg: -1.0%      min: -1.7%      max: -0.1%
script-servo-check
        avg: -0.7%      min: -1.6%      max: -0.1%
encoding-check
        avg: -0.7%      min: -1.4%      max: -0.1%
html5ever-check
        avg: -0.7%      min: -1.4%      max: -0.1%       
coercions-check
        avg: -0.6%?     min: -1.2%?     max: 0.1%?       
keccak-check
        avg: -0.4%      min: -0.6%      max: -0.1%
tuple-stress-check
        avg: -0.4%      min: -0.6%      max: -0.0%
ucd-check
        avg: -0.4%      min: -0.6%      max: -0.0%
deep-vector-check
        avg: -0.3%      min: -0.5%      max: 0.0%

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 1, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 1, 2019

⌛️ Trying commit d1a7bb3 with merge 64cbb2f...

bors added a commit that referenced this pull request Oct 1, 2019
Avoid `SmallVec::collect`

We can get sizeable speed-ups by avoiding `SmallVec::collect` when the number of elements is small.
// This code is hot enough that it's worth specializing for the most
// common length lists, to avoid the overhead of `SmallVec` creation.
// The match arms are in order of frequency. The 1, 2, and 0 cases are
// typically hit in ~95% of cases. We assume that if the upper and

This comment has been minimized.

Copy link
@Centril

Centril Oct 1, 2019

Member

Is this assumption OK? It seems to run counter to the Iterator docs -- In particular, are you sure that no unsafe code in the compiler relies on the correctness of collecting? You could specialize for TrustedLen in which case this assumption is definitely OK.

This comment has been minimized.

Copy link
@nnethercote

nnethercote Oct 1, 2019

Author Contributor

First I tried adding blanket TrustedLen requirements, but some of the iterators that feed into this method don't implement TrustedLen.

Then I tried to use specialization but failed. The problem is that we have this function:

fn intern_with<I: Iterator<Item=Self>, F: FnOnce(&[T]) -> R>(iter: I, f: F)

And we want one behaviour if I implements TrustedLen, and another behaviour if it doesn't. But you can't do that with an argument. This function appears within an impl block for Result<T, E>, i.e. I isn't part of the impl type.

As for the existing code... if the iterator gives fewer elements than the size hint (e.g. 1 when the hint was (2, Some(2))) then an unwrap will safely fail within the function. If the iterator gives more elements than the size hint (e.g. 2 when the hint was (1, Some(1))) then the wrong value will be interned. This could certainly cause correctness issues. It seems unlikely it could lead to unsafety, though I can't guarantee it. Also, it seems unlikely that an iterator would erroneously claim an exact size hint (i.e. where the lower bound and the upper bound match).

This comment has been minimized.

Copy link
@RalfJung

RalfJung Oct 12, 2019

Member

I can't say that reasoning based on probability makes me very confident in the safety of this change. :/

How bad/expensive would it be to assert! that calling next() again yields None?

This comment has been minimized.

Copy link
@Zoxc

Zoxc Oct 20, 2019

Contributor

I don't see any unsafe code here, and size_hint is required to produce conservative lower and upper bounds. If the bounds are the same, we know the size exactly.

src/librustc/ty/subst.rs Show resolved Hide resolved
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 1, 2019

☀️ Try build successful - checks-azure
Build commit: 64cbb2f (64cbb2f5a941712579d0a61b5ef6fba5af80d0e4)

f(&[])
}
_ => {
f(&iter.collect::<Result<SmallVec<[_; 8]>, _>>()?)

This comment has been minimized.

Copy link
@bluss

bluss Oct 3, 2019

Member

Since the small cases are already taken care of above, could it be a win to just collect to Vec here?

This comment has been minimized.

Copy link
@nnethercote

nnethercote Oct 3, 2019

Author Contributor

I haven't measured, but I suspect Vec would be slower because it always requires an allocation. The SmallVec only requires an allocation if the length exceeds 8, which is extremely rare.

Also, this collect-into-SmallVec idiom is very common within this module, so I think it's good for consistency reasons, too.

@zackmdavis

This comment has been minimized.

Copy link
Member

zackmdavis commented Oct 7, 2019

Thanks! 💖

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 7, 2019

📌 Commit d1a7bb3 has been approved by zackmdavis

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 8, 2019

⌛️ Testing commit d1a7bb3 with merge ec557aa...

bors added a commit that referenced this pull request Oct 8, 2019
Avoid `SmallVec::collect`

We can get sizeable speed-ups by avoiding `SmallVec::collect` when the number of elements is small.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 8, 2019

☀️ Test successful - checks-azure
Approved by: zackmdavis
Pushing ec557aa to master...

@bors bors added the merged-by-bors label Oct 8, 2019
@bors bors merged commit d1a7bb3 into rust-lang:master Oct 8, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191001.22 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@nnethercote nnethercote deleted the nnethercote:avoid-SmallVec-collect branch Oct 8, 2019
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Oct 10, 2019

I didn't do a CI perf run before landing, but here are the post-landing result. Lots of wins, up to 7.1%.

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