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

Final: Implementation for SE-0228: Fix ExpressibleByStringInterpolation #20214

Merged
merged 14 commits into from Nov 3, 2018

Conversation

Projects
None yet
4 participants
@brentdax
Collaborator

brentdax commented Nov 1, 2018

This PR implements a new string interpolation API with a different, finalized ABI from what we're currently shipping.

This is #19963 with a clean revision history and a final pass for style. The only functional changes are that literalCapacity is now calculated as the UTF-8 length of the string and the CustomString type I accidentally left in the benchmarks has been removed. I'll be running the last set of CI checks here and then merging.

I expect the final checks to still show some benchmark regressions. A few are known issues with non-ABI-affecting fixes planned. The others we think will be easier to fix on master, where we can coordinate changes more easily.

Implements SE-0228. Resolves SR-1260, SR-2303, and SR-3969. Resolves rdar://43621912.

cc @milseman @ravikandhadai

brentdax added some commits Nov 1, 2018

[CodeCompletion] Restrict ancestor search to brace
This change allows ExprParentFinder to restrict certain searches for parents to just AST nodes within the nearest surrounding BraceStmt. In the string interpolation rework, BraceStmts can appear in new places in the AST; this keeps code completion from looking at irrelevant context.

NFC in this commit, but keeps code completion from crashing once TapExpr is introduced.
[AST] Introduce TapExpr
TapExpr allows a block of code to to be inserted between two expressions, accessing and potentially mutating the result of its subexpression before giving it to its parent expression. It’s roughly equivalent to this function:

  func _tap<T>(_ value: T, do body: (inout T) throws -> Void) rethrows -> T {
    var copy = value
    try body(&copy)
    return copy
  }

Except that it doesn’t use a closure, so no variables are captured and no call frame is (even notionally) added.

This commit does not include tests because nothing in it actually uses TapExpr yet. It will be used by string interpolation.
SE-0228: Fix ExpressibleByStringInterpolation
This is the bulk of the implementation of the string interpolation rework. It includes a redesigned AST node, new parsing logic, new constraints and post-typechecking code generation, and new standard library types and members.
[Sema] Rip out typeCheckExpressionShallow()
With new string interpolation in place, it is no longer used by anything in the compiler.
[Sema] Diagnose invalid StringInterpolationProtocols
StringInterpolationProtocol informally requires conforming types to provide at least one method with the base name “appendInterpolation” with no (or a discardable) return value and visibility at least as broad as the conforming type’s. This change diagnoses an error when a conforming type does not have a method that meets those criteria.
[Stdlib] Fix map(String.init) source break
Some users, including some in the source compatibility suite, accidentally used init(stringInterpolationSegment:) by writing code like `map(String.init)`. Now that these intializers have been removed, the remaining initializers often end up tying during overload resolution. This change adds several overloads of `String.init(describing:)` which will break these ties in cases where the compiler previously selected `String.init(stringInterpolationSegment:)`.
[Sema] Make callWitness() take non-mutable arrays
It doesn’t actually need to mutate them.
[Stdlib] Improve floating-point interpolation performance
This change avoids constructing a String when interpolating a Float, Double, or Float80. Instead, we write the characters to a fixed-size buffer and then append them directly to the string’s storage.

This seems to improve performance for all three types, but especially for Double and Float80, which cannot always fit into a small string when stringified.
[NameLookup] Improve MemberLookupTable invalidation
In rare cases usually involving generated code, an overload added by an extension in the middle of a file would not be visible below it if the type had lazy members and the same base name had already been referenced above the extension. This change essentially dirties a type’s member lookup table whenever an extension is added to it, ensuring the entries in it will be updated.

This change also includes some debugging improvements for NameLookup.
[SILOptimizer] XFAIL dead object removal failure
The DeadObjectRemoval pass in SILOptimizer does not currently remove reworked string interpolations as well as the old design because their effects cannot be described by @_effects(readonly). That causes a test failure on Linux. This change temporarily silences that test. The SILOptimizer issue has been filed as SR-9008.
[Parser] Parse empty interpolations
Previously, the parser had an odd asymmetry which caused the same function to accept foo(), but reject “\()”. This change fixes the issue.

Already tested by test/Parse/try.swift, which uses this construct in one of its throwing interpolation tests.
@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 1, 2018

@swift-ci please test

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 1, 2018

@swift-ci please test source compatibility

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 1, 2018

@swift-ci please smoke test compiler performance

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 1, 2018

@swift-ci please benchmark

@swift-ci

This comment has been minimized.

Contributor

