Implementation for SE-0228: Fix ExpressibleByStringInterpolation #19963
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci please test |
|
@swift-ci please test compiler performance |
|
@swift-ci please smoke benchmark |
Build comment file:Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: Swift libraries
How to read the dataThe 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 |
|
We should investigate the performance and code size regressions before we land this |
|
@swift-ci please smoke benchmark |
|
Let's see how much StringInterpolation.o actually grew... |
Build comment file:Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: Swift libraries
How to read the dataThe 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 |
|
So StringInterpolation.o's existing benchmarks grew 5% in -O and 9.6% in -Osize; it's only the new benchmarks that made it look like it had grown 50-60%. Still need to get a handle on the rest of the code size increase, of course. I'm guessing it's because we're inlining more—we just need to find the right balance there. Before that, though, let's try a potential cheap fix for the FloatingPointPrinting_Float_interpolated performance regression. |
|
@swift-ci please smoke benchmark |
Build comment file:Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: Swift libraries
How to read the dataThe 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 |
beccadax
force-pushed
the
interpolation-rework
branch
from
4033d0d to
cbb1678
Compare
|
Did some offline benchmarking since CI was looking a little noisy. The
It's just that something else about new string interpolation is slower for all float types than the old version, and Comparing this branch's -emit-sil dump of |
|
A large part of the problem is the inlining of an early exit from
Even with the costs, though, inlining this check is often profitable, and removing it will cause performance regressions. On an iMac Pro: With inlining vs. without — Regression (8)
With inlining vs. without — Improvement (14)
The test being inlined is complicated—probably unnecessarily—and I'm hoping a simpler version will get us the best of both worlds. Barring that, perhaps the SIL optimization folks can make some suggestions. I mean, we can do better than this:
|
|
Just do the part that checks for the canonical empty string, that eliminates most of that CFG. |
|
@milseman That’s what I left my computer benchmarking— |
|
On my own machine, with everything closed down, literally unplugged from the network, I got wildly different benchmark results for this last night vs. this morning. But I think this is at least an improvement. We'll see what CI says. |
beccadax
force-pushed
the
interpolation-rework
branch
2 times, most recently
from
30b4055 to
878057b
Compare
|
@swift-ci please smoke benchmark |
xedin
reviewed
Oct 25, 2018
|
|
||
| if (auto subExpr = expr->getSubExpr()) { | ||
| auto subExprType = CS.getType(subExpr); | ||
| CS.addConstraint(ConstraintKind::Bind, subExprType, tv, locator); |
There was a problem hiding this comment.
One small change which I think might work here - instead of biding sub-expr type to type variable, you can return subExprType directly and allocate type variable only if there was no sub-expression...
There was a problem hiding this comment.
Lower priority than the runtime performance stuff, but I'll look at it. Thanks!
| interpolationProto->lookupDirect(tc.Context.Id_StringInterpolation); | ||
| if (associatedTypeArray.empty()) { | ||
| tc.diagnose(expr->getStartLoc(), diag::interpolation_broken_proto); | ||
| return nullptr; |
There was a problem hiding this comment.
I think interpolation protocol lookup and field lookup could be moved to TypeChecker just like we do for integers e.g. TypeChecker::getMaxIntegerType
There was a problem hiding this comment.
Other parts of CSGen look up associated types in various ad-hoc ways like "find the one associated type in this protocol (let's hope there's just one)" or "directly call getIdentifier() with a string literal containing the associated type's name, then look it up". That's not necessarily a good thing either, though, so maybe I should just clean all of those up.
| // Must be Conversion; if it's Equal, then in semi-rare cases, the | ||
| // interpolation temporary variable cannot be @lvalue. | ||
| CS.addConstraint(ConstraintKind::Conversion, appendingExprType, | ||
| interpolationTV, appendingLocator); |
There was a problem hiding this comment.
I'm really curious of what the example of behavior described in the comment might look like
There was a problem hiding this comment.
There's a minor, non-ABI improvement I want to make to this code that I can't do because it triggers this behavior. Once the branch is merged, I'll redo that work and show you the problem.
Build comment file:Performance: -O
Code size: -O
Performance: -Osize
Code size: -Osize
Performance: -Onone
Code size: Swift libraries
How to read the dataThe 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 |
|
Not there yet, but much, much better. CFG for Fibonacci now looks like this:
In principle, I think we should be able to resolve these at compile time in most or all cases: An instance which has passed through |
|
Gonna rebase to get floating-point string formatting improvements from @airspeedswift. |
|
reserveCapacitySlow, IIRC, can be called on lazily bridged Cocoa strings, so not native necessarily. Otherwise, yeah, we should come up with some pattern or behavior to allow the optimizer to constant-fold all the subsequent branches. |
beccadax
force-pushed
the
interpolation-rework
branch
from
878057b to
3e6ac59
Compare
|
@milseman Right—what I meant is that it's never the empty singleton. (Although in string interpolation, I don't think we can ever call |
This comment has been minimized.
This comment has been minimized.
Previously, the parser generated compound method names for appendInterpolation(…) methods because this helped when finding appendInterpolation methods declared in the same file. However, this turned out to prevent default arguments from working. This commit returns it to adding base names only and instead explicitly calls loadAllMembers() on the StringInterpolation type. I’m not sure why types in other expressions don’t need this but types in these generated expressions do, but it’s much closer to the problem and doesn’t seem to have any ill effects.
This change: 1. Adds accessors for the bit attached to NominalTypeDecl::LookupTable. These let you more easily search for and break on changes; more importantly, they give this bit a name and some documentation explaining what (I think) it means. 2. Includes the “ignoreNewExtensions” parameter in the debug output from NominalTypeDecl::lookupDirect(). 3. Adds a MemberLookupTable::dump() method.
These methods are never used and appear to be holdovers from an earlier implementation.
Previously, nothing would guarantee that, if a nominal type `T` with lazy members had extensions adding members with name `foo`, and these members were already in `T`’s LookupTable, and a new extension to `T` added another member named `foo`, the new extension’s member would be added to the LookupTable. This change makes it so that adding an extension clears the isLookupTablePopulated() flag, and so that when the flag is cleared on a type with lazy members, it updates all existing entries instead of just the ones present in the type itself. Finally, it removes a workaround in string interpolation which is no longer necessary because of this change.
Apparently, the macOS STL is more forgiving about tuples vs. pairs than the Linux one.
As currently written, the optimizer can completely remove the involvement of CustomString in some cases. We probably don’t want that, so let’s fix it.
Probably responsible for at least part of performance gap.
This change tests that, in string interpolation, code completion doesn’t suggest Void functions but suggests functions returning other types. This is a change in behavior, but I’m not convinced the old version was correct in the first place—why would you want to interpolate a function that returns Void? It’s valid but not useful, and code completion seems to try to avoid suggesting Void functions in other similar circumstances.
This already didn't work on 32-bit platforms; now it's not working on 64-bit Linux. Filed as SR-9008.
They will return, probably in a separate pull request; I just want to see how much they're inflating StringInterpolation.o's code size.
Gets most of the performance win without most of the code size loss.
Should avoid relying so heavily on the optimizer realizing it can remove branches. This had somewhat complicated performance impacts in local benchmarking; we’ll see what it does on CI.
This should allow us to separately control how appends are inlined for string interpolation.
beccadax
force-pushed
the
interpolation-rework
branch
from
e854689 to
f23d1cd
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just a rebase of the previous PR, #18590.
This PR implements a new string interpolation API with a different, finalized ABI from what we're currently shipping. We'd like to land it before the ABI freezes; otherwise we'll have to also support the existing initializers forever.
Remaining issues to resolve:
Issues we'll wait to address:
DoubleandFloat80are faster to interpolate than before, butFloatis slower. It's hard to test this in isolation while the rest of the branch is in flux, so we plan to land the change and then look atFloatinterpolation.Implements SE-0228. Resolves SR-1260, SR-2303, and SR-3969. Resolves rdar://43621912.
cc @milseman @ravikandhadai