Skip to content

Commit

Permalink
perf(watch-group): remove expression coalescing
Browse files Browse the repository at this point in the history 
Coalescing is rare but slows down WatchGroup creation.

Closes dart-archive#1328
  • Loading branch information
@mhevery @chirayuk
mhevery authored and chirayuk committed Aug 7, 2014
1 parent 76a8676 commit 9371491
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 36 deletions.
38 changes: 8 additions & 30 deletions lib/change_detection/watch_group.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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++}'
{
Expand All @@ -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;
Expand All @@ -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);
}

Expand All @@ -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.
Expand All @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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?
Expand Down
12 changes: 6 additions & 6 deletions test/change_detection/watch_group_spec.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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']);
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand All @@ -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', () {
Expand Down

0 comments on commit 9371491

Please sign in to comment.