swift-ci commented Nov 1, 2018

Build comment file:

Performance: -O

TEST OLD NEW DELTA RATIO
Regression
StringInterpolation 8687 11912 +37.1% 0.73x
RandomDoubleLCG 910 1056 +16.0% 0.86x
SubstringComparable 13 15 +15.4% 0.87x
StringWordBuilderReservingCapacity 1146 1260 +9.9% 0.91x (?)
FloatingPointPrinting_Float_description_small 5168 5609 +8.5% 0.92x
StringWordBuilder 1737 1881 +8.3% 0.92x (?)
Improvement
StringInterpolationManySmallSegments 17867 10110 -43.4% 1.77x
StringInterpolationSmall 4029 2628 -34.8% 1.53x
OpenClose 80 72 -10.0% 1.11x
StringHashing_abnormal 1419 1308 -7.8% 1.08x (?)
StringBuilderSmallReservingCapacity 500 462 -7.6% 1.08x
StringBuilder 490 454 -7.3% 1.08x
CStringLongAscii 3540 3282 -7.3% 1.08x
StringComparison_abnormal 834 774 -7.2% 1.08x
StringAdder 548 510 -6.9% 1.07x

Code size: -O

TEST OLD NEW DELTA RATIO
Regression
StringBuilder.o 11679 12295 +5.3% 0.95x
DeadArray.o 1872 1940 +3.6% 0.96x
ErrorHandling.o 2691 2723 +1.2% 0.99x
Improvement
Fibonacci.o 1936 1596 -17.6% 1.21x
MonteCarloPi.o 1920 1593 -17.0% 1.21x
SevenBoom.o 1950 1626 -16.6% 1.20x
ByteSwap.o 1960 1636 -16.5% 1.20x
NSDictionaryCastToSwift.o 1984 1657 -16.5% 1.20x
RangeIteration.o 1976 1652 -16.4% 1.20x
StringInterpolation.o 11382 9526 -16.3% 1.19x
PointerArithmetics.o 2070 1743 -15.8% 1.19x
BitCount.o 2200 1860 -15.5% 1.18x
ProtocolDispatch2.o 2224 1888 -15.1% 1.18x
Ackermann.o 2160 1836 -15.0% 1.18x
LinkedList.o 2263 1926 -14.9% 1.17x
XorLoop.o 2280 1953 -14.3% 1.17x
Memset.o 2392 2068 -13.5% 1.16x
ChainedFilterMap.o 3492 3053 -12.6% 1.14x
ObjectAllocation.o 4635 4059 -12.4% 1.14x
ArrayLiteral.o 3531 3095 -12.3% 1.14x
StrComplexWalk.o 3456 3049 -11.8% 1.13x
StackPromo.o 2583 2279 -11.8% 1.13x
Integrate.o 2828 2504 -11.5% 1.13x
MonteCarloE.o 3640 3236 -11.1% 1.12x
Exclusivity.o 4083 3635 -11.0% 1.12x
FloatingPointPrinting.o 6583 5911 -10.2% 1.11x
OpenClose.o 3823 3488 -8.8% 1.10x
DictionaryBridge.o 3634 3326 -8.5% 1.09x
Calculator.o 3336 3060 -8.3% 1.09x
PopFrontGeneric.o 5052 4644 -8.1% 1.09x
RandomValues.o 4135 3808 -7.9% 1.09x
PopFront.o 5577 5141 -7.8% 1.08x
SequenceAlgos.o 23075 21283 -7.8% 1.08x
PrefixWhile.o 24062 22222 -7.6% 1.08x
ArraySubscript.o 4264 3940 -7.6% 1.08x
RangeAssignment.o 5216 4820 -7.6% 1.08x
TwoSum.o 5960 5508 -7.6% 1.08x
DropWhile.o 23740 21948 -7.5% 1.08x
StringTests.o 10695 9911 -7.3% 1.08x
BinaryFloatingPointProperties.o 8039 7468 -7.1% 1.08x
RC4.o 5007 4667 -6.8% 1.07x
DropFirst.o 25228 23532 -6.7% 1.07x
DictionaryCopy.o 9120 8512 -6.7% 1.07x
MapReduce.o 24653 23053 -6.5% 1.07x
StrToInt.o 6675 6243 -6.5% 1.07x
DictionaryBridgeToObjC.o 6943 6495 -6.5% 1.07x
DropLast.o 25515 23915 -6.3% 1.07x
Suffix.o 26249 24617 -6.2% 1.07x
Prefix.o 24673 23201 -6.0% 1.06x
DictionarySwap.o 28502 26822 -5.9% 1.06x
LazyFilter.o 9801 9241 -5.7% 1.06x
NopDeinit.o 5907 5576 -5.6% 1.06x
HashQuadratic.o 5816 5492 -5.6% 1.06x
DictionaryKeysContains.o 12307 11675 -5.1% 1.05x
ObjectiveCBridging.o 45831 43479 -5.1% 1.05x
SortIntPyramids.o 9717 9237 -4.9% 1.05x
ObjectiveCBridgingStubs.o 9981 9501 -4.8% 1.05x
DictionaryOfAnyHashableStrings.o 10421 9925 -4.8% 1.05x
Queue.o 14587 13947 -4.4% 1.05x
Walsh.o 9464 9060 -4.3% 1.04x
RangeReplaceableCollectionPlusDefault.o 7620 7300 -4.2% 1.04x
Hash.o 29187 27971 -4.2% 1.04x
DictionarySubscriptDefault.o 29759 28543 -4.1% 1.04x
ObjectiveCNoBridgingStubs.o 8323 7999 -3.9% 1.04x
CString.o 8809 8473 -3.8% 1.04x
DictionaryRemove.o 15746 15154 -3.8% 1.04x
DictionaryCompactMapValues.o 22958 22110 -3.7% 1.04x
DictionaryGroup.o 17391 16767 -3.6% 1.04x
DictTest4.o 26702 25774 -3.5% 1.04x
SortLettersInPlace.o 12026 11654 -3.1% 1.03x
DictTest4Legacy.o 27864 27016 -3.0% 1.03x
NibbleSort.o 12260 11924 -2.7% 1.03x
DictTest2.o 20300 19788 -2.5% 1.03x
SetTests.o 59613 58189 -2.4% 1.02x
COWTree.o 14324 13988 -2.3% 1.02x
ReduceInto.o 52841 51769 -2.0% 1.02x
ArrayAppend.o 38982 38198 -2.0% 1.02x
LuhnAlgoLazy.o 17996 17660 -1.9% 1.02x
LuhnAlgoEager.o 17998 17662 -1.9% 1.02x
RGBHistogram.o 25221 24757 -1.8% 1.02x
Combos.o 18237 17917 -1.8% 1.02x
SortLargeExistentials.o 21838 21502 -1.5% 1.02x
DictTest.o 51659 50987 -1.3% 1.01x
RomanNumbers.o 15253 15061 -1.3% 1.01x
DriverUtils.o 188783 186511 -1.2% 1.01x
TestsUtils.o 24865 24577 -1.2% 1.01x
Substring.o 31401 31081 -1.0% 1.01x
StringRemoveDupes.o 33546 33210 -1.0% 1.01x

