Skip to content

Commit

Permalink
Separately format lines when a multisplit is split.
Browse files Browse the repository at this point in the history
The LineWriter's job is to take a stream of chunks and deliver batches of them
to LineSplitter. Each batch must be isolated such that splitting one won't
affect the results of later ones.

If the LineWriter is too pessimistic about this, it can end up giving the
LineSplitter a long list of chunks that it then dutifully splits into a few
separate lines. The output is correct, but the performance isn't optimal
because the splitter goes (much) faster on shorter chunk lists.

One cases where the LineWriter wasn't fine-grained enough was multisplits.
When a hard split occurs inside a multisplit, LineWriter correctly hardens all
of the splits associated with that multisplit. For example, if you have:

  {key:"k", [[[// comment
  ]]}

As soon as the line comment is hit, the formatter knows all of the surrounding
collection literals must be split:

  {
    key:"k",
    [
      [
        [
          // comment
        ]
      ]
    ]
  }

What it doesn't do is go back and say, "Ah, now that I know some of those
earlier chunks have hard splits, can I process them as separate lines?" This
meant that, for example:

  method() {
    veryLongExpression(...)
  }

would be split as a single blob, instead of breaking a separate line at the
"{". That, in turn makes things like a deep statement right at the beginning
of a method body gets even worse since the LineSplitter rolls it into the
previous line too.

This fixes that. Makes the benchmark 3x faster.

R=pquitslund@google.com

Review URL: https://chromiumcodereview.appspot.com//987253002
  • Loading branch information
munificent committed Mar 10, 2015
1 parent db6b9bf commit ff18272
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 24 deletions.
22 changes: 20 additions & 2 deletions lib/src/chunk.dart
Expand Up @@ -283,7 +283,8 @@ class SplitParam {
/// together, like parameter lists and binary operator expressions.
class Span {
/// Index of the first chunk contained in this span.
final int start;
int get start => _start;
int _start;

/// Index of the last chunk contained in this span.
int get end => _end;
Expand All @@ -293,7 +294,7 @@ class Span {
/// if the span is for a multisplit.
final int cost;

Span(this.start, this.cost);
Span(this._start, this.cost);

/// Marks this span as ending at [end].
void close(int end) {
Expand All @@ -314,6 +315,23 @@ class Span {

return result + ")";
}

/// Shifts the indexes of the chunk down by [offset].
///
/// This is used when a prefix of the chunk list gets pulled off by the
/// [LineWriter] after it gets formatted as a line. The remaining spans need
/// to have their indices shifted to account for the removed chunks.
///
/// Returns `true` if the span has shifted all the way off the front and
/// should just be discarded.
bool shift(int offset) {
if (end != null && end < offset) return true;

_start -= offset;
if (_end != null) _end -= offset;

return false;
}
}

/// A comment in the source, with a bit of information about the surrounding
Expand Down
60 changes: 38 additions & 22 deletions lib/src/line_writer.dart
Expand Up @@ -472,7 +472,7 @@ class LineWriter {
/// Finishes writing and returns a [SourceCode] containing the final output
/// and updated selection, if any.
SourceCode end() {
if (_chunks.isNotEmpty) _completeLine();
if (_chunks.isNotEmpty) _completeLine(_chunks.length);

// Be a good citizen, end with a newline.
if (_source.isCompilationUnit) _buffer.write(_formatter.lineEnding);
Expand Down Expand Up @@ -636,49 +636,49 @@ class LineWriter {
// Since we're about to write some text on the next line, we know the
// previous one is fully done being tweaked and merged, so now we can see
// if it can be split independently.
_checkForCompleteLine();
_checkForCompleteLine(_chunks.length);

_chunks.add(new Chunk(text));
}
}

/// Checks to see if we are currently at a point where the existing chunks
/// can be processed as a single line and processes them if so.
/// Checks to see if we the first [length] chunks can be processed as a
/// single line and processes them if so.
///
/// We want to send small lists of chunks to [LineSplitter] for performance.
/// We can do that when we know one set of chunks will absolutely not affect
/// anything following it. The rule for that is pretty simple: a hard newline
/// that is not nested inside an expression.
void _checkForCompleteLine() {
if (_chunks.isEmpty) return;
bool _checkForCompleteLine(int length) {
if (length == 0) return false;

// Hang on to the split info so we can reset the writer to start with it.
var split = _chunks[length - 1];

// Can only split on a hard line that is not nested in the middle of an
// expression.
if (!_chunks.last.isHardSplit || _chunks.last.nesting >= 0) return;
if (!split.isHardSplit || split.nesting >= 0) return false;

// Hang on to the split info so we can reset the writer to start with it.
var split = _chunks.last;

// Don't write any empty line, just discard it.
if (_chunks.isNotEmpty) {
_completeLine();
_chunks.clear();
}
_completeLine(length);

_spans.clear();
// Discard the formatted chunks and any spans contained in them.
_chunks.removeRange(0, length);
_spans.removeWhere((span) => span.shift(length));

// Get ready for the next line.
_bufferedNewlines = split.isDouble ? 2 : 1;
_beginningIndent = split.indent;

return true;
}

/// Hands off the current list of chunks to [LineSplitter] as a single logical
/// line.
void _completeLine() {
/// Hands off the first [length] chunks to the [LineSplitter] as a single
/// logical line to be split.
void _completeLine(int length) {
assert(_chunks.isNotEmpty);

if (debugFormatter) {
dumpChunks(_chunks);
dumpChunks(_chunks.take(length).toList());
print(_spans.join("\n"));
}

Expand All @@ -687,8 +687,18 @@ class LineWriter {
_buffer.write(_formatter.lineEnding);
}

// If we aren't completing the entire set of chunks, get the subset that we
// are completing.
var chunks = _chunks;
var spans = _spans;

if (length < _chunks.length) {
chunks = chunks.take(length).toList();
spans = spans.where((span) => span.start <= length).toList();
}

var splitter = new LineSplitter(_formatter.lineEnding, _formatter.pageWidth,
_chunks, _spans, _beginningIndent);
chunks, spans, _beginningIndent);
var selection = splitter.apply(_buffer);

if (selection[0] != null) _selectionStart = selection[0];
Expand Down Expand Up @@ -725,11 +735,17 @@ class LineWriter {
if (splitParams.isEmpty) return;

// Take any existing splits for the multisplits and hard split them.
for (var chunk in _chunks) {
for (var i = 0; i < _chunks.length; i++) {
var chunk = _chunks[i];
if (chunk.param == null) continue;

if (splitParams.contains(chunk.param)) {
chunk.harden();

// Now that this chunk is a hard split, we may be able to format up to
// it as its own line. If so, the chunks will get removed, so reset
// the loop counter.
if (_checkForCompleteLine(i + 1)) i = -1;
} else {
// If the chunk isn't hardened, but implies something that is, we can
// discard the implication since it is always satisfied now.
Expand Down

0 comments on commit ff18272

Please sign in to comment.