perf(watch-group): remove expression coalescing

Coalescing is rare but slows down WatchGroup creation.

Closes #1328

Author: mhevery

lib/change_detection/watch_group.dart

@@ -54,7 +54,6 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
   final ChangeDetectorGroup<_Handler> _changeDetector;
   /** A cache for sharing sub expression watching. Watching `a` and `a.b` will
   * watch `a` only once. */
-  final Map<String, WatchRecord<_Handler>> _cache;
   final RootWatchGroup _rootGroup;
 
   /// STATS: Number of field watchers which are in use.
@@ -108,7 +107,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
   WatchGroup _prevWatchGroup, _nextWatchGroup;
 
   WatchGroup._child(_parentWatchGroup, this._changeDetector, this.context,
-                    this._cache, this._rootGroup)
+                    this._rootGroup)
       : _parentWatchGroup = _parentWatchGroup,
         id = '${_parentWatchGroup.id}.${_parentWatchGroup._nextChildId++}'
   {
@@ -119,8 +118,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
   WatchGroup._root(this._changeDetector, this.context)
       : id = '',
         _rootGroup = null,
-        _parentWatchGroup = null,
-        _cache = new HashMap<String, WatchRecord<_Handler>>()
+        _parentWatchGroup = null
   {
     _marker.watchGrp = this;
     _evalWatchTail = _evalWatchHead = _marker;
@@ -139,10 +137,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
   }
 
   Watch watch(AST expression, ReactionFn reactionFn) {
-    WatchRecord<_Handler> watchRecord = _cache[expression.expression];
-    if (watchRecord == null) {
-      _cache[expression.expression] = watchRecord = expression.setupWatch(this);
-    }
+    WatchRecord<_Handler> watchRecord = expression.setupWatch(this);
     return watchRecord.handler.addReactionFn(reactionFn);
   }
 
@@ -161,10 +156,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
     _fieldCost++;
     fieldHandler.watchRecord = watchRecord;
 
-    WatchRecord<_Handler> lhsWR = _cache[lhs.expression];
-    if (lhsWR == null) {
-      lhsWR = _cache[lhs.expression] = lhs.setupWatch(this);
-    }
+    WatchRecord<_Handler> lhsWR = lhs.setupWatch(this);
 
     // We set a field forwarding handler on LHS. This will allow the change
     // objects to propagate to the current WatchRecord.
@@ -180,10 +172,7 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
     var watchRecord = _changeDetector.watch(null, null, collectionHandler);
     _collectionCost++;
     collectionHandler.watchRecord = watchRecord;
-    WatchRecord<_Handler> astWR = _cache[ast.expression];
-    if (astWR == null) {
-      astWR = _cache[ast.expression] = ast.setupWatch(this);
-    }
+    WatchRecord<_Handler> astWR = ast.setupWatch(this);
 
     // We set a field forwarding handler on LHS. This will allow the change
     // objects to propagate to the current WatchRecord.
@@ -234,32 +223,23 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
     invokeHandler.watchRecord = evalWatchRecord;
 
     if (lhsAST != null) {
-      var lhsWR = _cache[lhsAST.expression];
-      if (lhsWR == null) {
-        lhsWR = _cache[lhsAST.expression] = lhsAST.setupWatch(this);
-      }
+      var lhsWR = lhsAST.setupWatch(this);
       lhsWR.handler.addForwardHandler(invokeHandler);
       invokeHandler.acceptValue(lhsWR.currentValue);
     }
 
     // Convert the args from AST to WatchRecords
     for (var i = 0; i < argsAST.length; i++) {
       var ast = argsAST[i];
-      WatchRecord<_Handler> record = _cache[ast.expression];
-      if (record == null) {
-        record = _cache[ast.expression] = ast.setupWatch(this);
-      }
+      WatchRecord<_Handler> record = ast.setupWatch(this);
       _ArgHandler handler = new _PositionalArgHandler(this, evalWatchRecord, i);
       _ArgHandlerList._add(invokeHandler, handler);
       record.handler.addForwardHandler(handler);
       handler.acceptValue(record.currentValue);
     }
 
     namedArgsAST.forEach((Symbol name, AST ast) {
-      WatchRecord<_Handler> record = _cache[ast.expression];
-      if (record == null) {
-        record = _cache[ast.expression] = ast.setupWatch(this);
-      }
+      WatchRecord<_Handler> record = ast.setupWatch(this);
       _ArgHandler handler = new _NamedArgHandler(this, evalWatchRecord, name);
       _ArgHandlerList._add(invokeHandler, handler);
       record.handler.addForwardHandler(handler);
@@ -301,7 +281,6 @@ class WatchGroup implements _EvalWatchList, _WatchGroupList {
         this,
         _changeDetector.newGroup(),
         context == null ? this.context : context,
-        new HashMap<String, WatchRecord<_Handler>>(),
         _rootGroup == null ? this : _rootGroup);
     _WatchGroupList._add(this, childGroup);
     var marker = childGroup._marker;
@@ -581,7 +560,6 @@ abstract class _Handler implements _LinkedList, _LinkedListItem, _WatchList {
       _releaseWatch();
       // Remove ourselves from cache, or else new registrations will go to us,
       // but we are dead
-      watchGrp._cache.remove(expression);
 
       if (forwardingHandler != null) {
         // TODO(misko): why do we need this check?

test/change_detection/watch_group_spec.dart

@@ -273,7 +273,7 @@ void main() {
         var watch = watchGrp.watch(parse('user'), (v, p) => logger(v));
         var watchFirst = watchGrp.watch(parse('user.first'), (v, p) => logger(v));
         var watchLast = watchGrp.watch(parse('user.last'), (v, p) => logger(v));
-        expect(watchGrp.fieldCost).toEqual(3);
+        expect(watchGrp.fieldCost).toEqual(5);
 
         watchGrp.detectChanges();
         expect(logger).toEqual([user1, 'misko', 'hevery']);
@@ -285,7 +285,7 @@ void main() {
 
 
         watch.remove();
-        expect(watchGrp.fieldCost).toEqual(3);
+        expect(watchGrp.fieldCost).toEqual(4);
 
         watchFirst.remove();
         expect(watchGrp.fieldCost).toEqual(2);
@@ -863,8 +863,8 @@ void main() {
         expect(watchGrp.detectChanges()).toEqual(6);
         // expect(logger).toEqual(['0a', '0A', '1a', '1A', '2A', '1b']);
         expect(logger).toEqual(['0a', '1a', '1b', '0A', '1A', '2A']); // we go by registration order
-        expect(watchGrp.fieldCost).toEqual(1);
-        expect(watchGrp.totalFieldCost).toEqual(4);
+        expect(watchGrp.fieldCost).toEqual(2);
+        expect(watchGrp.totalFieldCost).toEqual(6);
         logger.clear();
 
         context['a'] = 1;
@@ -876,8 +876,8 @@ void main() {
         child1a.remove(); // should also remove child2
         expect(watchGrp.detectChanges()).toEqual(3);
         expect(logger).toEqual(['0a', '0A', '1b']);
-        expect(watchGrp.fieldCost).toEqual(1);
-        expect(watchGrp.totalFieldCost).toEqual(2);
+        expect(watchGrp.fieldCost).toEqual(2);
+        expect(watchGrp.totalFieldCost).toEqual(3);
       });
 
       it('should remove all method watches in group and group\'s children', () {