Skip to content

Attempt to use the high part of the size_hint in collect (again) #137908

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions library/alloc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@
#![feature(hashmap_internals)]
#![feature(intrinsics)]
#![feature(lang_items)]
#![feature(let_chains)]
#![feature(min_specialization)]
#![feature(multiple_supertrait_upcastable)]
#![feature(negative_impls)]
Expand Down
46 changes: 32 additions & 14 deletions library/alloc/src/vec/spec_from_iter_nested.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,39 @@ where
// empty, but the loop in extend_desugared() is not going to see the
// vector being full in the few subsequent loop iterations.
// So we get better branch prediction.
let mut vector = match iterator.next() {
None => return Vec::new(),
Some(element) => {
let (lower, _) = iterator.size_hint();
let initial_capacity =
cmp::max(RawVec::<T>::MIN_NON_ZERO_CAP, lower.saturating_add(1));
let mut vector = Vec::with_capacity(initial_capacity);
unsafe {
// SAFETY: We requested capacity at least 1
ptr::write(vector.as_mut_ptr(), element);
vector.set_len(1);
}
vector
}
let (low, high) = iterator.size_hint();
let Some(first) = iterator.next() else {
Comment on lines +25 to +26
Copy link
Member

Choose a reason for hiding this comment

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

I think the other comment still applies that some size_hints may be better after the first next -- or do you disagree?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've been meaning to come back and give that a shot. While it's certainly true, I'm also skeptical how valuable it is, since the usual case is things like flat_map that almost never have a good hint anyway -- and when they do, like flattening an iterator over arrays, it doesn't need the first one. But can try it.

(It makes me tempted to have a next_with_suggested_reserve -> Option<(NonZero<usize>, Item)>, too, but that's a bigger conversation.)

return Vec::new();
};
// `push`'s growth strategy is (currently) to double the capacity if
// there's no space available, so it can have up to 50% "wasted" space.
// Thus if the upper-bound on the size_hint also wouldn't waste more
// than that, just allocate it from the start. (After all, it's silly
// to allocate 254 for a hint of `(254, Some(255)`.)
let initial_capacity = {
// This is written like this to not overflow on any well-behaved iterator,
// even things like `repeat_n(val, isize::MAX as usize + 10)`
// where `low * 2` would need checking.
// A bad (but safe) iterator might have `low > high`, but if so it'll
// produce a huge `extra` that'll probably fail the following check.
let hint = if let Some(high) = high
&& let extra = high - low
&& extra < low
{
high
Copy link
Member

Choose a reason for hiding this comment

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

Should we cap this at isize::MAX bytes? It's conceivable on smaller targets that some iterator with a low near the edge isn't going to produce any more than that in practice, even if high would be too much, so maybe it's a bad idea to force a capacity error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that it would only ever matter for things that produce exactly low things. If it's just "near" the edge, it already panics today because it pre-allocates for low, then the low+1-th element tries to double it, and panics.

Said otherwise, this only uses the high as the hint if pushing could have tried to reserve that much anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I get that, but I still wonder if such "exactly low" cases exist that we would be harming.

I feel like the doubling logic should cap itself too, but that's a separate conversation.

Copy link
Member

Choose a reason for hiding this comment

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

What if we only try the high capacity first when it's in (low, low*2], or else fall back to just low.

} else {
low
};
cmp::max(RawVec::<T>::MIN_NON_ZERO_CAP, hint)
};
let mut vector = Vec::with_capacity(initial_capacity);
// SAFETY: We requested capacity at least MIN_NON_ZERO_CAP, which
// is never zero, so there's space for at least one element.
unsafe {
ptr::write(vector.as_mut_ptr(), first);
vector.set_len(1);
}

// must delegate to spec_extend() since extend() itself delegates
// to spec_from for empty Vecs
<Vec<T> as SpecExtend<T, I>>::spec_extend(&mut vector, iterator);
Expand Down
16 changes: 16 additions & 0 deletions tests/codegen/vec-collect-reserve-upper-bound.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//@ compile-flags: -Copt-level=3
#![crate_type = "lib"]

#[no_mangle]
pub fn should_use_low(a: [i32; 10], b: [i32; 100], p: fn(i32) -> bool) -> Vec<i32> {
// CHECK-LABEL: define void @should_use_low
// CHECK: call{{.+}}dereferenceable_or_null(40){{.+}}@__rust_alloc(
a.iter().copied().chain(b.iter().copied().filter(|x| p(*x))).collect()
}

#[no_mangle]
pub fn should_use_high(a: [i32; 100], b: [i32; 10], p: fn(i32) -> bool) -> Vec<i32> {
// CHECK-LABEL: define void @should_use_high
// CHECK: call{{.+}}dereferenceable_or_null(440){{.+}}@__rust_alloc(
a.iter().copied().chain(b.iter().copied().filter(|x| p(*x))).collect()
}
Loading