Skip to content

[stdlib] don't copy array contents on removeAll(keepingCapacity: true) #66085

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.

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 25, 2023

Conversation

oxy
Copy link
Contributor

@oxy oxy commented May 23, 2023

When calling Array.removeAll(keepingCapacity: true), bypass the replaceSubrange call unless the buffer is uniquely referenced. Also, add a benchmark to measure performance for the new branch.

Based on @glessard's work in #39784.
Resolves #56321 (rdar://71809690).

@oxy oxy changed the title Gh56321 [stdlib] don't copy array contents on removeAll(keepingCapacity: true) May 23, 2023
@phausler
Copy link
Contributor

@swift-ci please test

@oxy
Copy link
Contributor Author

oxy commented May 24, 2023

@swift-ci please benchmark

@oxy
Copy link
Contributor Author

oxy commented May 24, 2023

Benchmark result summary:

  • +1-5% codesize overhead on -Osize and -O (Array.removeAll is @inlinable)
  • Array.removeAll(keepingCapacity: true) is now constant-time for arrays that are not uniquely referenced. (350-600us -> 0-5us with the current benchmark)

Details:

Performance (x86_64): -O

Improvement OLD NEW DELTA RATIO
Array.removeAll.keepingCapacity.Int 601.0 0.102 -100.0% 5834.96x
Array.removeAll.keepingCapacity.Object 358.0 5.727 -98.4% 62.50x

Code size: -O

Regression OLD NEW DELTA RATIO
ArrayRemoveAll.o 7464 7896 +5.8% 0.95x
MirrorTest.o 11422 11604 +1.6% 0.98x

Code size: -Osize

Regression OLD NEW DELTA RATIO
ArrayRemoveAll.o 6969 7374 +5.8% 0.95x
StringSplitting.o 34491 36318 +5.3% 0.95x
MirrorTest.o 11298 11468 +1.5% 0.99x

@oxy oxy requested a review from lorentey May 24, 2023 16:40
@oxy
Copy link
Contributor Author

oxy commented May 24, 2023

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SR-13923] Array.removeAll(keepingCapacity: true) wastes time copying memory around when Array is not uniquely referenced
3 participants