[CSBindings] Try `Void` as a binding for `closure result` type variable #15134

xedin · 2018-03-10T02:50:58Z

If we could find a binding for type variable representing closure result
let's also add implicit Void type as another pontential binding to attempt
because there is an implicit conversion from () -> T to () -> Void
so finding Void early is going to help avoid function conversions down
the line.

Resolves: rdar://problem/37790062

xedin · 2018-03-10T02:51:06Z

@swift-ci please test

xedin · 2018-03-10T02:51:17Z

@swift-ci please test source compatibility

xedin · 2018-03-10T02:53:15Z

/cc @slavapestov since we have determineBestBindings look for related type inferable from subtype constraints this is going to make it so Void is inferred directly when possible, so no changes to CSApply to IRGen are required.

swift-ci · 2018-03-10T03:15:38Z

Build failed
Swift Test Linux Platform
Git Sha - e24c0d3654e1f97e54fda781459319b91a5d6b75

swift-ci · 2018-03-10T04:55:14Z

Build failed
Swift Test OS X Platform
Git Sha - e24c0d3654e1f97e54fda781459319b91a5d6b75

xedin · 2018-03-10T05:03:10Z

@swift-ci please test macOS platform

xedin · 2018-03-10T05:04:15Z

Looks like Linux is still broken :(

slavapestov · 2018-03-10T05:22:56Z

@xedin I'm not familiar with this code, I think @rudkx should review it. But if you want to test for the absence of a function conversion thunk, don't you need to write a SILGen test?

xedin · 2018-03-10T05:25:00Z

@slavapestov I just wanted to run source compatibility first to make sure that I didn't break anything :) I'll add couple more tests to SILGen.

xedin · 2018-03-10T05:26:18Z

/cc @rudkx please take a look, I can't figure out safer fix for this... (I've reverted changes to ClosureResult + IRGen since C style functions don't support thunks like that).

xedin · 2018-03-10T08:00:40Z

@swift-ci please test Linux platform

If we could find a binding for type variable representing closure result let's also add implicit `Void` type as another pontential binding to attempt because there is an implicit conversion from `() -> T` to `() -> Void` so finding `Void` early is going to help avoid function conversions down the line. Resolves: rdar://problem/37790062

xedin · 2018-03-11T08:42:27Z

Rebased the changes to getPotentialBindings

xedin · 2018-03-11T08:42:33Z

@swift-ci please test

xedin · 2018-03-11T08:47:21Z

@swift-ci please test source compatibility

xedin · 2018-03-12T02:47:12Z

@swift-ci please test Linux platform

DougGregor

LGTM, thank you!

rudkx

Otherwise LGTM.

rudkx · 2018-03-12T18:59:16Z

lib/Sema/CSBindings.cpp

+
+        if (auto *locator = typeVar->getImpl().getLocator()) {
+          auto path = locator->getPath();
+          auto voidType = TupleType::getEmpty(getASTContext());


ASTContext has TheEmptyTupleType for this (and it's already a CanType).

Included the fix to #15187

davezarzycki · 2018-03-13T02:38:43Z

This change broke Swift on Fedora Linux 27 (built with clang+llvm ToT). The following validation tests hang:

compiler_crashers_fixed/28684-isactuallycanonicalornull-forming-a-cantype-out-of-a-non-canonical-type.swift
compiler_crashers_fixed/28686-swift-typebase-getcanonicaltype.swift

davezarzycki · 2018-03-13T02:45:09Z

Backtraces (albeit Release+Assert):

http://znu.io/lldb-28684.out
http://znu.io/lldb-28686.out

davezarzycki · 2018-03-13T02:50:42Z

Well look at that. "Hang" turns out to be relative. The test suite takes 10x longer on my box after this pull request. Regardless, this seems like a massive performance regression.

xedin · 2018-03-13T02:55:09Z

@davezarzycki Are you sure that it's this change? It's only related to closure result type variable handling, in couple of narrow cases.

xedin · 2018-03-13T02:59:30Z

@davezarzycki It doesn't seem like ci.swift.org reports that behavior change in build tests before and after this PR went it, all of the incremental builds take ~1 hour.

davezarzycki · 2018-03-13T03:08:03Z

I just reverified that this PR is the source of the regression. Pull #15172 was merged before this PR, and that doesn't have the performance regression, therefore this PR is the source of the problem.

I'm not surprised the build bots mask the problem. They're slow. My Linux box can do a clean build and run validation suite (sans stress tests) in just a few minutes. Therefore this performance regression is quite obvious. :-/

xedin · 2018-03-13T03:11:24Z

Do you have any examples of expressions which became slower? 10x performance regression is not something which should leave CI times unchanged (theoretically that is).

davezarzycki · 2018-03-13T03:17:57Z

How do I extract the slow expressions from the tests that are slower?

The test suite performance is a great example of Amdahl's Law. With just a few cores, having one test become 10x longer is hard to notice, but with many cores, one test getting longer is quite noticeable.

xedin · 2018-03-13T03:27:23Z

Are you only talking about two tests you have mentioned? If so, they are pretty expreme cases of auto-generated code I am surprised that they don’t result in “expression to complex” to be honest. Don’t see a reason to revert this just based on such failures.

davezarzycki · 2018-03-13T03:39:34Z

Yes, just the two two tests. They run for almost exactly ten minutes each then exit after this PR. Here is some perf output, if it helps:

Samples: 1M of event 'cycles:uppp', Event count (approx.): 1029367409751
  Overhead  Command  Shared Object        Symbol
+   12.77%  swift    swift                [.] swift::constraints::ConstraintGraph::lookupNode
+    8.27%  swift    swift                [.] swift::constraints::ConstraintGraph::gatherConstraints
+    7.65%  swift    swift                [.] llvm::SmallPtrSetImpl<swift::TypeVariableType*>::insert
+    6.20%  swift    swift                [.] swift::TypeVariableType::Implementation::getRepresentative
+    5.96%  swift    swift                [.] swift::constraints::ConstraintGraphNode::getAdjacency
+    5.21%  swift    swift                [.] swift::Type::transformRec
+    4.74%  swift    swift                [.] connectedComponentsDFS
     4.21%  swift    swift                [.] swift::constraints::ConstraintGraph::computeConnectedComponents
+    3.17%  swift    swift                [.] swift::constraints::ConstraintSystem::determineBestBindings
+    2.76%  swift    swift                [.] llvm::function_ref<swift::Type (swift::Type)>::callback_fn<swift::Type simplifyTypeImpl<swift::constraints::ConstraintSystem::simplifyType(swift::Type)::$_7>(swift::constraints
+    2.28%  swift    swift                [.] swift::constraints::ConstraintGraphNode::getEquivalenceClass
+    2.23%  swift    swift                [.] swift::constraints::ConstraintSystem::getPotentialBindings
+    2.13%  swift    swift                [.] llvm::SmallPtrSetImplBase::insert_imp_big
+    2.11%  swift    swift                [.] llvm::DenseMapBase<llvm::DenseMap<std::pair<swift::Type, std::pair<swift::Type, unsigned int> >, swift::FunctionType*, llvm::DenseMapInfo<std::pair<swift::Type, std::pair<swift
     1.91%  swift    swift                [.] swift::constraints::ConstraintSystem::solveRec
+    1.55%  swift    swift                [.] llvm::SmallVectorImpl<swift::constraints::ConstraintSystem::PotentialBinding>::operator=
+    1.50%  swift    swift                [.] llvm::function_ref<llvm::Optional<swift::Type> (swift::TypeBase*)>::callback_fn<swift::Type::transform(llvm::function_ref<swift::Type (swift::Type)>) const::$_15>
+    1.47%  swift    swift                [.] swift::constraints::ConstraintGraph::contractEdges
+    1.32%  swift    swift                [.] swift::constraints::ConstraintSystem::matchTypes
+    1.15%  swift    swift                [.] swift::constraints::ConstraintSystem::getPotentialBindingForRelationalConstraint
+    1.12%  swift    swift                [.] (anonymous namespace)::Traversal::visitAnyFunctionType
+    0.82%  swift    swift                [.] llvm::SmallPtrSetImplBase::FindBucketFor
+    0.79%  swift    swift                [.] llvm::SmallPtrSetImpl<swift::TypeVariableType*>::count
+    0.79%  swift    swift                [.] swift::TypeVisitor<(anonymous namespace)::Traversal, bool>::visit
+    0.78%  swift    swift                [.] llvm::SmallPtrSetImpl<swift::constraints::Constraint*>::insert
+    0.77%  swift    swift                [.] llvm::SmallPtrSetImplBase::Grow
+    0.76%  swift    swift                [.] swift::constraints::ConstraintSystem::matchFunctionTypes
+    0.72%  swift    swift                [.] llvm::SmallPtrSetImplBase::SmallPtrSetImplBase
+    0.65%  swift    swift                [.] llvm::DenseMapBase<llvm::SmallDenseMap<swift::TypeVariableType*, swift::constraints::ConstraintSystem::PotentialBindings, 4u, llvm::DenseMapInfo<swift::TypeVariableType*>, llvm
+    0.63%  swift    swift                [.] llvm::DenseMapBase<llvm::DenseMap<swift::constraints::Constraint*, unsigned int, llvm::DenseMapInfo<swift::constraints::Constraint*>, llvm::detail::DenseMapPair<swift::constrai

xedin · 2018-03-13T03:45:18Z

Thanks! I will re-adjust the tests in the separate PR then.

xedin · 2018-03-13T04:30:28Z

It's weird that on Linux it's so much slower - I just ran one of the tests locally and it only took 1.56s to complete.

davezarzycki · 2018-03-13T12:18:13Z

Ya, I observed that at first too. Once I copied the exact process arguments from the output of ps axww, I was able to reproduce this. Which did you do?

davezarzycki · 2018-03-13T12:54:24Z

It seems that the interaction of this pull request and -swift-version 3 (as invoked by the test suite) causes one of the two tests to become slow. I haven't checked the other. Does this help?

xedin · 2018-03-13T19:18:15Z

@davezarzycki I just ran it using --test-paths option via ./utils/build-script that would involve lit on the specific file/directory which represents the way CI runs the tests too.

davezarzycki · 2018-03-13T19:32:18Z

Weird. I don't personally care about the performance of -swift-version 3. Can we force these tests to use the current version of the language? And for whatever it may be worth, without -swift-version 3, the tests finish in about 10 and 20 seconds respectively, which is still dramatically slower than 1.56 seconds on your computer. Very weird.

xedin · 2018-03-13T19:41:25Z

I think it's okay if we remove swift-version from the test, since they are trying to make sure that we don't crash trying to solve this anyway. But I'm curious, if you remove -swift-version 3 how long do these tests take on your machine?

davezarzycki · 2018-03-13T19:42:47Z

10 and 20 seconds respectively, as I just wrote.

xedin · 2018-03-13T19:43:43Z

Ah sorry I misread that, how about with -swift-version 3 then?

davezarzycki · 2018-03-13T19:44:06Z

They both seem to time out precisely after 10 minutes.

davezarzycki · 2018-03-13T19:46:36Z

That's probably because 'not' and/or test harness itself assumes success if it these two tests haven't crashed in 10 minutes. :-/

xedin · 2018-03-13T19:51:13Z

I just extracted the command line for the first test you mentioned and try to run it with and without -swift-version 3 and it's pretty much the same result on macOS (DebugAssert build) so I'm wondering what is so special about Linux in this case, since solver should behave the same regardless what platform it's run on.

Anyway, I'll modify these tests to limit type-checker runtime to 1 second which is good anyway.

davezarzycki · 2018-03-13T19:52:00Z

I'm running Release-Assert, if it matters.

xedin requested a review from rudkx March 10, 2018 05:25

xedin force-pushed the rdar-37790062-v3 branch from e24c0d3 to 18d2c1d Compare March 11, 2018 08:42

swiftlang deleted a comment from swift-ci Mar 11, 2018

swiftlang deleted a comment from swift-ci Mar 12, 2018

DougGregor approved these changes Mar 12, 2018

View reviewed changes

xedin merged commit 94dbbd7 into swiftlang:master Mar 12, 2018

rudkx reviewed Mar 12, 2018

View reviewed changes

xedin mentioned this pull request Mar 13, 2018

[crashers] NFC: Limit time spent in solver for crashers 28684 and 28686 #15215

Merged

davezarzycki mentioned this pull request Mar 15, 2018

Allow @available(swift, ...) nested within @available(macOS, ...) #15213

Merged

xedin mentioned this pull request Mar 15, 2018

[SR-7059] Failed to infer generic type parameter in Xcode 9.3 beta 3 #49607

Closed

[CSBindings] Try Void as a binding for closure result type variable #15134

[CSBindings] Try Void as a binding for closure result type variable #15134

Uh oh!

Conversation

xedin commented Mar 10, 2018

Uh oh!

xedin commented Mar 10, 2018

Uh oh!

xedin commented Mar 10, 2018

Uh oh!

xedin commented Mar 10, 2018

Uh oh!

swift-ci commented Mar 10, 2018

Uh oh!

swift-ci commented Mar 10, 2018

Uh oh!

xedin commented Mar 10, 2018

Uh oh!

xedin commented Mar 10, 2018

Uh oh!

slavapestov commented Mar 10, 2018

Uh oh!

xedin commented Mar 10, 2018

Uh oh!

xedin commented Mar 10, 2018

Uh oh!

xedin commented Mar 10, 2018

Uh oh!

xedin commented Mar 11, 2018

Uh oh!

xedin commented Mar 11, 2018

Uh oh!

xedin commented Mar 11, 2018

Uh oh!

xedin commented Mar 12, 2018

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

rudkx left a comment

Choose a reason for hiding this comment

Uh oh!

rudkx Mar 12, 2018

Choose a reason for hiding this comment

Uh oh!

xedin Mar 12, 2018

Choose a reason for hiding this comment

Uh oh!

davezarzycki commented Mar 13, 2018

Uh oh!

davezarzycki commented Mar 13, 2018

Uh oh!

davezarzycki commented Mar 13, 2018

Uh oh!

xedin commented Mar 13, 2018

Uh oh!

xedin commented Mar 13, 2018

Uh oh!

davezarzycki commented Mar 13, 2018

Uh oh!

xedin commented Mar 13, 2018

Uh oh!

davezarzycki commented Mar 13, 2018

Uh oh!

xedin commented Mar 13, 2018

Uh oh!

davezarzycki commented Mar 13, 2018

Uh oh!

xedin commented Mar 13, 2018

Uh oh!

xedin commented Mar 13, 2018

Uh oh!

davezarzycki commented Mar 13, 2018

Uh oh!

davezarzycki commented Mar 13, 2018

Uh oh!

xedin commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davezarzycki commented Mar 13, 2018

[CSBindings] Try `Void` as a binding for `closure result` type variable #15134

[CSBindings] Try `Void` as a binding for `closure result` type variable #15134

xedin commented Mar 13, 2018 •

edited

Loading