Skip to content

Commit 6b2b31e

Browse files
authored
Don't eagerly bind pinned Pieces in Solution. (#1407)
Before this change, constructor for Solution would recursively traverse the entire piece tree from the root down. It did so looking for pieces that were pinned, so that it could add them to the pieceState map and (more importantly) apply any constraints that the pinned piece's state implies on other child pieces. Unfortunately, this traversal interacts poorly with the optimization to format some piece subtrees separately. The Solution constructor for the root piece would traverse the whole tree, then Solution constructors for any child pieces being formatted separately would also traverse their piece trees, and so on. In practice, it was roughly quadratic. Oops. :( This change removes that traversal completely. Instead, when a piece is pinned, we apply any constraints at that point in time and then pin the constrained pieces. Then, during solving, Solution looks to see if the piece has a pinned state before looking at its own bound state map. That way, we never have to bother redundantly storing the states of pinned pieces in the state map. On my machine, this makes the large benchmark about 17% faster.
1 parent be7eb5e commit 6b2b31e

File tree

2 files changed

+12
-24
lines changed

2 files changed

+12
-24
lines changed

lib/src/back_end/solution.dart

Lines changed: 5 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -104,24 +104,6 @@ class Solution implements Comparable<Solution> {
104104
var pieceStates = <Piece, State>{};
105105
var cost = 0;
106106

107-
// Bind every pinned piece to its state and propagate any constraints from
108-
// those.
109-
void traversePinned(Piece piece) {
110-
if (piece.pinnedState case var pinned?) {
111-
var additionalCost = _tryBind(pieceStates, piece, pinned);
112-
113-
// Pieces should be implemented such that they never get pinned into a
114-
// conflicting state because then there's no possible solution, so
115-
// [_tryBind()] should always succeed and [additionalCost] won't be
116-
// `null`.
117-
cost += additionalCost!;
118-
}
119-
120-
piece.forEachChild(traversePinned);
121-
}
122-
123-
traversePinned(root);
124-
125107
// If we're formatting a subtree of a larger Piece tree that binds [root]
126108
// to [rootState], then bind it in this solution too.
127109
if (rootState != null) {
@@ -149,10 +131,12 @@ class Solution implements Comparable<Solution> {
149131
/// The state this solution selects for [piece].
150132
///
151133
/// If no state has been selected, defaults to the first state.
152-
State pieceState(Piece piece) => _pieceStates[piece] ?? State.unsplit;
134+
State pieceState(Piece piece) =>
135+
piece.pinnedState ?? _pieceStates[piece] ?? State.unsplit;
153136

154-
/// Whether [piece] has been bound to a state in this set.
155-
bool isBound(Piece piece) => _pieceStates.containsKey(piece);
137+
/// Whether [piece] has been bound to a state in this set (or is pinned).
138+
bool isBound(Piece piece) =>
139+
piece.pinnedState != null || _pieceStates.containsKey(piece);
156140

157141
/// Increases the total overflow for this solution by [overflow].
158142
///

lib/src/piece/piece.dart

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,7 @@ abstract class Piece {
1919
/// This is [State.unsplit], which all pieces support, followed by any other
2020
/// [additionalStates].
2121
List<State> get states {
22-
// Pinned pieces should be bound eagerly in the Solution so we shouldn't
23-
// ever need to iterate over their states.
24-
assert(_pinnedState == null);
22+
if (_pinnedState case var pinned?) return [pinned];
2523
return [State.unsplit, ...additionalStates];
2624
}
2725

@@ -132,6 +130,12 @@ abstract class Piece {
132130
/// Forces this piece to always use [state].
133131
void pin(State state) {
134132
_pinnedState = state;
133+
134+
// If this piece's pinned state constrains any child pieces, pin those too,
135+
// recursively.
136+
applyConstraints(state, (other, constrainedState) {
137+
other.pin(constrainedState);
138+
});
135139
}
136140

137141
/// The name of this piece as it appears in debug output.

0 commit comments

Comments
 (0)