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

Optimize sequence algorithms to eliminate bounds-checking #40716

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Saklad5
Copy link

@Saklad5 Saklad5 commented Dec 30, 2021

This pull request tweaks several algorithms in the standard library to improve performance optimization, mainly through eliminating extraneous bounds checks by replacing not-equal operators with less-than operators.

These improvements were brought to my attention by @karwa while discussing the performance roadmap, and have already been implemented in a limited form for WebURL.

However, I am having trouble running local benchmarks to confirm performance improvements. I asked for assistance on the Swift Forums over a week ago, but have yet to get a reply. I’m opening this pull request as a draft in the hopes that someone may help me with that.

Using less-than and greater-than operators instead of checking
equivalency allows the compiler to eliminate bounds-checking more
thoroughly.
@harlanhaskins
Copy link
Contributor

@swift-ci please benchmark

@swift-ci
Copy link
Contributor

swift-ci commented Jan 3, 2022

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
ArrayAppendGenericStructs 1370 2450 +78.8% 0.56x (?)
ObjectiveCBridgeStubFromNSDate 6280 7130 +13.5% 0.88x (?)
ObjectiveCBridgeFromNSSetAnyObjectForced 5220 5700 +9.2% 0.92x (?)
StringBuilder 305 331 +8.5% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
ObjectiveCBridgeStubFromNSDateRef 5030 4200 -16.5% 1.20x (?)
DataAppendDataSmallToSmall 3960 3660 -7.6% 1.08x (?)
SortStringsUnicode 3210 2975 -7.3% 1.08x (?)
EqualSubstringSubstring 42 39 -7.1% 1.08x (?)
EqualStringSubstring 42 39 -7.1% 1.08x (?)
EqualSubstringString 42 39 -7.1% 1.08x (?)
EqualSubstringSubstringGenericEquatable 43 40 -7.0% 1.07x
LessSubstringSubstringGenericComparable 43 40 -7.0% 1.07x (?)

Code size: -O

Performance (x86_64): -Osize

Regression OLD NEW DELTA RATIO
StringBuilder 304 330 +8.6% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
ArrayAppendOptionals 2480 1350 -45.6% 1.84x (?)
LessSubstringSubstring 43 40 -7.0% 1.07x (?)
EqualSubstringSubstring 43 40 -7.0% 1.07x (?)
EqualStringSubstring 43 40 -7.0% 1.07x (?)
EqualSubstringSubstringGenericEquatable 43 40 -7.0% 1.07x (?)
EqualSubstringString 43 40 -7.0% 1.07x (?)
LessSubstringSubstringGenericComparable 43 40 -7.0% 1.07x (?)
SortStringsUnicode 3185 2965 -6.9% 1.07x (?)

Code size: -Osize

Performance (x86_64): -Onone

Regression OLD NEW DELTA RATIO
StringToDataMedium 7450 8150 +9.4% 0.91x (?)
 
Improvement OLD NEW DELTA RATIO
Breadcrumbs.MutatedIdxToUTF16.ASCII 13 12 -7.7% 1.08x (?)

Code size: -swiftlibs

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

@Saklad5
Copy link
Author

Saklad5 commented Jan 7, 2022

Could someone give me some guidance on running the benchmark correctly on my own machine? I want to use git bisect to work out which changes are causing regressions.

For that matter, I notice that all but one of the changes (an improvement) are flagged with (?). Does that mean they should be ignored, and that the changes so far have no meaningful regression in practice?

@harlanhaskins
Copy link
Contributor

I've added @atrick as a reviewer who should be able to shed some light on this. There's also this document which I am not sure if you've seen yet (if so, please ignore!) https://github.com/apple/swift/tree/main/benchmark

@Saklad5
Copy link
Author

Saklad5 commented Jan 7, 2022

I've added @atrick as a reviewer who should be able to shed some light on this. There's also this document which I am not sure if you've seen yet (if so, please ignore!) https://github.com/apple/swift/tree/main/benchmark

I read it from start to finish repeatedly before bothering anyone, in the hopes of figuring this out myself. It was very helpful in most regards, but it doesn’t explain what I’m seeing here. I even glanced at the Python scripts to see if there were any obvious causes, but nothing immediately jumps out and at that point it’s preferable for the solution to be discussed publicly so people can find it going forward.

@eeckstein
Copy link
Contributor

eeckstein commented Jan 10, 2022

I don't think there is a real regression in these results. The ? indicates that the result is noisy.

Unfortunately the document is a bit outdated. The easiest way to compile+run the benchmarks locally is to use cmake (as described in the document) and copy the built swift libraries to the benchmark build dir, e.g:

$ cmake ../../swift/benchmark -G Ninja -DSWIFT_EXEC=$swiftexec
$ ninja swift-benchmark-macosx-x86_64
$ cd lib/swift
$ cp -a "$swiftdir/lib/swift/macosx" ./

Compile two versions of the benchmarks: a baseline and the thing what you want to test. Then use https://github.com/apple/swift/blob/main/benchmark/scripts/run_smoke_bench to run the benchmarks, e.g.

$ run_smoke_bench -num-reruns 4 -skip-check-added baseline_build_dir my_feature_build_dir

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

4 participants