Skip to content

Avoid CFAllocator lookup overhead in String bridging #24000

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
Apr 16, 2019

Conversation

Catfish-Man
Copy link
Contributor

@Catfish-Man Catfish-Man commented Apr 12, 2019

Fixes rdar://problem/49925943

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@swift-ci
Copy link
Contributor

Build failed before running benchmark.

@Catfish-Man Catfish-Man force-pushed the thread-specific-doh branch from 9f51798 to 7587ab7 Compare April 13, 2019 00:13
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@swift-ci
Copy link
Contributor

Build failed before running benchmark.

@Catfish-Man
Copy link
Contributor Author

Yah. I should build it locally before running benchmarks apparently though :D

@Catfish-Man Catfish-Man force-pushed the thread-specific-doh branch from 7587ab7 to 452efd6 Compare April 15, 2019 17:39
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke benchmark

@swift-ci
Copy link
Contributor

Performance: -O

TEST OLD NEW DELTA RATIO
Regression
FlattenListLoop 3979 4336 +9.0% 0.92x
RemoveWhereMoveInts 34 37 +8.8% 0.92x (?)
Array2D 6928 7520 +8.5% 0.92x (?)
RemoveWhereSwapInts 60 65 +8.3% 0.92x
MapReduce 368 397 +7.9% 0.93x (?)
MapReduceAnyCollection 369 398 +7.9% 0.93x
FlattenListFlatMap 6073 6550 +7.9% 0.93x (?)
Improvement
ObjectiveCBridgeToNSString 414 362 -12.6% 1.14x (?)
ObjectiveCBridgeFromNSString 1080 1005 -6.9% 1.07x (?)
ObjectiveCBridgeStubFromNSString 885 826 -6.7% 1.07x (?)

Performance: -Osize

TEST OLD NEW DELTA RATIO
Regression
FlattenListLoop 4068 4431 +8.9% 0.92x (?)
RemoveWhereMoveInts 34 37 +8.8% 0.92x
Array2D 6912 7520 +8.8% 0.92x (?)
RemoveWhereSwapInts 62 67 +8.1% 0.93x
Improvement
ObjectiveCBridgeToNSString 417 365 -12.5% 1.14x (?)
EqualSubstringString 43 40 -7.0% 1.07x (?)
StringAdder 422 394 -6.6% 1.07x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Improvement
ObjectiveCBridgeToNSString 500 448 -10.4% 1.12x (?)
ArrayOfPOD 854 777 -9.0% 1.10x (?)
EqualStringSubstring 49 45 -8.2% 1.09x (?)
ObjectiveCBridgeStubFromNSString 923 859 -6.9% 1.07x (?)
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

@Catfish-Man
Copy link
Contributor Author

All the regressed tests have nothing to do with the code that changed, so I think this is a win

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

@Catfish-Man Catfish-Man changed the title Avoid TSD overhead in String bridging Avoid CFAllocator lookup overhead in String bridging Apr 15, 2019
@Catfish-Man Catfish-Man self-assigned this Apr 15, 2019
@Catfish-Man Catfish-Man requested a review from milseman April 15, 2019 18:32
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Member

@milseman milseman left a comment

Choose a reason for hiding this comment

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

Please add an assertion in case someone tries to call it with a real allocator and intended it. Otherwise, LGTM

@Catfish-Man Catfish-Man force-pushed the thread-specific-doh branch from 452efd6 to 4366a1a Compare April 15, 2019 22:11
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

@Catfish-Man Catfish-Man force-pushed the thread-specific-doh branch from 4366a1a to 584fbfc Compare April 15, 2019 22:28
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

2 similar comments
@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

@Catfish-Man
Copy link
Contributor Author

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 388c21a into swiftlang:master Apr 16, 2019
@Catfish-Man Catfish-Man deleted the thread-specific-doh branch April 16, 2019 01:17
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.

4 participants