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

Reduce the impact of Vec::reserve calls that do not cause any allocation #83357

Merged
merged 2 commits into from
Mar 30, 2021
+17 −1
Merged

Reduce the impact of Vec::reserve calls that do not cause any allocation #83357

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5e3710
Mark RawVec::reserve as inline and outline the resizing logic
saethlin Mar 21, 2021
73d7734
fmt, change to cold
saethlin Mar 21, 2021
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
18 changes: 17 additions & 1 deletion library/alloc/src/raw_vec.rs
View file
Original file line number Diff line number Diff line change
@@ -315,8 +315,24 @@ impl<T, A: Allocator> RawVec<T, A> {
/// # vector.push_all(&[1, 3, 5, 7, 9]);
/// # }
/// ```
#[inline]
pub fn reserve(&mut self, len: usize, additional: usize) {
handle_reserve(self.try_reserve(len, additional));
// Callers expect this function to be very cheap when there is already sufficient capacity.
// Therefore, we move all the resizing and error-handling logic from grow_amortized and
// handle_reserve behind a call, while making sure that the this function is likely to be
// inlined as just a comparison and a call if the comparison fails.
#[cold]
Copy link
Member

Choose a reason for hiding this comment

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

Why mark this as cold instead of inline(never)? I would expect this to be called somewhat frequently in a normal program.

Copy link
Member

@the8472 the8472 Mar 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

From what I have read in other comments is that cold simply means llvm's coldcc. So it minimizes the impact on the caller, isn't that the goal here? We assume the caller code to be hotter.

Copy link
Contributor

Choose a reason for hiding this comment

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

In some places inside vec (and some other modules) it's annotated via

#[cold]
#[inline(never)]

Copy link
Member Author

Choose a reason for hiding this comment

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

@jyn514 That's kind of right and I'm taking a new look at this PR with the same goal of reducing the amount of time spent in reserve calls that don't cause any capacity change.

It looks to me like most of the Vec::reserve calls are guarded by a capacity check (such as the one in push). But some aren't. This manifests in the serde_json benchmarks because serializers do a lot of Write::write_all calls which is implemented by Vec::extend_from_slice, which is implemented (eventually) by append_elements, which has an unguarded self.reserve. I'm assessing adding an if self.len() + additional > self.capacity() guard around the call in append_elements, and going to take a look at either guarding all the reserve calls or sinking the check into Vec::reserve, and deduplicating the check around a lot of the call sites in Vec.

Copy link
Member

@scottmcm scottmcm Mar 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From what I have read in other comments is that cold simply means llvm's coldcc.

Note that #[cold] is the cold function attribute https://llvm.org/docs/LangRef.html#function-attributes

Changing the ABI (and thus having incompatible fn pointers) is extern "rust-cold" (#97544).

EDIT: Sorry, didn't realize this thread was so old 🤦 It was 5th on the "most recently updated" list I was looking at.

fn do_reserve_and_handle<T, A: Allocator>(
slf: &mut RawVec<T, A>,
len: usize,
additional: usize,
) {
handle_reserve(slf.grow_amortized(len, additional));
}

if self.needs_to_grow(len, additional) {
do_reserve_and_handle(self, len, additional);
}
}

/// The same as `reserve`, but returns on errors instead of panicking or aborting.