Skip to content

Try using withContiguousStorageIfAvailable in RangeReplaceableCollection.append(contentsOf:) before falling back to a slow element-by-element loop. #65778

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

Merged
merged 2 commits into from
May 19, 2023
Merged

Try using withContiguousStorageIfAvailable in RangeReplaceableCollection.append(contentsOf:) before falling back to a slow element-by-element loop. #65778

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
465aa22
Try using withContiguousStorageIfAvailable in RangeReplaceableCollect…
Catfish-Man May 8, 2023
5efc2da
_onFastPath, in inlineable code, will end up applying to the caller. …
Catfish-Man May 15, 2023
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
16 changes: 11 additions & 5 deletions stdlib/public/core/RangeReplaceableCollection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,11 +453,17 @@ extension RangeReplaceableCollection {
@inlinable
public mutating func append<S: Sequence>(contentsOf newElements: __owned S)
where S.Element == Element {

let approximateCapacity = self.count + newElements.underestimatedCount
self.reserveCapacity(approximateCapacity)
for element in newElements {
append(element)

let done:Void? = newElements.withContiguousStorageIfAvailable {
replaceSubrange(endIndex..<endIndex, with: $0)
}

if done == nil {
let approximateCapacity = self.count + newElements.underestimatedCount
self.reserveCapacity(approximateCapacity)
Copy link
Member

@lorentey lorentey May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Note for followup work: This reserveCapacity call is extremely undesirable, for the same reason append(_:) doesn't call reserveCapacity(self.count + 1): for contiguous collections, reserveCapacity does not use the usual exponential resizing logic, it allocates precisely as much memory as needed. This makes calling append(contentsOf:) in a loop algorithmically worse than if reserveCapacity wasn't used: in the worst case, it can cause calling append(contentsOf:) n times (with the same argument) to reallocate storage on every single invocation, rather than log(n) times.

for element in newElements {
append(element)
}
}
}

Expand Down