Skip to content

Add an overload of append(contentsOf:) on Array that takes a Collection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences #77487

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

Jump to bottom
Merged
merged 13 commits into from
Dec 14, 2024
Merged

Add an overload of append(contentsOf:) on Array that takes a Collection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences #77487

merged 13 commits into from
Dec 14, 2024

Conversation

Catfish-Man
Copy link
Contributor

@Catfish-Man Catfish-Man commented Nov 8, 2024 •
edited
Loading

Fixes rdar://139455035

@Catfish-Man Catfish-Man self-assigned this Nov 8, 2024
@Catfish-Man Catfish-Man requested a review from a team as a code owner November 8, 2024 17:05
@Catfish-Man
Copy link
Contributor Author

@swift-ci test

@Catfish-Man
Copy link
Contributor Author

@swift-ci Apple Silicon benchmark

Catfish-Man
Catfish-Man commented Nov 8, 2024
View reviewed changes
@Catfish-Man
Copy link
Contributor Author

@swift-ci Apple Silicon benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci Apple Silicon benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci please benchmark

@Catfish-Man
Copy link
Contributor Author 

------- Performance (arm64): -O -------

REGRESSION                                OLD      NEW        DELTA    RATIO    
ConvertFloatingPoint.MockFloat64ToInt64   292.5    339.286    +16.0%   **0.86x**
FindString.Rec3.Array                     69.943   80.133     +14.6%   **0.87x**
NSError                                   51.436   55.703     +8.3%    **0.92x (?)**
StringFromLongWholeSubstring              1.854    1.998      +7.8%    **0.93x**

IMPROVEMENT                               OLD      NEW        DELTA    RATIO    
ArrayAppendLatin1Substring                9288.0   1633.787   -82.4%   **5.68x**
ArrayAppendAsciiSubstring                 9012.0   1593.0     -82.3%   **5.66x**
ArrayAppendUTF16Substring                 9012.0   1594.4     -82.3%   **5.65x**
LineSink.scalars.alpha                    28.988   17.0       -41.4%   **1.71x**
FlattenListFlatMap                        2600.0   2243.0     -13.7%   **1.16x (?)**

------- Code size: -O -------

REGRESSION          OLD     NEW     DELTA    RATIO  
QueueTest.o         7615    9363    +23.0%   **0.81x**
FindStringNaive.o   5341    5857    +9.7%    **0.91x**
ArrayAppend.o       19641   20873   +6.3%    **0.94x**
StringSplitting.o   27567   28339   +2.8%    **0.97x**

IMPROVEMENT         OLD     NEW     DELTA    RATIO  
FlattenList.o       2738    2478    -9.5%    **1.10x**
Suffix.o            15518   14974   -3.5%    **1.04x**
RangeOverlaps.o     4475    4367    -2.4%    **1.02x**

@Catfish-Man
Copy link
Contributor Author

@swift-ci test

1 similar comment
@Catfish-Man
Copy link
Contributor Author

@swift-ci test

@Catfish-Man Catfish-Man enabled auto-merge (squash) November 8, 2024 21:43
@Catfish-Man
Copy link
Contributor Author

Asked Erik to take a look at the test failure, since it's about the _effects thing we discussed

lorentey
lorentey previously requested changes Nov 11, 2024
View reviewed changes
@lorentey lorentey disabled auto-merge November 11, 2024 20:33
@Catfish-Man
Copy link
Contributor Author

Not clear to me why pushing that auto-requested review from Erik…

@Catfish-Man
Copy link
Contributor Author

@swift-ci test

1 similar comment
@Catfish-Man
Copy link
Contributor Author

@swift-ci test

@Catfish-Man
Copy link
Contributor Author

@swift-ci Apple Silicon benchmark

@Catfish-Man
Copy link
Contributor Author

@swift-ci test

@Catfish-Man
Copy link
Contributor Author

@swift-ci Apple Silicon benchmark

lorentey
lorentey reviewed Nov 12, 2024
View reviewed changes
@lorentey lorentey dismissed their stale review November 12, 2024 02:47

The issue I noted was addressed, but there are parts I still don't understand.

@Catfish-Man
Copy link
Contributor Author

@swift-ci please Apple Silicon benchmark

Catfish-Man
Catfish-Man commented Nov 12, 2024
View reviewed changes
stdlib/public/core/ContiguousArrayBuffer.swift
@@ -1063,6 +1063,12 @@ extension Sequence {
internal func _copySequenceToContiguousArray<
S: Sequence
>(_ source: S) -> ContiguousArray<S.Element> {
let contigArray = source.withContiguousStorageIfAvailable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part is new btw

@Catfish-Man
Copy link
Contributor Author

Well that was completely ridiculous. Wasted several hours because build-script stopped rebuilding the tests, so I kept getting failures from the old code rather than my fix.

@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci Apple Silicon benchmark

@Catfish-Man Catfish-Man enabled auto-merge (squash) November 13, 2024 01:55
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

Catfish-Man and others added 11 commits December 6, 2024 16:23
@Catfish-Man
Add an overload of append(contentsOf:) on Array that takes a Collecti…
acb9893 
…on instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@Catfish-Man
Add an overload of append(contentsOf:) on ArraySlice that takes a Col…
346484e 
…lection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@Catfish-Man
Add an overload of append(contentsOf:) on ContiguousArray that takes …
716ea70 
…a Collection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@Catfish-Man
Add _aEIC in one place it was missing
53a2c3a
@Catfish-Man
Remove inlinable, aEIC takes care of it
258c5b5
@Catfish-Man @lorentey
Assert that the incoming elements have a correct count implementation
71a8c89 
Co-authored-by: Karoy Lorentey <[email protected]>
@Catfish-Man
Use withContiguousStorageIfAvailable in _copySequenceToContiguousArra…
b07c7ed 
…y too
@Catfish-Man
Verify counts in ArraySlice and ContiguousArray too
5d593ec
@Catfish-Man
Build fix
716bbfd
@Catfish-Man
Remove effect annotations from types that don't currently use them
0b25e51
@Catfish-Man
Fix assertion failures
da87c55
@Catfish-Man Catfish-Man force-pushed the what-about-second-append-contentsof branch from 0dede68 to da87c55 Compare December 7, 2024 00:25
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

1 similar comment
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man Catfish-Man disabled auto-merge December 14, 2024 00:33
@Catfish-Man
Copy link
Contributor Author

@swift-ci please test

@Catfish-Man
Copy link
Contributor Author

@swift-ci Apple Silicon benchmark

@Catfish-Man Catfish-Man enabled auto-merge (squash) December 14, 2024 01:26
@Catfish-Man Catfish-Man merged commit b71f768 into main Dec 14, 2024
5 of 6 checks passed
@Catfish-Man Catfish-Man deleted the what-about-second-append-contentsof branch December 14, 2024 07:11
@hamishknight hamishknight mentioned this pull request Dec 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorentey lorentey lorentey left review comments

@glessard glessard glessard approved these changes

@stephentyrone stephentyrone stephentyrone approved these changes

@AhmedIAlsayed AhmedIAlsayed AhmedIAlsayed approved these changes

@eeckstein eeckstein Awaiting requested review from eeckstein

Assignees

@Catfish-Man Catfish-Man

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Catfish-Man @glessard @lorentey @stephentyrone @AhmedIAlsayed