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.

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

[stdlib] Remove workaround for sort optimization #40081

Merged
merged 1 commit into from Jan 12, 2022

Conversation

HassanElDesouky
Copy link
Contributor

Remove workaround for sort optimization. Cherry picked from #40056.

Resolves SR-14750.

cc @lorentey, @glessard, @natecook1000

@glessard
Copy link
Contributor

glessard commented Nov 8, 2021

@swift-ci please test macOS platform

@HassanElDesouky
Copy link
Contributor Author

@glessard can you re-run the tests again. There was a merge conflict that I just resolved.

@glessard
Copy link
Contributor

glessard commented Nov 9, 2021

@swift-ci please test macOS platform

@glessard
Copy link
Contributor

glessard commented Nov 9, 2021

@swift-ci please test Linux platform

@glessard
Copy link
Contributor

@natecook1000 @lorentey I'm not sure what to make of the passed tests: it seems like this should register as an ABI change. Thoughts?

@glessard
Copy link
Contributor

If we don't merge this, an alternative is to at least add the JIRA bug number: #40138

@glessard
Copy link
Contributor

@HassanElDesouky The addition of the SR bug number was merged to main, and we have a merge conflict again. It's preferable to solve the conflict by rebasing rather than merging (it makes for a more understandable history.) Thanks!

@HassanElDesouky
Copy link
Contributor Author

@glessard Thank you for adding the SR bug number. I just rebased the branch and fixed the merge conflict.

@glessard
Copy link
Contributor

@swift-ci Please Build Toolchain Linux Platform

@swift-ci
Copy link
Collaborator

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - 4d375c9d6e85be96e9d4a25f7ab6c683bf9114a3

Install command
tar zxf swift-PR-40081-731-ubuntu16.04.tar.gz
More info

@HassanElDesouky
Copy link
Contributor Author

@glessard any update whether we're going to land this or not?

@lorentey
Copy link
Member

lorentey commented Nov 17, 2021

@LucianoPAlmeida's note on ABI stability isn't resolved yet! Sadly I believe we'll need to preserve the Bool return value as these functions are ABI. (Otherwise we would introduce binary compatibility issues.)

@glessard
Copy link
Contributor

glessard commented Dec 8, 2021

We could perhaps remove the workarounds without changing the ABI: simply return true, and add the @discardableResult attribute. If nothing else, it should be a minor win in code size.

@HassanElDesouky
Copy link
Contributor Author

@glessard Nice idea! I'm just traveling now so I'll do this solution once I get back.

stdlib/public/core/Sort.swift Outdated Show resolved Hide resolved
stdlib/public/core/Sort.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@glessard glessard left a comment

Choose a reason for hiding this comment

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

Looks good. I'll trigger a tests run.

@glessard
Copy link
Contributor

@swift-ci please test

@xwu
Copy link
Collaborator

xwu commented Jan 11, 2022

@swift-ci please benchmark

@swift-ci
Copy link
Collaborator

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
FlattenListFlatMap 3876 4233 +9.2% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
StringBuilder 331 305 -7.9% 1.09x (?)

Code size: -O

Improvement OLD NEW DELTA RATIO
Phonebook.o 9947 9659 -2.9% 1.03x
SortLettersInPlace.o 8896 8720 -2.0% 1.02x
RGBHistogram.o 21834 21450 -1.8% 1.02x
SortStrings.o 27523 27043 -1.7% 1.02x
SortLargeExistentials.o 20100 19860 -1.2% 1.01x

Performance (x86_64): -Osize

Improvement OLD NEW DELTA RATIO
StringBuilderSmallReservingCapacity 340 314 -7.6% 1.08x (?)
StringBuilderWithLongSubstring 1620 1510 -6.8% 1.07x (?)

Code size: -Osize

Improvement OLD NEW DELTA RATIO
SortLettersInPlace.o 8249 8033 -2.6% 1.03x
Phonebook.o 9201 8975 -2.5% 1.03x
RGBHistogram.o 20085 19692 -2.0% 1.02x
SortIntPyramids.o 8841 8669 -1.9% 1.02x
SortStrings.o 26996 26562 -1.6% 1.02x
StaticArray.o 12005 11833 -1.4% 1.01x
SortLargeExistentials.o 20824 20552 -1.3% 1.01x
NibbleSort.o 14456 14284 -1.2% 1.01x

Performance (x86_64): -Onone

Improvement OLD NEW DELTA RATIO
CharIndexing_russian_unicodeScalars_Backwards 160320 148640 -7.3% 1.08x (?)

Code size: -swiftlibs

Improvement OLD NEW DELTA RATIO
libswiftSwiftOnoneSupport.dylib 163840 147456 -10.0% 1.11x
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 Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

@LucianoPAlmeida
Copy link
Collaborator

@swift-ci Please test Windows Platform

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 6316555

@glessard
Copy link
Contributor

The test failure was in a test related to the move-only work. Seems to fail for a few different PRs.

@glessard
Copy link
Contributor

#40806 should fix the test failure above.

@glessard
Copy link
Contributor

@swift-ci please test macOS platform

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - 6316555

@xwu
Copy link
Collaborator

xwu commented Jan 12, 2022

@swift-ci please test macOS platform

@HassanElDesouky
Copy link
Contributor Author

Tests passed 🎉
@glessard

@glessard glessard merged commit a1e2ebb into apple:main Jan 12, 2022
@glessard
Copy link
Contributor

Thanks @HassanElDesouky for working on this!

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.

None yet

6 participants