[stdlib][performance] skip copying old values during removeAll(keepCapacity: true) #616
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
dduan
changed the title
dduan
force-pushed
the
removeAll_speedup
branch
from
c48b898 to
2111f7b
Compare
As noted in FIXME(performance), calling `.removeAll(keepCapacity: true)` on a containter of type Dictionay or Set causes a copy of its storage being made. Only then, it would proceed to delete each element. This patch makes these hashed collections skip the wasteful copying step.
dduan
force-pushed
the
removeAll_speedup
branch
from
2111f7b to
b177c69
Compare
|
@dduan Can you please write a simple benchmark that verifies that things go faster now? |
|
@dduan Excellent!! |
|
Your thoughts, @gribozavr ? |
|
Only a 438x improvement? I don't know, perhaps we should pass on this. :-) +1 from me, but @gribozavr or @dabrahams need to approve it. |
|
@dduan Looks great, thank you! |
gribozavr
added a commit
that referenced
this pull request
Dec 18, 2015
[stdlib][performance] skip copying old values during removeAll(keepCapacity: true)
freak4pc
pushed a commit
to freak4pc/swift
that referenced
this pull request
Sep 28, 2022
Add a Swift 5.0 version of Result
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
(This is a re-submission of #477 with clearer patch and description.)
For
.removeAll(keepCapacity: true)on a non-uniquely referencedDictionaryorSet, the existing code implements COW by callingensureUniqueNativeStorage(), which actually copies the content of its storage. Only then the elements get deleted one by one.A FIXME(performance) comment stated this wastefulness (benchmark result posted in #477 agrees). A better solution is directly replace the native storage with equal capacity.
A note on Set: As @dabrahams pointed out,
aSet.removeAll()is the equivalent ofaSet.intersectInPlace([]). Ideally,intersectInPlace(), being the more general case, should be the target for optimization. In successfully doing so, the equivalence in math can be directly realized in code.In that light, this patch can be treated as solely an optimization for
Dictionary. The speedup for Set is a side-effect (courtesy of GYP and architect of this file).Tests in both
swift/validation-test/stdlib/Set.swiftand./lit swift/validation-test/stdlib/Dictionary.swiftare passing.