Skip to content

stdlib: add a shortcut for Array.append(contentsOf:) in case the argument is an Array, too. #29220

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 1 commit into from
Jan 16, 2020
Merged

stdlib: add a shortcut for Array.append(contentsOf:) in case the argument is an Array, too. #29220

merged 1 commit into from
Jan 16, 2020

Conversation

eeckstein
Copy link
Contributor

This additional check lets the optimizer eliminate most of the append-code in specializations where the appended sequence is also an Array.
For example, when "adding" arrays, e.g. arr += other_arr

@eeckstein
stdlib: add a shortcut for Array.append(contentsOf:) in case the argu…
172c72b 
…ment is an Array, too.

This additional check lets the optimizer eliminate most of the append-code in specializations where the appended sequence is also an Array.
For example, when "adding" arrays, e.g. arr += other_arr
@eeckstein
Copy link
Contributor Author

@swift-ci test

@eeckstein
Copy link
Contributor Author

@swift-ci benchmark

@swift-ci
Copy link
Contributor

Performance: -O

Regression OLD NEW DELTA RATIO
ArrayAppendLazyMap 680 3550 +422.1% 0.19x
ArrayAppendUTF16 1462 1768 +20.9% 0.83x (?)
ArrayAppendLatin1 1462 1734 +18.6% 0.84x (?)
 
Improvement OLD NEW DELTA RATIO
ArrayAppendRepeatCol 590 500 -15.3% 1.18x (?)
DictOfArraysToArrayOfDicts 624 574 -8.0% 1.09x (?)
CharacterLiteralsSmall 213 196 -8.0% 1.09x (?)

Code size: -O

Improvement OLD NEW DELTA RATIO
PopFrontGeneric.o 2920 2584 -11.5% 1.13x
RangeOverlaps.o 6738 6034 -10.4% 1.12x
DictOfArraysToArrayOfDicts.o 22688 20432 -9.9% 1.11x
ArrayAppend.o 26311 24127 -8.3% 1.09x
PopFront.o 4107 3771 -8.2% 1.09x
BucketSort.o 8083 7699 -4.8% 1.05x
SortIntPyramids.o 9209 8813 -4.3% 1.04x
ObjectiveCBridgingStubs.o 16877 16269 -3.6% 1.04x
main.o 55587 53939 -3.0% 1.03x

Performance: -Osize

Regression OLD NEW DELTA RATIO
ArrayAppendLazyMap 680 4350 +539.7% 0.16x
ArrayAppendLatin1Substring 9432 19800 +109.9% 0.48x
ArrayAppendUTF16Substring 9252 19404 +109.7% 0.48x
ArrayAppendAsciiSubstring 9252 19368 +109.3% 0.48x
ArrayAppendSequence 680 860 +26.5% 0.79x
ArrayAppendLatin1 1564 1836 +17.4% 0.85x (?)
ArrayAppendAscii 1564 1768 +13.0% 0.88x (?)
ArrayAppendUTF16 1564 1768 +13.0% 0.88x (?)
FlattenListLoop 2613 2939 +12.5% 0.89x (?)
SortIntPyramid 465 520 +11.8% 0.89x (?)
SubstringFromLongStringGeneric 12 13 +8.3% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
FlattenListFlatMap 33947 3581 -89.5% 9.48x
ArrayAppendRepeatCol 590 500 -15.3% 1.18x (?)
DictOfArraysToArrayOfDicts 640 586 -8.4% 1.09x (?)
Chars2 3450 3200 -7.2% 1.08x (?)

Code size: -Osize

Improvement OLD NEW DELTA RATIO
DictOfArraysToArrayOfDicts.o 18964 16884 -11.0% 1.12x
PopFrontGeneric.o 2944 2624 -10.9% 1.12x
FlattenList.o 4114 3698 -10.1% 1.11x
RangeOverlaps.o 6324 5700 -9.9% 1.11x
ArrayAppend.o 22591 20935 -7.3% 1.08x
PopFront.o 3713 3457 -6.9% 1.07x
BucketSort.o 7875 7491 -4.9% 1.05x
ObjectiveCBridgingStubs.o 15301 14701 -3.9% 1.04x
main.o 47323 45683 -3.5% 1.04x
SortIntPyramids.o 9118 8806 -3.4% 1.04x

Performance: -Onone

Improvement OLD NEW DELTA RATIO
CharacterLiteralsLarge 490 452 -7.8% 1.08x (?)

Code size: -swiftlibs

Improvement OLD NEW DELTA RATIO
libswiftStdlibUnittest.dylib 339968 335872 -1.2% 1.01x
How to read the data The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview 
  Model Name: Mac mini
  Model Identifier: Macmini8,1
  Processor Name: Intel Core i7
  Processor Speed: 3.2 GHz
  Number of Processors: 1
  Total Number of Cores: 6
  L2 Cache (per Core): 256 KB
  L3 Cache: 12 MB
  Memory: 64 GB

@eeckstein
Copy link
Contributor Author

The ArrayAppendLazyMap regression is due to an unfortunate different inlining decision (which prevents a closure to be inlined). I don't think this is relevant in real world scenarios.

@airspeedswift
Copy link
Member

PopFrontGeneric.o | 2920 | 2584 | -11.5% | 1.13x
👀

@eeckstein eeckstein merged commit 2715c70 into swiftlang:master Jan 16, 2020
@eeckstein eeckstein deleted the improve-array-append branch January 16, 2020 10:51
lorentey added a commit to lorentey/swift that referenced this pull request Oct 25, 2020
@lorentey
[stdlib] Fix Array.append(contentsOf:) for arguments of type NSArray
184367c 
Due to a couple of unfortunate circumstances, appending an NSArray instance to an Array instance does not actually append any elements.

The cause is swiftlang#29220, which accidentally optimized away the actual loop that appends the elements in this particular case. (And only this particular case, which is why this wasn’t detected by the test suite.)

When the argument to `Array.append(contentsOf:)` is of type NSArray, the `newElements is [Element]` expression is compiled into a runtime check that returns true, eliminating the subsequent loop over the remaining items of the iterator. Sadly, NSArray.underestimatedCount` currently returns 0, so the earlier _copyContents call is a noop, so no elements get added to `self` at all.

Turning the `is` test into a direct equality check between the metatype instances resolves the issue.
@lorentey lorentey mentioned this pull request Oct 25, 2020
Merged
lorentey added a commit to lorentey/swift that referenced this pull request Oct 26, 2020
@lorentey
[stdlib] Fix Array.append(contentsOf:) for arguments of type NSArray
4826224 
Due to a couple of unfortunate circumstances, appending an NSArray instance to an Array instance does not actually append any elements.

The cause is swiftlang#29220, which accidentally optimized away the actual loop that appends the elements in this particular case. (And only this particular case, which is why this wasn’t detected by the test suite.)

When the argument to `Array.append(contentsOf:)` is of type NSArray, the `newElements is [Element]` expression is compiled into a runtime check that returns true, eliminating the subsequent loop over the remaining items of the iterator. Sadly, NSArray.underestimatedCount` currently returns 0, so the earlier _copyContents call is a noop, so no elements get added to `self` at all.

Turning the `is` test into a direct equality check between the metatype instances resolves the issue.

(cherry picked from commit 184367c)
@lorentey lorentey mentioned this pull request Oct 26, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@airspeedswift airspeedswift airspeedswift approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eeckstein @swift-ci @airspeedswift