Try using withContiguousStorageIfAvailable in RangeReplaceableCollection.append(contentsOf:) before falling back to a slow element-by-element loop. #65778
Conversation
|
@swift-ci please benchmark |
|
|
||
| if done == nil { | ||
| let approximateCapacity = self.count + newElements.underestimatedCount | ||
| self.reserveCapacity(approximateCapacity) |
There was a problem hiding this comment.
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.
…ion.append(contentsOf:) before falling back to a slow element-by-element loop. Fixes rdar://109059874
Catfish-Man
force-pushed
the
contiguous-conundrum
branch
from
740d037 to
465aa22
Compare
|
@swift-ci please smoke benchmark |
|
adding some benchmarks in #65802 |
|
@swift-ci please benchmark |
|
Yup, new benchmarks detect the improvement, to say the least
|
…We don't want that, so omit it
|
@swift-ci please test |
|
@swift-ci please smoke benchmark |
There was a problem hiding this comment.
We should make sure that both paths are exercised in our test suite, by e.g. adding dedicated tests for both. (If we are already testing that, then of course this is moot.)
This is an observable change of calling patterns, so ideally we should also have a test that verifies that withContiguousStorageIfAvailable is invoked -- however, adding that may be more effort than it's worth.
|
Tests here #65953 |
|
@swift-ci please test and merge |
Fixes rdar://109059874
Should make a bunch of operations on naive RRC implementations much faster