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

[DO NOT MERGE][stdlib] Keep Dictionary storage at least 25% full when possible #26921

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

Conversation

lorentey
Copy link
Member

This is an experimental implementation of shrinking Dictionary storage after removals.

It isn't clear this is worth doing, and the bulk of the actual work towards landing it would be in benchmarking and adding test coverage, which is not at all done. However, the implementation is an interesting exercise in library evolution.

It's almost possible to make this feature deploy back to earlier OSes -- except Set/Dictionary doesn't record the reserved capacity in 5.0 & 5.1, and that arguably rules out shrinking. (We'd also have to make the minimum load factor part of the ABI, but that's less of an issue.)

rdar://problem/18114559

@lorentey
Copy link
Member Author

Just to see if we have adequate benchmarks for Dictionary removals:

@swift-ci benchmark

@lorentey
Copy link
Member Author

@swift-ci test

@swift-ci
Copy link
Collaborator

Performance: -O

Regression OLD NEW DELTA RATIO
DictionaryRemove 3150 7460 +136.8% 0.42x
DictionaryRemoveOfObjects 21700 27900 +28.6% 0.78x
ObjectiveCBridgeStubFromNSDateRef 3760 4190 +11.4% 0.90x (?)
 
Improvement OLD NEW DELTA RATIO
String.data.Medium 98 86 -12.2% 1.14x (?)
CharacterLiteralsLarge 108 97 -10.2% 1.11x (?)
String.data.LargeUnicode 106 96 -9.4% 1.10x (?)
CharacterLiteralsSmall 340 317 -6.8% 1.07x (?)

Code size: -O

Regression OLD NEW DELTA RATIO
DictionaryRemove.o 15985 16337 +2.2% 0.98x

Performance: -Osize

Regression OLD NEW DELTA RATIO
DictionaryRemove 5640 15100 +167.7% 0.37x
DictionaryRemoveOfObjects 29900 48800 +63.2% 0.61x
 
Improvement OLD NEW DELTA RATIO
ObjectiveCBridgeStubFromNSStringRef 207 175 -15.5% 1.18x (?)
ObjectiveCBridgeFromNSSetAnyObjectToString 78000 71500 -8.3% 1.09x (?)
NSStringConversion.UTF8 831 776 -6.6% 1.07x (?)

Code size: -Osize

Improvement OLD NEW DELTA RATIO
DictionaryRemove.o 10833 10657 -1.6% 1.02x

Performance: -Onone

Regression OLD NEW DELTA RATIO
DictionaryRemove 13230 24070 +81.9% 0.55x
DictionaryRemoveOfObjects 44400 69300 +56.1% 0.64x
 
Improvement OLD NEW DELTA RATIO
ArrayOfPOD 1146 1064 -7.2% 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

@lorentey
Copy link
Member Author

A 2x slowdown matches how insertion behaves when capacity was reserved vs when it wasn't.

Some of it is probably spent on runtime availability checks that we could perhaps eliminate.

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - a2e2f9b

@lorentey lorentey marked this pull request as draft April 9, 2020 22:45
@shahmishal
Copy link
Member

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

  • How to change the base branch: (Link)
  • More detail about the branch update: (Link)

@lorentey lorentey changed the base branch from master to main October 2, 2020 22:11
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

3 participants