Performance: -Osize

TEST OLD NEW DELTA RATIO
Regression
StringInterpolation 8760 11491 +31.2% 0.76x
FloatingPointPrinting_Float_interpolated 37330 44845 +20.1% 0.83x
StringWordBuilderReservingCapacity 1146 1274 +11.2% 0.90x (?)
DropWhileAnyCollectionLazy 256 282 +10.2% 0.91x
StringWordBuilder 1758 1899 +8.0% 0.93x (?)
FloatingPointPrinting_Float_description_uniform 5018 5416 +7.9% 0.93x
Improvement
StringInterpolationManySmallSegments 16483 9919 -39.8% 1.66x
StringInterpolationSmall 4003 2597 -35.1% 1.54x
FloatingPointPrinting_Double_interpolated 61540 48128 -21.8% 1.28x
DropLastAnyCollection 65 60 -7.7% 1.08x
StringBuilder 489 452 -7.6% 1.08x
StringBuilderSmallReservingCapacity 499 462 -7.4% 1.08x
StringHashing_abnormal 1430 1328 -7.1% 1.08x
CStringLongAscii 3543 3294 -7.0% 1.08x

Code size: -Osize

TEST OLD NEW DELTA RATIO
Regression
DeadArray.o 1698 1906 +12.2% 0.89x
StringBuilder.o 11210 11810 +5.4% 0.95x
DictionaryOfAnyHashableStrings.o 9997 10413 +4.2% 0.96x
ErrorHandling.o 3030 3078 +1.6% 0.98x
DictTest3.o 23094 23430 +1.5% 0.99x
Improvement
StringInterpolation.o 9140 7620 -16.6% 1.20x
ObjectAllocation.o 4505 3934 -12.7% 1.15x
SequenceAlgos.o 25820 22732 -12.0% 1.14x
MonteCarloPi.o 1781 1578 -11.4% 1.13x
FloatingPointPrinting.o 5712 5072 -11.2% 1.13x
BitCount.o 1837 1634 -11.1% 1.12x
Fibonacci.o 1845 1642 -11.0% 1.12x
NSDictionaryCastToSwift.o 1877 1674 -10.8% 1.12x
DropWhile.o 23420 20972 -10.5% 1.12x
StringTests.o 8497 7638 -10.1% 1.11x
SevenBoom.o 2019 1816 -10.1% 1.11x
ByteSwap.o 1869 1682 -10.0% 1.11x
PrefixWhile.o 24446 22014 -9.9% 1.11x
MapReduce.o 22381 20237 -9.6% 1.11x
LinkedList.o 2124 1921 -9.6% 1.11x
DropFirst.o 23812 21572 -9.4% 1.10x
Prefix.o 23777 21585 -9.2% 1.10x
ChainedFilterMap.o 3492 3172 -9.2% 1.10x
ProtocolDispatch2.o 2103 1916 -8.9% 1.10x
Exclusivity.o 3815 3484 -8.7% 1.10x
RangeIteration.o 1861 1706 -8.3% 1.09x
PointerArithmetics.o 1971 1816 -7.9% 1.09x
DropLast.o 25539 23539 -7.8% 1.08x
Suffix.o 25937 23921 -7.8% 1.08x
ArrayLiteral.o 3160 2920 -7.6% 1.08x
Integrate.o 2705 2502 -7.5% 1.08x
MonteCarloE.o 3874 3586 -7.4% 1.08x
XorLoop.o 2090 1938 -7.3% 1.08x
Hash.o 22287 20735 -7.0% 1.07x
ObjectiveCBridging.o 44151 41143 -6.8% 1.07x
Memset.o 2132 1988 -6.8% 1.07x
DictionaryCopy.o 8377 7817 -6.7% 1.07x
Calculator.o 2957 2770 -6.3% 1.07x
StrComplexWalk.o 3573 3350 -6.2% 1.07x
BinaryFloatingPointProperties.o 7737 7257 -6.2% 1.07x
Ackermann.o 2085 1957 -6.1% 1.07x
PopFront.o 5214 4894 -6.1% 1.07x
DictionaryBridge.o 3671 3468 -5.5% 1.06x
LazyFilter.o 9137 8641 -5.4% 1.06x
DictionarySwap.o 25771 24395 -5.3% 1.06x
TwoSum.o 5597 5309 -5.1% 1.05x
DictionaryBridgeToObjC.o 6589 6269 -4.9% 1.05x
DictionarySubscriptDefault.o 26743 25479 -4.7% 1.05x
StackPromo.o 2437 2325 -4.6% 1.05x
DictionaryGroup.o 15959 15255 -4.4% 1.05x
PopFrontGeneric.o 4949 4733 -4.4% 1.05x
RandomValues.o 3620 3465 -4.3% 1.04x
Walsh.o 6322 6052 -4.3% 1.04x
RC4.o 4089 3929 -3.9% 1.04x
OpenClose.o 3691 3547 -3.9% 1.04x
DictionaryCompactMapValues.o 21470 20638 -3.9% 1.04x
RangeAssignment.o 5162 4962 -3.9% 1.04x
DictionaryRemove.o 14479 13919 -3.9% 1.04x
ArraySubscript.o 3986 3842 -3.6% 1.04x
HashQuadratic.o 5320 5128 -3.6% 1.04x
Queue.o 13291 12851 -3.3% 1.03x
SortIntPyramids.o 10026 9706 -3.2% 1.03x
SetTests.o 52485 50837 -3.1% 1.03x
NopDeinit.o 5621 5445 -3.1% 1.03x
ObjectiveCBridgingStubs.o 9101 8839 -2.9% 1.03x
DictionaryKeysContains.o 11715 11427 -2.5% 1.03x
DictTest4.o 21247 20735 -2.4% 1.02x
DictTest4Legacy.o 23033 22505 -2.3% 1.02x
WordCount.o 93562 91514 -2.2% 1.02x
DictTest2.o 16518 16166 -2.1% 1.02x
ReduceInto.o 46096 45168 -2.0% 1.02x
SortLettersInPlace.o 9995 9803 -1.9% 1.02x
StrToInt.o 6708 6580 -1.9% 1.02x
ArrayAppend.o 37886 37182 -1.9% 1.02x
ObjectiveCNoBridgingStubs.o 7951 7807 -1.8% 1.02x
CString.o 6322 6210 -1.8% 1.02x
RangeReplaceableCollectionPlusDefault.o 7077 6965 -1.6% 1.02x
RGBHistogram.o 23053 22749 -1.3% 1.01x
COWTree.o 13442 13298 -1.1% 1.01x
Combos.o 16581 16405 -1.1% 1.01x
DictTest.o 48901 48389 -1.0% 1.01x
DriverUtils.o 161659 159975 -1.0% 1.01x

