Skip to content

[cxx-interop] Optimize CxxIterator and CxxSequence #61577

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 1 commit into from
Oct 14, 2022

Conversation

egorzhdan
Copy link
Contributor

  1. Instead of storing the C++ collection within class CxxIterator, store a boxed C++ collection and make CxxIterator a struct. This enables a number of Swift optimizations for iterators, while preserving the guarantee that the C++ collection is not copied or moved in memory (which would invalidate iterators).
  2. Make sequence parameter shared. This avoids a second copy of the C++ collection when initializing CxxIterator.

Credits to Alex Lorenz for this idea!

@egorzhdan egorzhdan added the c++ interop Feature: Interoperability with C++ label Oct 14, 2022
@egorzhdan egorzhdan requested review from hyp and zoecarver October 14, 2022 13:51
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please benchmark

@egorzhdan
Copy link
Contributor Author

Regression OLD NEW DELTA RATIO
FlattenListLoop 976 1613 +65.3% 0.61x (?)
CxxVecU32.sum.Swift.subscriptLoop 48 76 +58.3% 0.63x
CxxVecU32.sum.Swift.iteratorLoop 48 76 +58.3% 0.63x
Dictionary4 158 202 +27.8% 0.78x (?)
 
Improvement OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.forInLoop 6647 2677 -59.7% 2.48x
CxxVecU32.sum.Swift.reduce 6792 5305 -21.9% 1.28x
StringWalk 1520 1360 -10.5% 1.12x (?)
NSError 133 120 -9.8% 1.11x (?)
SortStrings 507 472 -6.9% 1.07x (?)

Code size: -Osize

Regression OLD NEW DELTA RATIO
CxxVectorSum.o 8512 8780 +3.1% 0.97x

Performance (x86_64): -Onone

Improvement OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.forInLoop 11274 7849 -30.4% 1.44x
CxxVecU32.sum.Swift.reduce 12824 10097 -21.3% 1.27x

Code size: -swiftlibs

@egorzhdan
Copy link
Contributor Author

These two benchmarks should not be affected by the change, since they use raw C++ iterators. I'm not sure why the results have changed:

CxxVecU32.sum.Swift.subscriptLoop
CxxVecU32.sum.Swift.iteratorLoop

@egorzhdan
Copy link
Contributor Author

@swift-ci please benchmark

1. Instead of storing the C++ collection within `class CxxIterator`, store a boxed C++ collection and make `CxxIterator` a struct. This enables a number of Swift optimizations for iterators, while preserving the guarantee that the C++ collection is not copied or moved in memory (which would invalidate iterators).
2. Make `sequence` parameter shared. This avoids a second copy of the C++ collection when initializing `CxxIterator`.

Credits to Alex Lorenz for this idea!
@egorzhdan
Copy link
Contributor Author

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 2542 4340 +70.7% 0.59x (?)
FlattenListLoop 995 1618 +62.6% 0.61x (?)
 
Improvement OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.forInLoop 6443 2703 -58.0% 2.38x
CxxVecU32.sum.Swift.reduce 6664 5247 -21.3% 1.27x (?)
Dictionary3 150 121 -19.3% 1.24x (?)

Code size: -O

Regression OLD NEW DELTA RATIO
CxxVectorSum.o 9570 9818 +2.6% 0.97x

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 2397 4418 +84.3% 0.54x (?)
FlattenListLoop 979 1616 +65.1% 0.61x (?)
CxxVecU32.sum.Swift.subscriptLoop 48 76 +58.3% 0.63x
CxxVecU32.sum.Swift.iteratorLoop 48 76 +58.3% 0.63x (?)
Dictionary4 158 202 +27.8% 0.78x (?)
 
Improvement OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.forInLoop 6657 2681 -59.7% 2.48x
CxxVecU32.sum.Swift.reduce 6800 5307 -22.0% 1.28x (?)
StrComplexWalk 3240 2950 -9.0% 1.10x (?)
StringWalk 1520 1400 -7.9% 1.09x (?)
Chars2 3750 3500 -6.7% 1.07x (?)

Code size: -Osize

Regression OLD NEW DELTA RATIO
CxxVectorSum.o 8512 8780 +3.1% 0.97x

Performance (x86_64): -Onone

Improvement OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.forInLoop 11261 7854 -30.3% 1.43x
CxxVecU32.sum.Swift.reduce 12560 10078 -19.8% 1.25x (?)

Code size: -swiftlibs

@egorzhdan egorzhdan force-pushed the egorzhdan/cxx-iterator-boxing branch from b8ab3d1 to 10b7057 Compare October 14, 2022 15:54
@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test

@egorzhdan
Copy link
Contributor Author

@swift-ci please benchmark

@egorzhdan
Copy link
Contributor Author

@swift-ci please smoke test Windows

@zoecarver
Copy link
Contributor

I'm not sure why the results have changed

It's just noise. Any benchmark under 100us is going to be mostly noise (maybe these ones are helpful to establish a baseline though).

Copy link
Contributor

@zoecarver zoecarver left a comment

Choose a reason for hiding this comment

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

This is awesome, thanks Egor!

@egorzhdan
Copy link
Contributor Author

Performance (x86_64): -O

Improvement OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.reduce 6669 21 -99.7% 317.56x
CxxVecU32.sum.Swift.forInLoop 6445 21 -99.7% 306.89x
FlattenListFlatMap 2900 2620 -9.7% 1.11x (?)

Code size: -O

Regression OLD NEW DELTA RATIO
CxxVectorSum.o 9570 10256 +7.2% 0.93x

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.subscriptLoop 48 76 +58.3% 0.63x
CxxVecU32.sum.Swift.iteratorLoop 48 76 +58.3% 0.63x
 
Improvement OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.reduce 6800 56 -99.2% 121.43x
CxxVecU32.sum.Swift.forInLoop 6701 56 -99.2% 119.66x
FlattenListFlatMap 4558 4048 -11.2% 1.13x (?)

Code size: -Osize

Regression OLD NEW DELTA RATIO
CxxVectorSum.o 8512 8618 +1.2% 0.99x

Performance (x86_64): -Onone

Improvement OLD NEW DELTA RATIO
CxxVecU32.sum.Swift.forInLoop 11293 7205 -36.2% 1.57x
CxxVecU32.sum.Swift.reduce 12536 9287 -25.9% 1.35x

Code size: -swiftlibs

@zoecarver
Copy link
Contributor

300x improvement, that's what I like to see! 😎

@egorzhdan egorzhdan merged commit 05e3c55 into main Oct 14, 2022
@egorzhdan egorzhdan deleted the egorzhdan/cxx-iterator-boxing branch October 14, 2022 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ interop Feature: Interoperability with C++
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants