Skip to content
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

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 14 commits into from
+163 −4

Conversation

Catfish-Man
Copy link
Contributor

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

Fixes rdar://139455035

@Catfish-Man
Add an overload of append(contentsOf:) on Array that takes a Collecti…
70529b9 
…on instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@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
Add an overload of append(contentsOf:) on ArraySlice that takes a Col…
81106a5 
…lection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@Catfish-Man
Copy link
Contributor Author

@swift-ci Apple Silicon benchmark

@Catfish-Man
Add an overload of append(contentsOf:) on ContiguousArray that takes …
47c486f 
…a Collection instead of a Sequence, and try using it to accelerate wCSIA-compatible Sequences
@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

@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
stdlib/public/core/Array.swift Outdated Show resolved Hide resolved
stdlib/public/core/Array.swift Outdated
/// - Complexity: O(*m*) on average, where *m* is the length of
/// `newElements`, over many calls to `append(contentsOf:)` on the same
/// array.
@inlinable @_alwaysEmitIntoClient
Copy link
Member

@lorentey lorentey Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: This is a little misleading: @_alwaysEmitIntoClient already implies inlinability, but @inlinable carries an expectation that it will export an ABI symbol. I recommend just using aeic, like we do in most similar cases in the stdlib.

Suggested change
@inlinable @_alwaysEmitIntoClient
@_alwaysEmitIntoClient

Update: D'oh, we appear to be less consistent about this than I thought. The Array code is pretty consistent though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll fix this once I figure out the SIL test failure :) I had just @inlinable originally but Steve correctly pointed out that would make the Sequence-taking one explode when back-deployed if it didn't get inlined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

stdlib/public/core/Array.swift Show resolved Hide resolved
@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

@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 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
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

@AhmedIAlsayed AhmedIAlsayed AhmedIAlsayed approved these changes

@stephentyrone stephentyrone stephentyrone approved these changes

@glessard glessard glessard approved these changes

@eeckstein eeckstein Awaiting requested review from eeckstein eeckstein is a code owner

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