Performance: -Onone

TEST OLD NEW DELTA RATIO
Regression
DictionaryBridgeToObjC_Access 1110 1347 +21.4% 0.82x (?)
StringEqualPointerComparison 3628 4114 +13.4% 0.88x
StringHasSuffixAscii 5114 5686 +11.2% 0.90x
StringHasPrefixAscii 5028 5543 +10.2% 0.91x
Improvement
StringInterpolationSmall 6642 3375 -49.2% 1.97x
StringInterpolationManySmallSegments 18627 11669 -37.4% 1.60x
FloatingPointPrinting_Double_interpolated 104653 70610 -32.5% 1.48x
FloatingPointPrinting_Float80_interpolated 136350 99866 -26.8% 1.37x
ArrayAppendStrings 10870 8767 -19.3% 1.24x
StringHashing_abnormal 1567 1407 -10.2% 1.11x

Code size: Swift libraries

TEST OLD NEW DELTA RATIO
Regression
libswiftSwiftReflectionTest.dylib 45056 53248 +18.2% 0.85x
Improvement
libswiftNetwork.dylib 163840 159744 -2.5% 1.03x
libswiftStdlibUnittest.dylib 409600 401408 -2.0% 1.02x
libswiftFoundation.dylib 1716224 1695744 -1.2% 1.01x
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

