Skip to content

[stdlib] Set, Dictionary: Flatten switch statements #19611

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[stdlib] Set, Dictionary: Flatten switch statements #19611

wants to merge 3 commits into from

Conversation

@lorentey
Copy link
Member

@lorentey lorentey commented Sep 28, 2018

The optimizer needs to work extra hard to process nested switch statements. Flatten them out to simplify optimization and to hopefully speed things up a little, especially around opaque stuff.

@lorentey
Copy link
Member Author

lorentey commented Sep 28, 2018

@swift-ci please test

@lorentey
Copy link
Member Author

lorentey commented Sep 28, 2018

@swift-ci please smoke benchmark

@swift-ci

This comment has been minimized.

Sign in to view
@lorentey
Copy link
Member Author

lorentey commented Sep 28, 2018

Boo. I'll revert the read-only parts then. This may still be useful for mutations, though.

Do we get any compilation benefit, though?

@swift-ci please smoke test compiler performance

@lorentey
Copy link
Member Author

lorentey commented Sep 28, 2018

Hm, actually, this seems to only slow down the _modify accessor of the Values view. That's an easy fix.

I'll run some local benchmarks to see a bit more detail.

@airspeedswift
Copy link
Member

airspeedswift commented Sep 28, 2018

Meanwhile I'm going to see if rebasing #18953 on top of this helps with that PR's regressions.

@swift-ci
Copy link
Contributor

swift-ci commented Sep 28, 2018

Build comment file:

Summary for master smoketest

Unexpected test results, excluded stats for Kingfisher, ReactiveCocoa

No regressions above thresholds

Debug

debug brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 167,891,521,919 168,208,219,458 316,697,539 0.19%
LLVM.NumLLVMBytesOutput 9,519,976 9,519,976 0 0.0%
time.swift-driver.wall 20.9s 20.7s -197.0ms -0.94%

debug detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 5,796 5,796 0 0.0%
AST.NumLoadedModules 1,281 1,281 0 0.0%
AST.NumTotalClangImportedEntities 17,924 17,924 0 0.0%
AST.NumUsedConformances 1,346 1,346 0 0.0%
IRModule.NumIRBasicBlocks 32,510 32,510 0 0.0%
IRModule.NumIRFunctions 16,732 16,732 0 0.0%
IRModule.NumIRGlobals 14,495 14,495 0 0.0%
IRModule.NumIRInsts 445,415 445,415 0 0.0%
IRModule.NumIRValueSymbols 29,947 29,947 0 0.0%
LLVM.NumLLVMBytesOutput 9,519,976 9,519,976 0 0.0%
SILModule.NumSILGenFunctions 7,845 7,845 0 0.0%
SILModule.NumSILOptFunctions 10,598 10,598 0 0.0%
Sema.NumConformancesDeserialized 21,895 21,895 0 0.0%
Sema.NumConstraintScopes 70,886 70,886 0 0.0%
Sema.NumDeclsDeserialized 248,991 248,991 0 0.0%
Sema.NumDeclsValidated 16,273 16,273 0 0.0%
Sema.NumFunctionsTypechecked 7,006 7,006 0 0.0%
Sema.NumGenericSignatureBuilders 7,425 7,425 0 0.0%
Sema.NumLazyGenericEnvironments 56,442 56,442 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 2,346 2,346 0 0.0%
Sema.NumLazyIterableDeclContexts 40,950 40,950 0 0.0%
Sema.NumTypesDeserialized 101,475 101,475 0 0.0%
Sema.NumTypesValidated 13,937 13,937 0 0.0%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 221,687,038,523 221,866,213,070 179,174,547 0.08%
LLVM.NumLLVMBytesOutput 10,569,844 10,571,936 2,092 0.02%
time.swift-driver.wall 36.8s 36.9s 111.2ms 0.3%

release detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 1,383 1,383 0 0.0%
AST.NumLoadedModules 100 100 0 0.0%
AST.NumTotalClangImportedEntities 4,761 4,761 0 0.0%
AST.NumUsedConformances 1,348 1,348 0 0.0%
IRModule.NumIRBasicBlocks 35,129 35,134 5 0.01%
IRModule.NumIRFunctions 15,060 15,062 2 0.01%
IRModule.NumIRGlobals 13,858 13,858 0 0.0%
IRModule.NumIRInsts 336,377 336,436 59 0.02%
IRModule.NumIRValueSymbols 27,854 27,856 2 0.01%
LLVM.NumLLVMBytesOutput 10,569,844 10,571,936 2,092 0.02%
SILModule.NumSILGenFunctions 6,078 6,078 0 0.0%
SILModule.NumSILOptFunctions 8,944 8,943 -1 -0.01%
Sema.NumConformancesDeserialized 14,791 14,805 14 0.09%
Sema.NumConstraintScopes 69,488 69,488 0 0.0%
Sema.NumDeclsDeserialized 51,999 52,018 19 0.04%
Sema.NumDeclsValidated 10,362 10,362 0 0.0%
Sema.NumFunctionsTypechecked 4,360 4,360 0 0.0%
Sema.NumGenericSignatureBuilders 2,076 2,076 0 0.0%
Sema.NumLazyGenericEnvironments 11,754 11,755 1 0.01%
Sema.NumLazyGenericEnvironmentsLoaded 201 201 0 0.0%
Sema.NumLazyIterableDeclContexts 5,965 5,963 -2 -0.03%
Sema.NumTypesDeserialized 28,354 28,356 2 0.01%
Sema.NumTypesValidated 7,103 7,103 0 0.0%

@lorentey
[stdlib] Set, Dictionary: Flatten switch statements
77bf535 
The optimizer dislikes nested switch statements; flatten them out to simplify optimization and to hopefully speed things up a little.
@lorentey lorentey force-pushed the dont-switch-that-deep branch from 9b2d653 to 77bf535 Compare October 1, 2018 15:43
@lorentey
Copy link
Member Author

lorentey commented Oct 1, 2018

@swift-ci please smoke benchmark

@swift-ci

This comment has been minimized.

Sign in to view
@lorentey
Copy link
Member Author

lorentey commented Oct 1, 2018

@swift-ci please smoke benchmark

@lorentey
Copy link
Member Author

lorentey commented Oct 1, 2018

@swift-ci please smoke test compiler performance

@lorentey
Copy link
Member Author

lorentey commented Oct 1, 2018

The swapAt slowdown was due to a bug in my uniqueness checks; it should be fixed now.

@swift-ci
Copy link
Contributor

swift-ci commented Oct 1, 2018

Build comment file:

Performance: -O

TEST OLD NEW DELTA RATIO
Improvement
IterateData 1788 1613 -9.8% 1.11x

Code size: -O

TEST OLD NEW DELTA RATIO
Regression
DictionaryCompactMapValues.o 26210 27834 +6.2% 0.94x
Improvement
DictionarySwap.o 31502 29814 -5.4% 1.06x
DictionaryRemove.o 18158 17910 -1.4% 1.01x

Performance: -Osize

TEST OLD NEW DELTA RATIO
Improvement
StringWalk 1977 1680 -15.0% 1.18x
IterateData 1807 1581 -12.5% 1.14x
Array2D 13194 11998 -9.1% 1.10x
StringHashing_ascii 35 32 -8.6% 1.09x
Hanoi 2437 2246 -7.8% 1.09x
Calculator 605 561 -7.3% 1.08x
MapReduceAnyCollection 441 409 -7.3% 1.08x

Code size: -Osize

TEST OLD NEW DELTA RATIO
Regression
DictionaryCompactMapValues.o 23594 24722 +4.8% 0.95x
Improvement
DictionarySwap.o 27331 26347 -3.6% 1.04x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Improvement
MapReduceLazyCollection 30788 22355 -27.4% 1.38x
MapReduceLazyCollectionShort 43718 33117 -24.2% 1.32x
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

@swift-ci
Copy link
Contributor

swift-ci commented Oct 1, 2018

Build comment file:

Summary for master smoketest

Unexpected test results, excluded stats for Kingfisher, ReactiveCocoa

No regressions above thresholds

Debug

debug brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 181,389,040,359 181,374,157,252 -14,883,107 -0.01%
LLVM.NumLLVMBytesOutput 9,536,244 9,536,244 0 0.0%
time.swift-driver.wall 23.3s 23.4s 82.6ms 0.35%

