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.

Sign up for GitHub

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
Merged

Avoid SmallVec::collect #64949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1419260
Avoid `SmallVec::collect()` in `SubstsRef::super_fold_with()`.
nnethercote Oct 1, 2019
1937c20
Avoid `SmallVec::collect()` in `Result::intern_with()`.
nnethercote Oct 1, 2019
d1a7bb3
Avoid `SmallVec::collect()` in `List<Predicate>::super_fold_with()`.
nnethercote Oct 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions src/librustc/ty/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2848,8 +2848,29 @@ impl<'a, T, R> InternIteratorElement<T, R> for &'a T

impl<T, R, E> InternIteratorElement<T, R> for Result<T, E> {
type Output = Result<R, E>;
fn intern_with<I: Iterator<Item=Self>, F: FnOnce(&[T]) -> R>(iter: I, f: F) -> Self::Output {
Ok(f(&iter.collect::<Result<SmallVec<[_; 8]>, _>>()?))
fn intern_with<I: Iterator<Item=Self>, F: FnOnce(&[T]) -> R>(mut iter: I, f: F)
-> Self::Output {
// 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// lower bounds from `size_hint` agree they are correct.
Ok(match iter.size_hint() {
(1, Some(1)) => {
f(&[iter.next().unwrap()?])
}
(2, Some(2)) => {
let t0 = iter.next().unwrap()?;
let t1 = iter.next().unwrap()?;
f(&[t0, t1])
}
(0, Some(0)) => {
f(&[])
}
_ => {
f(&iter.collect::<Result<SmallVec<[_; 8]>, _>>()?)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
})
}
}

Expand Down
17 changes: 15 additions & 2 deletions src/librustc/ty/structural_impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1223,8 +1223,21 @@ BraceStructTypeFoldableImpl! {

impl<'tcx> TypeFoldable<'tcx> for &'tcx ty::List<ty::Predicate<'tcx>> {
fn super_fold_with<F: TypeFolder<'tcx>>(&self, folder: &mut F) -> Self {
let v = self.iter().map(|p| p.fold_with(folder)).collect::<SmallVec<[_; 8]>>();
folder.tcx().intern_predicates(&v)
// This code is hot enough that it's worth specializing for a list of
// length 0. (No other length is common enough to be worth singling
// out).
if self.len() == 0 {
self
} else {
// Don't bother interning if nothing changed, which is the common
// case.
let v = self.iter().map(|p| p.fold_with(folder)).collect::<SmallVec<[_; 8]>>();
if v[..] == self[..] {
self
} else {
folder.tcx().intern_predicates(&v)
}
}
}

fn super_visit_with<V: TypeVisitor<'tcx>>(&self, visitor: &mut V) -> bool {
Expand Down
43 changes: 35 additions & 8 deletions src/librustc/ty/subst.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,14 +383,41 @@ impl<'a, 'tcx> InternalSubsts<'tcx> {

impl<'tcx> TypeFoldable<'tcx> for SubstsRef<'tcx> {
fn super_fold_with<F: TypeFolder<'tcx>>(&self, folder: &mut F) -> Self {
let params: SmallVec<[_; 8]> = self.iter().map(|k| k.fold_with(folder)).collect();

// If folding doesn't change the substs, it's faster to avoid
// calling `mk_substs` and instead reuse the existing substs.
if params[..] == self[..] {
self
} else {
folder.tcx().intern_substs(&params)
// 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 90--99.99% of cases. When folding doesn't change
// the substs, it's faster to reuse the existing substs rather than
// calling `intern_substs`.
match self.len() {
1 => {
let param0 = self[0].fold_with(folder);
if param0 == self[0] {
self
} else {
folder.tcx().intern_substs(&[param0])
}
}
2 => {
nnethercote marked this conversation as resolved.
Show resolved Hide resolved
let param0 = self[0].fold_with(folder);
let param1 = self[1].fold_with(folder);
if param0 == self[0] && param1 == self[1] {
self
} else {
folder.tcx().intern_substs(&[param0, param1])
}
}
0 => {
self
}
_ => {
let params: SmallVec<[_; 8]> = self.iter().map(|k| k.fold_with(folder)).collect();
if params[..] == self[..] {
self
} else {
folder.tcx().intern_substs(&params)
}
}
}
}

Expand Down