This comment has been minimized.

Contributor

swift-ci commented Nov 1, 2018

Build comment file:

Summary for master smoketest

Unexpected test results, excluded stats for Kingfisher, ReactiveCocoa, ReactiveSwift

Regressions found (see below)

Debug

debug brief

Regressed (0)
name old new delta delta_pct
Improved (1)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 80,405,282,767 79,497,964,936 -907,317,831 -1.13%
Unchanged (delta < 1.0% or delta < 100.0ms) (2)
name old new delta delta_pct
LLVM.NumLLVMBytesOutput 2,898,548 2,916,224 17,676 0.61%
time.swift-driver.wall 10.9s 10.9s 18.6ms 0.17%

debug detailed

Regressed (1)
name old new delta delta_pct
Sema.NumDeclsValidated 5,358 5,804 446 8.32% ⛔️
Improved (6)
name old new delta delta_pct
IRModule.NumIRInsts 133,974 129,201 -4,773 -3.56%
Sema.NumConstraintScopes 43,096 26,184 -16,912 -39.24%
Sema.NumDeclsDeserialized 97,820 96,602 -1,218 -1.25%
Sema.NumLazyGenericEnvironments 20,626 20,280 -346 -1.68%
Sema.NumLazyGenericEnvironmentsLoaded 432 425 -7 -1.62%
Sema.NumTypesDeserialized 39,207 38,737 -470 -1.2%
Unchanged (delta < 1.0% or delta < 100.0ms) (16)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 3,577 3,577 0 0.0%
AST.NumLoadedModules 654 654 0 0.0%
AST.NumTotalClangImportedEntities 13,508 13,505 -3 -0.02%
AST.NumUsedConformances 508 507 -1 -0.2%
IRModule.NumIRBasicBlocks 12,092 12,108 16 0.13%
IRModule.NumIRFunctions 5,240 5,262 22 0.42%
IRModule.NumIRGlobals 4,708 4,725 17 0.36%
IRModule.NumIRValueSymbols 9,677 9,716 39 0.4%
LLVM.NumLLVMBytesOutput 2,898,548 2,916,224 17,676 0.61%
SILModule.NumSILGenFunctions 2,482 2,499 17 0.68%
SILModule.NumSILOptFunctions 3,541 3,538 -3 -0.08%
Sema.NumConformancesDeserialized 8,942 8,876 -66 -0.74%
Sema.NumFunctionsTypechecked 3,274 3,274 0 0.0%
Sema.NumGenericSignatureBuilders 2,738 2,724 -14 -0.51%
Sema.NumLazyIterableDeclContexts 18,938 18,920 -18 -0.1%
Sema.NumTypesValidated 3,385 3,385 0 0.0%

Release

release brief