debug detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 4,559 4,559 0 0.0%
AST.NumLoadedModules 1,510 1,510 0 0.0%
AST.NumTotalClangImportedEntities 17,945 17,945 0 0.0%
AST.NumUsedConformances 1,352 1,352 0 0.0%
IRModule.NumIRBasicBlocks 32,531 32,531 0 0.0%
IRModule.NumIRFunctions 16,669 16,669 0 0.0%
IRModule.NumIRGlobals 14,577 14,577 0 0.0%
IRModule.NumIRInsts 442,784 442,784 0 0.0%
IRModule.NumIRValueSymbols 29,936 29,936 0 0.0%
LLVM.NumLLVMBytesOutput 9,536,244 9,536,244 0 0.0%
SILModule.NumSILGenFunctions 7,843 7,843 0 0.0%
SILModule.NumSILOptFunctions 10,890 10,890 0 0.0%
Sema.NumConformancesDeserialized 23,406 23,406 0 0.0%
Sema.NumConstraintScopes 70,308 70,308 0 0.0%
Sema.NumDeclsDeserialized 230,163 230,163 0 0.0%
Sema.NumDeclsValidated 16,641 16,641 0 0.0%
Sema.NumFunctionsTypechecked 6,090 6,090 0 0.0%
Sema.NumGenericSignatureBuilders 7,344 7,344 0 0.0%
Sema.NumLazyGenericEnvironments 52,494 52,494 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 2,384 2,384 0 0.0%
Sema.NumLazyIterableDeclContexts 43,951 43,951 0 0.0%
Sema.NumTypesDeserialized 100,642 100,642 0 0.0%
Sema.NumTypesValidated 14,185 14,185 0 0.0%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (3)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 221,617,538,233 221,605,443,672 -12,094,561 -0.01%
LLVM.NumLLVMBytesOutput 10,561,092 10,563,176 2,084 0.02%
time.swift-driver.wall 41.0s 41.0s 27.3ms 0.07%

release detailed

Regressed (0)
name old new delta delta_pct
Improved (0)
name old new delta delta_pct
Unchanged (delta < 1.0% or delta < 100.0ms) (23)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 1,111 1,111 0 0.0%
AST.NumLoadedModules 100 100 0 0.0%
AST.NumTotalClangImportedEntities 4,731 4,731 0 0.0%
AST.NumUsedConformances 1,354 1,354 0 0.0%
IRModule.NumIRBasicBlocks 35,017 35,022 5 0.01%
IRModule.NumIRFunctions 15,022 15,024 2 0.01%
IRModule.NumIRGlobals 13,938 13,938 0 0.0%
IRModule.NumIRInsts 334,355 334,414 59 0.02%
IRModule.NumIRValueSymbols 27,866 27,868 2 0.01%
LLVM.NumLLVMBytesOutput 10,561,092 10,563,176 2,084 0.02%
SILModule.NumSILGenFunctions 6,076 6,076 0 0.0%
SILModule.NumSILOptFunctions 9,094 9,095 1 0.01%
Sema.NumConformancesDeserialized 14,940 14,951 11 0.07%
Sema.NumConstraintScopes 69,604 69,604 0 0.0%
Sema.NumDeclsDeserialized 45,638 45,638 0 0.0%
Sema.NumDeclsValidated 10,362 10,362 0 0.0%
Sema.NumFunctionsTypechecked 4,162 4,162 0 0.0%
Sema.NumGenericSignatureBuilders 2,044 2,044 0 0.0%
Sema.NumLazyGenericEnvironments 10,022 10,022 0 0.0%
Sema.NumLazyGenericEnvironmentsLoaded 201 201 0 0.0%
Sema.NumLazyIterableDeclContexts 5,860 5,860 0 0.0%
Sema.NumTypesDeserialized 26,845 26,837 -8 -0.03%
Sema.NumTypesValidated 7,103 7,103 0 0.0%

This was referenced Oct 3, 2018
Closed
Merged
@airspeedswift
Copy link
Member

airspeedswift commented Nov 28, 2018

This got superseded, right?

@lorentey
Copy link
Member Author

lorentey commented Nov 28, 2018

@airspeedswift Yes; a simplified variant landed, and then #19688 switched Set/Dictionary from using enums to Bridge Objects anyway. Closing.

@lorentey lorentey closed this Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@airspeedswift airspeedswift Awaiting requested review from airspeedswift

@gottesmm gottesmm Awaiting requested review from gottesmm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lorentey @swift-ci @airspeedswift