[ADT] Only call reserve on empty containers in append_values #172109
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kuhar
requested review from
amd-eochoalo,
kazutakahirata,
krzysz00 and
nikic
Member
|
@llvm/pr-subscribers-llvm-adt Author: Jakub Kuderski (kuhar) ChangesCalling reserve in a loop can prevent amortized constant time appends when the container doesn't round up the capacity. Full diff: https://github.com/llvm/llvm-project/pull/172109.diff 2 Files Affected:
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index 3c18aae1b519c..409cd7b60fa00 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -2152,7 +2152,17 @@ void append_range(Container &C, Range &&R) {
/// Appends all `Values` to container `C`.
template <typename Container, typename... Args>
void append_values(Container &C, Args &&...Values) {
- C.reserve(range_size(C) + sizeof...(Args));
+ if (size_t InitialSize = range_size(C); InitialSize == 0) {
+ // Only reserve if the container is empty. Reserving on a non-empty
+ // container may interfere with the exponential growth strategy, if the
+ // container does not round up the capacity. Consider `append_values` called
+ // repeatedly in a loop: each call would reserve exactly `size + N`, causing
+ // the capacity to grow linearly (e.g., 100 -> 105 -> 110 -> ...) instead of
+ // exponentially (e.g., 100 -> 200 -> ...). Linear growth turns the
+ // amortized O(1) append into O(n) because every few insertions trigger a
+ // reallocation and copy of all elements.
+ C.reserve(InitialSize + sizeof...(Args));
+ }
// Append all values one by one.
((void)C.insert(C.end(), std::forward<Args>(Values)), ...);
}
diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp
index e356f6b540568..ae79b852aecf5 100644
--- a/llvm/unittests/ADT/STLExtrasTest.cpp
+++ b/llvm/unittests/ADT/STLExtrasTest.cpp
@@ -701,6 +701,41 @@ TEST(STLExtrasTest, AppendValues) {
EXPECT_THAT(Set, UnorderedElementsAre(1, 2, 3));
}
+TEST(STLExtrasTest, AppendValuesReserve) {
+ // A vector wrapper that tracks reserve() calls.
+ struct TrackedVector : std::vector<int> {
+ using std::vector<int>::vector;
+ size_t LastReservedSize = 0;
+ unsigned ReserveCallCount = 0;
+
+ void reserve(size_t N) {
+ LastReservedSize = N;
+ ++ReserveCallCount;
+ std::vector<int>::reserve(N);
+ }
+ };
+
+ // When empty, reserve should be called.
+ TrackedVector Empty;
+ append_values(Empty, 1, 2, 3);
+ EXPECT_EQ(Empty.ReserveCallCount, 1u);
+ EXPECT_EQ(Empty.LastReservedSize, 3u);
+ EXPECT_THAT(Empty, ElementsAre(1, 2, 3));
+
+ // When non-empty, reserve should NOT be called to avoid preventing
+ // exponential growth.
+ TrackedVector NonEmpty = {1, 2};
+ append_values(NonEmpty, 3, 4);
+ EXPECT_EQ(NonEmpty.ReserveCallCount, 0u);
+ EXPECT_THAT(NonEmpty, ElementsAre(1, 2, 3, 4));
+
+ // Appending more values to a now non-empty container should still not
+ // reserve.
+ append_values(Empty, 4, 5);
+ EXPECT_EQ(Empty.ReserveCallCount, 1u); // Still 1 from initial call.
+ EXPECT_THAT(Empty, ElementsAre(1, 2, 3, 4, 5));
+}
+
TEST(STLExtrasTest, ADLTest) {
some_namespace::some_struct s{{1, 2, 3, 4, 5}, ""};
some_namespace::some_struct s2{{2, 4, 6, 8, 10}, ""};
|
krzysz00
approved these changes
Dec 13, 2025
nikic
reviewed
Dec 13, 2025
| template <typename Container, typename... Args> | ||
| void append_values(Container &C, Args &&...Values) { | ||
| C.reserve(range_size(C) + sizeof...(Args)); | ||
| if (size_t InitialSize = range_size(C); InitialSize == 0) { |
Contributor
There was a problem hiding this comment.
Side note: This code should be using llvm::size() and not range_size(). It makes no sense to do a full container scan with std::distance() in this context.
If we have any usages where size() is not available, those should just not attempt to reserve() at all.
Member
Author
There was a problem hiding this comment.
I got back to this line of work recently and think we need some fast size function that allows for both random access iterators and containers with .size(), so a combination of std::size (arrays and .size()) and llvm::size (random access only).
Here specifically, I'm not aware of any containers that have .insert() but don't have (fast) .size() (almost every usage is with vector and small vector), so ACK but I'm going to address this separately.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/20301 Here is the relevant piece of the build log for the reference |
anonymouspc
pushed a commit
to anonymouspc/llvm
that referenced
this pull request
Dec 15, 2025
…2109) Calling reserve in a loop can prevent amortized constant time appends when the container doesn't round up the capacity.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Calling reserve in a loop can prevent amortized constant time appends when the container doesn't round up the capacity.