Regressed (0)
name old new delta delta_pct
Improved (2)
name old new delta delta_pct
Frontend.NumInstructionsExecuted 96,012,811,182 94,018,303,061 -1,994,508,121 -2.08%
time.swift-driver.wall 18.1s 17.7s -385.2ms -2.13%
Unchanged (delta < 1.0% or delta < 100.0ms) (1)
name old new delta delta_pct
LLVM.NumLLVMBytesOutput 3,076,556 3,063,552 -13,004 -0.42%

release detailed

Regressed (2)
name old new delta delta_pct
Sema.NumDeclsValidated 3,000 3,446 446 14.87% ⛔️
Sema.NumGenericSignatureBuilders 632 643 11 1.74% ⛔️
Improved (2)
name old new delta delta_pct
IRModule.NumIRInsts 116,067 112,020 -4,047 -3.49%
Sema.NumConstraintScopes 37,838 20,986 -16,852 -44.54%
Unchanged (delta < 1.0% or delta < 100.0ms) (19)
name old new delta delta_pct
AST.NumImportedExternalDefinitions 829 829 0 0.0%
AST.NumLoadedModules 65 65 0 0.0%
AST.NumTotalClangImportedEntities 3,124 3,124 0 0.0%
AST.NumUsedConformances 508 507 -1 -0.2%
IRModule.NumIRBasicBlocks 13,539 13,513 -26 -0.19%
IRModule.NumIRFunctions 4,221 4,205 -16 -0.38%
IRModule.NumIRGlobals 4,218 4,232 14 0.33%
IRModule.NumIRValueSymbols 8,318 8,315 -3 -0.04%
LLVM.NumLLVMBytesOutput 3,076,556 3,063,552 -13,004 -0.42%
SILModule.NumSILGenFunctions 1,959 1,965 6 0.31%
SILModule.NumSILOptFunctions 2,720 2,742 22 0.81%
Sema.NumConformancesDeserialized 5,676 5,720 44 0.78%
Sema.NumDeclsDeserialized 21,979 22,063 84 0.38%
Sema.NumFunctionsTypechecked 1,676 1,676 0 0.0%
Sema.NumLazyGenericEnvironments 4,473 4,467 -6 -0.13%
Sema.NumLazyGenericEnvironmentsLoaded 82 82 0 0.0%
Sema.NumLazyIterableDeclContexts 3,224 3,240 16 0.5%
Sema.NumTypesDeserialized 11,535 11,620 85 0.74%
Sema.NumTypesValidated 1,517 1,517 0 0.0%

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 1, 2018

Alamofire failed to compile Source/MultipartFormData.swift line 95. It's using a string interpolation in a weird way—in an initializer for a lazy property, interpolating the value of a non-lazy property—which is causing its temporary to become immutable for some reason.

Unfortunately, I haven't been able to reproduce it yet, even by downloading and compiling the entire project locally. The console indicates that Alamofire did fail in CI release mode too, though. Perhaps my setup isn't a precise enough reproduction of the CI environment.

I really wanted to get this in tonight, but I guess it'll have to wait for tomorrow. The other CI results don't concern me; the performance benchmarks are showing both more regressions and more improvements, so either the CI is being weird tonight or something recently landed (like BuiltinInt) that shifted performance around in complicated ways. Either way, it's not a good reason to not merge.

(If someone with a little more seniority than me wants to merge this despite the source compatibility failure, go ahead. I may be too conservative about this.)

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 2, 2018

@swift-ci please test source compatibility

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 2, 2018

Those CI "failures" are actually because two projects unexpectedly passed.

 Passed: 180
 Failed: 0
XFailed: 17
UPassed: 2
  Total: 199

Should land tomorrow.

[Sema] Fix batch-mode-only lazy var bug
The temporary variable used by string interpolation needs to be recontextualized when it’s inserted into a synthesized getter. Fixes a compilation failure in Alamofire.

I’ll probably follow up on this bug a bit more after merging.
@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 2, 2018

@swift-ci please test

@swift-ci

This comment was marked as resolved.

Contributor

swift-ci commented Nov 2, 2018

Build failed
Swift Test OS X Platform
Git Sha - d79f7ce

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 2, 2018

Ah, the famous "Please ignore this build failure" build failure...

@swift-ci

This comment was marked as resolved.

Contributor

swift-ci commented Nov 2, 2018

Build failed
Swift Test Linux Platform
Git Sha - d79f7ce

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 2, 2018

@swift-ci please test os x platform

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 3, 2018

Previous incarnations of this PR have been reviewed by @xedin, @milseman, @slavapestov, and @benrimmington.

Hold on to your butts...

