Skip to content

[ConstraintSystem] Change the order in which we attempt disjunctions to be stable. #17975

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

Merged
merged 3 commits into from
Jul 16, 2018
Merged

[ConstraintSystem] Change the order in which we attempt disjunctions to be stable. #17975

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e82fcb8
[ConstraintSystem] Move disjunction selection closer to where selected
rudkx Jul 15, 2018
6fa403d
[ConstraintSystem] Change the order in which we attempt disjunctions
rudkx Jul 16, 2018
8b2b4a7
Small simplification resulting from using std::min_element.
rudkx Jul 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 28 additions & 21 deletions lib/Sema/CSSolver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1492,13 +1492,8 @@ bool ConstraintSystem::solveRec(SmallVectorImpl<Solution> &solutions,

// If we don't have more than one component, just solve the whole
// system.
if (numComponents < 2) {
SmallVector<Constraint *, 8> disjunctions;
collectDisjunctions(disjunctions);

return solveSimplified(selectDisjunction(disjunctions), solutions,
allowFreeTypeVariables);
}
if (numComponents < 2)
return solveSimplified(solutions, allowFreeTypeVariables);

if (TC.Context.LangOpts.DebugConstraintSolver) {
auto &log = getASTContext().TypeCheckerDebug->getStream();
Expand Down Expand Up @@ -1885,46 +1880,56 @@ static Constraint *selectBestBindingDisjunction(
return nullptr;
}

Constraint *ConstraintSystem::selectDisjunction(
SmallVectorImpl<Constraint *> &disjunctions) {
Constraint *ConstraintSystem::selectDisjunction() {
SmallVector<Constraint *, 4> disjunctions;

collectDisjunctions(disjunctions);

if (disjunctions.empty())
return nullptr;

auto *disjunction =
selectBestBindingDisjunction(*this, disjunctions);

if (disjunction)
return disjunction;

// Pick the smallest disjunction.
// FIXME: This heuristic isn't great, but it helped somewhat for
// overload sets.
// Pick the disjunction with the lowest disjunction number in order
// to solve them in the order they were created (which should be
// stable within an expression).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean abandoning the idea of finding a better heuristic?

I've been wondering if it'd be possible to use a smarter heuristic here. One that takes into account not just disjunction size, but also

  1. the dependencies between the disjunctions (to minimize backtracking)
  2. the relative restrictiveness of their related constraints.

Would the approach you're taking here preclude that?

(Or maybe that approach is an "obviously" bad idea. If so, I'd be grateful for any recommendations for things to read up on.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a fixed ordering by definition precludes trying other orderings.

We've considered and tried some other heuristics here and they have shown some promise, but have not really provided significant benefit.

The path I am looking at now is:

  • Fix the ordering that we traverse in based on the user-written expression (or at least the way we walk them in the compiler).
  • Rather than removing short-circuiting that we do, double-down on the notion that we should stop solving once we have a clear "best" solution.
  • Determine the "best" solution primarily by comparing overloads we're attempting in a given disjunction. My plan is to make a partial ordering of overloads using isDeclAsSpecializedAs, and then having a sort of hierarchy of disjunctions based on that ordering (i.e. three incomparable overloads would all be at the same level in the hierarchy, ones that are clearly less specialized would be at the next level - all the overloads would form a poset).
  • All the elements of the disjunction at the same level would be attempted, and if any results in a successful solution, stop. If more than one does, then use secondary criteria to determine a winner. If none does, move on to the next level in the hierarchy.
  • Where the current scoring mechanism fits into this is TBD, but my hope is that we can eventually remove the short-circuiting based on score, and instead use that as one of the tie-break criteria. Some initial experimentation also leads me to believe that we'll need to use the score in some way to decide whether the solutions we find in one level of the hierarchy are clear winners.

The hope is that this newer sort of short circuiting will be more predictable and easier to reason about as well as allowing us to bail out earlier and come up with a final solution faster as a result.

disjunction = disjunctions[0];
auto bestSize = disjunction->countActiveNestedConstraints();
if (bestSize > 2) {
auto found = DisjunctionNumber.find(disjunction);
assert(found != DisjunctionNumber.end());
auto lowestNumber = found->second;
if (lowestNumber > 0) {
for (auto contender : llvm::makeArrayRef(disjunctions).slice(1)) {
unsigned newSize = contender->countActiveNestedConstraints();
if (newSize < bestSize) {
bestSize = newSize;
auto found = DisjunctionNumber.find(contender);
assert(found != DisjunctionNumber.end());
unsigned newNumber = found->second;
if (newNumber < lowestNumber) {
lowestNumber = newNumber;
disjunction = contender;

if (bestSize == 2)
if (lowestNumber == 0)
break;
}
}
}

// If there are no active constraints in the disjunction, there is
// no solution.
if (bestSize == 0)
return nullptr;
// if (bestSize == 0)
// return nullptr;

return disjunction;
}

bool ConstraintSystem::solveSimplified(
Constraint *disjunction, SmallVectorImpl<Solution> &solutions,
SmallVectorImpl<Solution> &solutions,
FreeTypeVariableBinding allowFreeTypeVariables) {

auto *disjunction = selectDisjunction();

auto bestBindings = determineBestBindings();

// If we have a binding that does not involve type variables, and is
Expand All @@ -1937,6 +1942,8 @@ bool ConstraintSystem::solveSimplified(
allowFreeTypeVariables);
}



// If there are no disjunctions we can't solve this system unless we have
// free type variables and are allowing them in the solution.
if (!disjunction) {
Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/Constraint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//
// This source file is part of the Swift.org open source project
//
// Copyright (c) 2014 - 2017 Apple Inc. and the Swift project authors
// Copyright (c) 2014 - 2018 Apple Inc. and the Swift project authors
// Licensed under Apache License v2.0 with Runtime Library Exception
//
// See https://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -798,6 +798,7 @@ Constraint *Constraint::createDisjunction(ConstraintSystem &cs,
auto disjunction = new (mem) Constraint(ConstraintKind::Disjunction,
cs.allocateCopy(constraints), locator, typeVars);
disjunction->RememberChoice = (bool) rememberChoice;
cs.noteNewDisjunction(disjunction);
return disjunction;
}

Expand Down
32 changes: 17 additions & 15 deletions lib/Sema/ConstraintSystem.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,13 @@ class ConstraintSystem {
/// The original CS if this CS was created as a simplification of another CS
ConstraintSystem *baseCS = nullptr;

/// \brief The total number of disjunctions created.
unsigned CountDisjunctions = 0;

/// \brief Map from disjunction to the number indicating the order it
// was created in.
llvm::DenseMap<Constraint *, unsigned> DisjunctionNumber;

private:

/// \brief Allocator used for all of the related constraint systems.
Expand Down Expand Up @@ -1852,6 +1859,11 @@ class ConstraintSystem {
bool allowFixes,
ConstraintLocatorBuilder locator);

void noteNewDisjunction(Constraint *constraint) {
assert(constraint->getKind() == ConstraintKind::Disjunction);
DisjunctionNumber[constraint] = CountDisjunctions++;
}

/// \brief Add a disjunction constraint.
void addDisjunctionConstraint(ArrayRef<Constraint *> constraints,
ConstraintLocatorBuilder locator,
Expand Down Expand Up @@ -2920,17 +2932,13 @@ class ConstraintSystem {
/// \brief Solve the system of constraints after it has already been
/// simplified.
///
/// \param disjunction The disjunction to try and solve using simplified
/// constraint system.
///
/// \param solutions The set of solutions to this system of constraints.
///
/// \param allowFreeTypeVariables How to bind free type variables in
/// the solution.
///
/// \returns true if an error occurred, false otherwise.
bool solveSimplified(Constraint *disjunction,
SmallVectorImpl<Solution> &solutions,
bool solveSimplified(SmallVectorImpl<Solution> &solutions,
FreeTypeVariableBinding allowFreeTypeVariables);

/// \brief Find reduced domains of disjunction constraints for given
Expand All @@ -2942,15 +2950,10 @@ class ConstraintSystem {
/// \param expr The expression to find reductions for.
void shrink(Expr *expr);

/// \brief Pick a disjunction from the given list, which,
/// based on the associated constraints, is the most viable to
/// reduce depth of the search tree.
///
/// \param disjunctions A collection of disjunctions to examine.
/// \brief Pick a disjunction from the InactiveConstraints list.
///
/// \returns The disjunction with most weight relative to others, based
/// on the number of constraints associated with it, or nullptr otherwise.
Constraint *selectDisjunction(SmallVectorImpl<Constraint *> &disjunctions);
/// \returns The selected disjunction.
Constraint *selectDisjunction();

bool simplifyForConstraintPropagation();
void collectNeighboringBindOverloadDisjunctions(
Expand Down Expand Up @@ -3345,8 +3348,7 @@ class Component {
llvm::SaveAndRestore<ConstraintSystem::SolverScope *>
partialSolutionScope(cs.solverState->PartialSolutionScope, &scope);

failed = cs.solveSimplified(cs.selectDisjunction(Disjunctions), solutions,
allowFreeTypeVariables);
failed = cs.solveSimplified(solutions, allowFreeTypeVariables);
}

// Put the constraints back into their original bucket.
Expand Down
3 changes: 2 additions & 1 deletion validation-test/stdlib/AnyHashable.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,8 +757,9 @@ AnyHashableTests.test("AnyHashable(MinimalHashableRCSwiftError)/Hashable") {
xs,
equalityOracle: { $0 / 2 == $1 / 2 },
hashEqualityOracle: { $0 / 4 == $1 / 4 })
let mapped = xs.map(AnyHashable.init)
checkHashable(
xs.map(AnyHashable.init),
mapped,
equalityOracle: { $0 / 2 == $1 / 2 },
hashEqualityOracle: { $0 / 4 == $1 / 4 })
}
Expand Down