Skip to content

Commit 3f825f6

Browse files
authored
Invalidate eagerly by newline constraint (#1495)
Propagate newline constrains eagerly when binding states in a Solution. One of the main ways the formatter defines its formatting style is by having constraints that prohibit newlines in some child pieces when a parent is in a given state. The two simplest examples (which cover a large amount of code) is that when an InfixPiece or a ListPiece is in State.unsplit, they don't allow any newlines in any of their children. That constraint effectively creates the desired style which is that a split inside an infix operand or list element forces the surrounding expression to split. Whenever we bind a parent piece to some state, we can now ask it if it allows any of its children to contain newlines. For any given child, if the answer is "no" then: - If the child is already bound to a state that contains newlines, then we know this solution is a dead end. It and every solution you can ever derive from it will contain an invalid newline. - If the child isn't bound to a state yet, we can still look at all of its states and see which of them contain newlines. Any state that does can be eliminated because we'll never successfully bind the child to that state without violating this constraint. It may turn out that no states are left, in which case again we have a dead end solution. Or there may be just one valid state (usually State.unsplit), and we can immediately the child to that state and do this whole process recursively for this child. Or there may be just a couple of states left and we can at least winnow this child's states down to that list when we go to expand it later. The end result is that even before actually formatting a solution, we can often tell if it's a dead end and discard it. If not, we can often bind a whole bunch of the bound piece's children in one go instead of having to do them one at a time and slowly formatting the interim solutions at each step. The end result is a pretty decent improvement. Here's the micro benchmarks: ``` Benchmark (tall) fastest median slowest average baseline ----------------------------- -------- ------- ------- ------- -------- block 0.070 0.071 0.122 0.075 92.7% chain 0.640 0.654 0.681 0.655 254.5% collection 0.175 0.180 0.193 0.181 96.3% collection_large 0.930 0.955 0.988 0.955 97.2% conditional 0.067 0.068 0.086 0.071 132.4% curry 0.596 0.608 1.478 0.631 278.9% flutter_popup_menu_test 0.293 0.302 0.326 0.303 141.4% flutter_scrollbar_test 0.160 0.166 0.184 0.166 96.1% function_call 1.460 1.483 1.680 1.490 97.5% infix_large 0.709 0.733 0.761 0.733 97.4% infix_small 0.175 0.181 0.198 0.181 93.0% interpolation 0.091 0.096 0.118 0.096 98.7% large 3.514 3.574 3.767 3.596 129.5% top_level 0.146 0.150 0.182 0.152 106.7% ``` There's a slight regression on some of the tiny microbenchmarks (probably because there's some overhead traversing and binding children to State.unsplit when that solution ends up winning anyway). But the improvement to the larger ones is significant. More interesting is the overall performance. Here's formatting the Flutter repo: ``` Current formatter 10.964 ======================================== This PR 8.826 ================================ Short style formatter 4.558 ================ ``` The current formatter is 58.43% slower than the old short style formatter. This PR is 24.22% faster than the current formatter. It's 48.36% slower than the old formatter, but it gets the formatter 33.37% of the way to the old short style one.
1 parent 8a236c0 commit 3f825f6

23 files changed

+719
-479
lines changed

lib/src/back_end/code_writer.dart

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ class CodeWriter {
209209
/// and multi-line strings.
210210
void whitespace(Whitespace whitespace, {bool flushLeft = false}) {
211211
if (whitespace case Whitespace.newline || Whitespace.blankLine) {
212-
_handleNewline(allowNewlines: true);
212+
_hadNewline = true;
213213
_pendingIndent = flushLeft ? 0 : _indentStack.last.indent;
214214
}
215215

@@ -231,15 +231,15 @@ class CodeWriter {
231231
/// be `false`. It's up to the parent piece to only call this when it's safe
232232
/// to do so. In practice, this usually means when the parent piece knows that
233233
/// [piece] will have a newline before and after it.
234-
void format(Piece piece, {bool separate = false, bool allowNewlines = true}) {
234+
void format(Piece piece, {bool separate = false}) {
235235
if (separate) {
236236
Profile.count('CodeWriter.format() piece separate');
237237

238238
_formatSeparate(piece);
239239
} else {
240240
Profile.count('CodeWriter.format() piece inline');
241241

242-
_formatInline(piece, allowNewlines: allowNewlines);
242+
_formatInline(piece);
243243
}
244244
}
245245

@@ -270,7 +270,7 @@ class CodeWriter {
270270
}
271271

272272
/// Format [piece] writing directly into this [CodeWriter].
273-
void _formatInline(Piece piece, {required bool allowNewlines}) {
273+
void _formatInline(Piece piece) {
274274
// Begin a new formatting context for this child.
275275
var previousPiece = _currentPiece;
276276
_currentPiece = piece;
@@ -309,9 +309,25 @@ class CodeWriter {
309309

310310
_currentPiece = previousPiece;
311311

312-
// If the child contained a newline then the parent transitively does.
313-
if (childHadNewline && _currentPiece != null) {
314-
_handleNewline(allowNewlines: allowNewlines);
312+
// If the child contained a newline then invalidate the solution if any of
313+
// the containing pieces don't allow one at this point in the tree.
314+
if (childHadNewline) {
315+
// TODO(rnystrom): We already do much of the newline constraint validation
316+
// when the Solution is first created before we format. For performance,
317+
// it would be good to do *all* of it before formatting. The missing part
318+
// is that pieces containing hard newlines (comments, multiline strings,
319+
// sequences, etc.) do not constrain their parents when the solution is
320+
// first created. If we can get that working, then this check can be
321+
// removed.
322+
if (_currentPiece case var parent?
323+
when !parent.allowNewlineInChild(
324+
_solution.pieceState(parent), piece)) {
325+
_solution.invalidate(_currentPiece!);
326+
}
327+
328+
// Note that this piece contains a newline so that we can propagate that
329+
// up to containing pieces too.
330+
_hadNewline = true;
315331
}
316332
}
317333

@@ -327,18 +343,6 @@ class CodeWriter {
327343
_solution.endSelection(_buffer.length + end);
328344
}
329345

330-
/// Notes that a newline has been written.
331-
///
332-
/// If this occurs in a place where newlines are prohibited, then invalidates
333-
/// the solution.
334-
void _handleNewline({required bool allowNewlines}) {
335-
if (!allowNewlines) _solution.invalidate(_currentPiece!);
336-
337-
// Note that this piece contains a newline so that we can propagate that
338-
// up to containing pieces too.
339-
_hadNewline = true;
340-
}
341-
342346
/// Write any pending whitespace.
343347
///
344348
/// This is called before non-whitespace text is about to be written, or

0 commit comments

Comments
 (0)