@brentdax brentdax merged commit 9bd1a26 into apple:master Nov 3, 2018

4 checks passed

Swift Test Linux Platform 11431 tests run, 10315 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 57340 tests run, 2412 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details
@milseman

Just a drive by commentary of some things I should of caught in the review. Don't worry about changing this, I'm trying to incorporate this into the UTF-8 branch and wouldn't want churn here.

We can discuss the details over a whiteboard if you like

}
extension TextOutputStream {
public mutating func _lock() {}
public mutating func _unlock() {}
@inlinable
public mutating func _writeASCII(_ buffer: UnsafeBufferPointer<UInt8>) {

This comment has been minimized.

@milseman

milseman Nov 4, 2018

Contributor

Does this have to be public? Since _fromASCII is not inlinable, then this probably shouldn't be either.

This comment has been minimized.

@milseman

milseman Nov 4, 2018

Contributor

Err crap, is it because we can't have non-public requirements on public protocols?

BTW, I think that floating point types' TextOutputStreamable.write<Target> should have a @_specialize(where Target == String for the 99% case, which is also the perf-sensitive case. That should give us better perf, and since that's the only call site for this, we can form an immortal String there.

edit: Perhaps also DefaultStringInterpolation now...

Longer-term, we don't really care for a buffer-returning function for _float${Bits}ToString, we just want to write directly into the allocated extra capacity. Same for Int->String, etc. CC @stephentyrone who might be looking at that.

This is something we can fix up after I land my branch

This comment has been minimized.

@brentdax

brentdax Nov 5, 2018

Collaborator

I agree that it could make sense to specialize write(to:) for String and possibly DefaultStringInterpolation.

The implementation and use of _float${Bits}ToString is entirely behind resilience barriers, so we can change it in the future. I suppose what we can't do in the future, though, is replace _writeASCII(_:) with whatever primitives we'd need to have a stream allocate some empty space, let us write into it, and then tell it how much of that space we used.

/// - Parameter other: A string to append.
public mutating func write(_ other: String) {
self += other
}
public mutating func _writeASCII(_ buffer: UnsafeBufferPointer<UInt8>) {

This comment has been minimized.

@milseman

milseman Nov 4, 2018

Contributor

public?

@@ -836,6 +836,7 @@ extension String {
/// // Prints "Hello, friend"
///
/// - Parameter other: Another string.
@inlinable

This comment has been minimized.

@milseman

milseman Nov 4, 2018

Contributor

If _StringGuts.append isn't inlinable, then this shouldn't be either.

This comment has been minimized.

@brentdax

brentdax Nov 5, 2018

Collaborator

You're right. I'll let you fix that up.

self.init(_large: storage)
}
}

This comment has been minimized.

@milseman

milseman Nov 4, 2018

Contributor

How important is the @inlinable part? Is it because of forming a canonical empty string with the small check? If so, then I'll adapt my adaptation, but otherwise would like to keep the allocation out of inlinable code when possible. Helps code size and resilience.

This comment has been minimized.

@brentdax

brentdax Nov 5, 2018

Collaborator

I want to inline if capacity <= _SmallUTF8String.capacity because, if we only use this in string interpolation, capacity will always be a constant and the optimizer can fold away one of the branches. I also want to inline self.init() because it should reduce down to the empty string singleton constant.

I don't really care about what's in the else branch, though (although ideally it would skip any small-string tests). If this implementation inlines more than you want to, you can choose a different one.

return
}
_appendSlow(other)
}

This comment has been minimized.

@milseman

milseman Nov 4, 2018

Contributor

Is this inlinable still necessary now that we have the _initialCapacity init? Could we drop it?

This comment has been minimized.

@brentdax

brentdax Nov 5, 2018

Collaborator

