Optimize splitting lines with many rules.

The constraint checking was quadratic in the number of rules in the
line which is bad on pathologically long lines like #456.

This gets it back down to linear. It takes that bug from 7 minutes to
about 20 seconds on my machine. The benchmark, which is closer to real
code, gets about 20% faster.

I'm still leaving #456 open because I think there's more work to be
done there, but this is a solid improvement overall.

[email protected]

Review URL: https://codereview.chromium.org//1418483008 .

CHANGELOG.md

@@ -12,6 +12,7 @@
 * Indent multi-line collections in default values (#441).
 * Handle `if` statements without curly bodies better (#448).
 * Handle loop statements without curly bodies better (#449).
+* Optimize splitting lines with many rules.
 
 # 0.2.0
 

lib/src/chunk.dart

@@ -254,8 +254,8 @@ class Chunk extends Selection {
     } else {
       parts.add(rule.toString());
 
-      if (_rule.outerRules.isNotEmpty) {
-        parts.add("-> ${_rule.outerRules.join(' ')}");
+      if (_rule.constrainedRules.isNotEmpty) {
+        parts.add("-> ${_rule.constrainedRules.join(' ')}");
       }
     }
 

lib/src/dart_formatter.dart

@@ -114,8 +114,7 @@ class DartFormatter {
             stringSource,
             token.offset,
             math.max(token.length, 1),
-            ParserErrorCode.UNEXPECTED_TOKEN,
-            [token.lexeme]);
+            ParserErrorCode.UNEXPECTED_TOKEN, [token.lexeme]);
 
         throw new FormatterException([error]);
       }

lib/src/debug.dart

@@ -118,8 +118,10 @@ void dumpChunks(int start, List<Chunk> chunks) {
     if (chunk.rule != null) {
       row.add(chunk.isHardSplit ? "" : chunk.rule.toString());
 
-      var outerRules = chunk.rule.outerRules.toSet().intersection(rules);
-      writeIf(outerRules.isNotEmpty, () => "-> ${outerRules.join(" ")}");
+      var constrainedRules =
+          chunk.rule.constrainedRules.toSet().intersection(rules);
+      writeIf(constrainedRules.isNotEmpty,
+          () => "-> ${constrainedRules.join(" ")}");
     } else {
       row.add("(no rule)");
 

lib/src/line_splitting/line_splitter.dart

@@ -145,6 +145,12 @@ class LineSplitter {
     for (var i = 0; i < rules.length; i++) {
       rules[i].index = i;
     }
+
+    // Now that every used rule has an index, tell the rules to discard any
+    // constraints on unindexed rules.
+    for (var rule in rules) {
+      rule.forgetUnusedRules();
+    }
   }
 
   /// Determine the best way to split the chunks into lines that fit in the

lib/src/line_splitting/rule_set.dart

@@ -59,9 +59,7 @@ class RuleSet {
     _values[rule.index] = value;
 
     // Test this rule against the other rules being bound.
-    for (var other in rules) {
-      if (rule == other) continue;
-
+    for (var other in rule.constrainedRules) {
       var otherValue = _values[other.index];
       var constraint = rule.constrain(value, other);
 

lib/src/line_splitting/solve_state.dart

@@ -25,6 +25,16 @@ class SolveState {
   final LineSplitter _splitter;
   final RuleSet _ruleValues;
 
+  /// The set of [Rule]s that are bound by [_ruleValues].
+  ///
+  /// Cached by [_ensureConstraints] for use by [_ensureUnboundConstraints].
+  Set<Rule> _boundRules;
+
+  /// The set of [Rule]s that are not bound by [_ruleValues].
+  ///
+  /// Cached by [_ensureConstraints] for use by [_ensureUnboundConstraints].
+  Set<Rule> _unboundRules;
+
   /// The unbound rules in this state that can be bound to produce new more
   /// refined states.
   ///
@@ -249,6 +259,8 @@ class SolveState {
       if (_constraints[rule] != other._constraints[rule]) return false;
     }
 
+    _ensureUnboundConstraints();
+    other._ensureUnboundConstraints();
     if (_unboundConstraints.length != other._unboundConstraints.length) {
       return false;
     }
@@ -427,10 +439,6 @@ class SolveState {
 
     var added = false;
     for (var constrained in rule.allConstrainedRules) {
-      // The rule may constrain some other rule that was already hardened and
-      // discarded. In that case, we can ignore it.
-      if (constrained.index == null) continue;
-
       if (_ruleValues.contains(constrained)) continue;
 
       _liveRules.add(constrained);
@@ -482,37 +490,55 @@ class SolveState {
   void _ensureConstraints() {
     if (_constraints != null) return;
 
-    var unboundRules = [];
-    var boundRules = [];
+    _unboundRules = new Set();
+    _boundRules = new Set();
 
     for (var rule in _splitter.rules) {
       if (_ruleValues.contains(rule)) {
-        boundRules.add(rule);
+        _boundRules.add(rule);
       } else {
-        unboundRules.add(rule);
+        _unboundRules.add(rule);
       }
     }
 
     _constraints = {};
 
-    for (var unbound in unboundRules) {
-      for (var bound in boundRules) {
+    for (var bound in _boundRules) {
+      for (var unbound in bound.constrainedRules) {
+        if (!_unboundRules.contains(unbound)) continue;
+
         var value = _ruleValues.getValue(bound);
         var constraint = bound.constrain(value, unbound);
         if (constraint != null) {
           _constraints[unbound] = constraint;
         }
       }
     }
+  }
+
+  /// Lazily initializes the [_unboundConstraints], which is needed to compare
+  /// two states for overlap.
+  ///
+  /// We do this lazily because the calculation is a bit slow, and is only
+  /// needed when we have two states with the same score.
+  void _ensureUnboundConstraints() {
+    if (_unboundConstraints != null) return;
+
+    // _ensureConstraints should be called first which initializes these.
+    assert(_boundRules != null);
+    assert(_unboundRules != null);
 
     _unboundConstraints = {};
 
-    for (var unbound in unboundRules) {
+    for (var unbound in _unboundRules) {
       var disallowedValues;
 
-      for (var value = 0; value < unbound.numValues; value++) {
-        for (var j = 0; j < boundRules.length; j++) {
-          var bound = boundRules[j];
+      for (var bound in unbound.constrainedRules) {
+        if (!_boundRules.contains(bound)) continue;
+
+        var boundValue = _ruleValues.getValue(bound);
+
+        for (var value = 0; value < unbound.numValues; value++) {
           var constraint = unbound.constrain(value, bound);
 
           // If the unbound rule doesn't place any constraint on this bound
@@ -523,7 +549,6 @@ class SolveState {
           // we don't need to track it. This way, two states that have the
           // same bound value, one of which has a satisfied constraint, are
           // still allowed to overlap.
-          var boundValue = _ruleValues.getValue(bound);
           if (constraint == boundValue) continue;
           if (constraint == -1 && boundValue != 0) continue;
 

lib/src/rule/argument.dart

@@ -10,7 +10,7 @@ import 'rule.dart';
 /// Base class for a rule that handles argument or parameter lists.
 abstract class ArgumentRule extends Rule {
   /// The rule used to split collections in the argument list, if any.
-  final Rule _collectionRule;
+  Rule _collectionRule;
 
   /// If true, then inner rules that are written will force this rule to split.
   ///
@@ -27,13 +27,15 @@ abstract class ArgumentRule extends Rule {
   /// collections in the list.
   ArgumentRule(this._collectionRule);
 
-  Iterable<Rule> get constrainedRules {
-    var rules = super.constrainedRules;
-    if (_collectionRule != null) {
-      rules = rules.toList()..add(_collectionRule);
-    }
+  void addConstrainedRules(Set<Rule> rules) {
+    if (_collectionRule != null) rules.add(_collectionRule);
+  }
 
-    return rules;
+  void forgetUnusedRules() {
+    super.forgetUnusedRules();
+    if (_collectionRule != null && _collectionRule.index == null) {
+      _collectionRule = null;
+    }
   }
 
   /// Called before a collection argument is written.
@@ -68,13 +70,15 @@ abstract class PositionalRule extends ArgumentRule {
   /// arguments in the list.
   PositionalRule(Rule collectionRule) : super(collectionRule);
 
-  Iterable<Rule> get constrainedRules {
-    var rules = super.constrainedRules;
-    if (_namedArgsRule != null) {
-      rules = rules.toList()..add(_namedArgsRule);
-    }
+  void addConstrainedRules(Set<Rule> rules) {
+    if (_namedArgsRule != null) rules.add(_namedArgsRule);
+  }
 
-    return rules;
+  void forgetUnusedRules() {
+    super.forgetUnusedRules();
+    if (_namedArgsRule != null && _namedArgsRule.index == null) {
+      _namedArgsRule = null;
+    }
   }
 
   /// Remembers [chunk] as containing the split that occurs right before an
@@ -122,8 +126,6 @@ class SinglePositionalRule extends PositionalRule {
   /// internally without forcing a split before the argument.
   final bool splitsOnInnerRules;
 
-  bool hack = false;
-
   /// Creates a new rule for a positional argument list.
   ///
   /// If [collectionRule] is given, it is the rule used to split the

lib/src/rule/rule.dart

@@ -41,7 +41,6 @@ abstract class Rule extends FastHash {
   ///
   /// This contains all direct as well as transitive relationships. If A
   /// contains B which contains C, C's outerRules contains both B and A.
-  Iterable<Rule> get outerRules => _outerRules;
   final Set<Rule> _outerRules = new Set<Rule>();
 
   /// Adds [inner] as an inner rule of this rule if it cares about inner rules.
@@ -78,8 +77,43 @@ abstract class Rule extends FastHash {
     return null;
   }
 
+  /// A protected method for subclasses to add the rules that they constrain
+  /// to [rules].
+  ///
+  /// Called by [Rule] the first time [constrainedRules] is accessed.
+  void addConstrainedRules(Set<Rule> rules) {}
+
+  /// Discards constraints on any rule that doesn't have an index.
+  ///
+  /// This is called by [LineSplitter] after it has indexed all of the in-use
+  /// rules. A rule may end up with a constraint on a rule that's no longer
+  /// used by any chunk. This can happen if the rule gets hardened, or if it
+  /// simply never got used by a chunk. For example, a rule for splitting an
+  /// empty list of metadata annotations.
+  ///
+  /// This removes all of those.
+  void forgetUnusedRules() {
+    _outerRules.retainWhere((rule) => rule.index != null);
+
+    // Clear the cached ones too.
+    _constrainedRules = null;
+    _allConstrainedRules = null;
+  }
+
   /// The other [Rule]s that this rule places immediate constraints on.
-  Iterable<Rule> get constrainedRules => _outerRules;
+  Set<Rule> get constrainedRules {
+    // Lazy initialize this on first use. Note: Assumes this is only called
+    // after the chunks have been written and any constraints have been wired
+    // up.
+    if (_constrainedRules == null) {
+      _constrainedRules = _outerRules.toSet();
+      addConstrainedRules(_constrainedRules);
+    }
+
+    return _constrainedRules;
+  }
+
+  Set<Rule> _constrainedRules;
 
   /// The transitive closure of all of the rules this rule places constraints
   /// on, directly or indirectly, including itself.