Currently, DefaultStringInterpolation.write(_:) is calling _appendSlow(_:) directly to bypass append(_:)'s empty-string check, which is false for the majority of segments. This seemed to benchmark well, but it might be over-fitting the benchmarks and/or an unnecessary complication. Or...

}
}
extension DefaultStringInterpolation: TextOutputStream {
@inlinable

This comment has been minimized.

@milseman

milseman Nov 4, 2018

Contributor

Can drop inlinable if _appendSlow isn't

This comment has been minimized.

@brentdax

brentdax Nov 5, 2018

Collaborator

...it might make sense to make this non-inlineable. If you'd like, I can make a throwaway branch to see how that benchmarks (against the current implementation).

}
@inlinable
public mutating func _writeASCII(_ buffer: UnsafeBufferPointer<UInt8>) {

This comment has been minimized.

@milseman

milseman Nov 4, 2018

Contributor

public? Also, can drop inlinable if append isn't

milseman added a commit to milseman/swift that referenced this pull request Nov 4, 2018

Implementation for SE-0228: Fix ExpressibleByStringInterpolation (app…
…le#20214)

* [CodeCompletion] Restrict ancestor search to brace

This change allows ExprParentFinder to restrict certain searches for parents to just AST nodes within the nearest surrounding BraceStmt. In the string interpolation rework, BraceStmts can appear in new places in the AST; this keeps code completion from looking at irrelevant context.

NFC in this commit, but keeps code completion from crashing once TapExpr is introduced.

* Remove test relying on ExpressibleByStringInterpolation being deprecated

Since soon enough, it won’t be anymore.

* [AST] Introduce TapExpr

TapExpr allows a block of code to to be inserted between two expressions, accessing and potentially mutating the result of its subexpression before giving it to its parent expression. It’s roughly equivalent to this function:

  func _tap<T>(_ value: T, do body: (inout T) throws -> Void) rethrows -> T {
    var copy = value
    try body(&copy)
    return copy
  }

Except that it doesn’t use a closure, so no variables are captured and no call frame is (even notionally) added.

This commit does not include tests because nothing in it actually uses TapExpr yet. It will be used by string interpolation.

* SE-0228: Fix ExpressibleByStringInterpolation

This is the bulk of the implementation of the string interpolation rework. It includes a redesigned AST node, new parsing logic, new constraints and post-typechecking code generation, and new standard library types and members.

* [Sema] Rip out typeCheckExpressionShallow()

With new string interpolation in place, it is no longer used by anything in the compiler.

* [Sema] Diagnose invalid StringInterpolationProtocols

StringInterpolationProtocol informally requires conforming types to provide at least one method with the base name “appendInterpolation” with no (or a discardable) return value and visibility at least as broad as the conforming type’s. This change diagnoses an error when a conforming type does not have a method that meets those criteria.

* [Stdlib] Fix map(String.init) source break

Some users, including some in the source compatibility suite, accidentally used init(stringInterpolationSegment:) by writing code like `map(String.init)`. Now that these intializers have been removed, the remaining initializers often end up tying during overload resolution. This change adds several overloads of `String.init(describing:)` which will break these ties in cases where the compiler previously selected `String.init(stringInterpolationSegment:)`.

* [Sema] Make callWitness() take non-mutable arrays

It doesn’t actually need to mutate them.

* [Stdlib] Improve floating-point interpolation performance

This change avoids constructing a String when interpolating a Float, Double, or Float80. Instead, we write the characters to a fixed-size buffer and then append them directly to the string’s storage.

This seems to improve performance for all three types, but especially for Double and Float80, which cannot always fit into a small string when stringified.

* [NameLookup] Improve MemberLookupTable invalidation

In rare cases usually involving generated code, an overload added by an extension in the middle of a file would not be visible below it if the type had lazy members and the same base name had already been referenced above the extension. This change essentially dirties a type’s member lookup table whenever an extension is added to it, ensuring the entries in it will be updated.

This change also includes some debugging improvements for NameLookup.

* [SILOptimizer] XFAIL dead object removal failure

The DeadObjectRemoval pass in SILOptimizer does not currently remove reworked string interpolations as well as the old design because their effects cannot be described by @_effects(readonly). That causes a test failure on Linux. This change temporarily silences that test. The SILOptimizer issue has been filed as SR-9008.

* Confess string interpolation’s source stability sins

* [Parser] Parse empty interpolations

Previously, the parser had an odd asymmetry which caused the same function to accept foo(), but reject “\()”. This change fixes the issue.

Already tested by test/Parse/try.swift, which uses this construct in one of its throwing interpolation tests.

* [Sema] Fix batch-mode-only lazy var bug

The temporary variable used by string interpolation needs to be recontextualized when it’s inserted into a synthesized getter. Fixes a compilation failure in Alamofire.

I’ll probably follow up on this bug a bit more after merging.
@rintaro

This comment has been minimized.

Member

rintaro commented Nov 5, 2018

@brentdax
Reverting "[CodeCompletion] Restrict ancestor search to brace" in #20323 .
While I agree analyzing context beyond BraceStmt is useless, this specific change does nothing.

@brentdax

This comment has been minimized.

Collaborator

brentdax commented Nov 5, 2018

@rintaro Interesting. That particular part of the PR is very old and, at the time, was necessary to resolve some failing (and I think crashing) tests. If those tests no longer fail without it, I agree that it's probably no longer needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment