Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

Commit

Permalink
fix(change-detection): remove memory leak, use iterator
Browse files Browse the repository at this point in the history 
When the change detection finds change it creates a
linked list of records which have changed. The linked
list is not bound to groups but rather crosses groups,
this implies that if we remove a group it is possible that
a record in a live group retains a pointer to a
record in the now deleted record. While technically
not a leak, the memory will not be realized until that
record detects another change and gets re-added to the
dirty list. If the record never fires than the
memory is retained forever.
  • Loading branch information
@mhevery
mhevery committed Mar 5, 2014
1 parent 5b1416b commit 75fbded
Show file tree
Hide file tree
Showing 6 changed files with 223 additions and 161 deletions.
6 changes: 3 additions & 3 deletions lib/change_detection/ast.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,8 @@ String _argList(List<AST> items) => items.join(', ');
/**
* The name is a bit oxymoron, but it is essentially the NullObject pattern.
*
* This allows children to set a handler on this ChangeRecord and then let it write the initial
* constant value to the forwarding ChangeRecord.
* This allows children to set a handler on this Record and then let it write the initial
* constant value to the forwarding Record.
*/
class _ConstantWatchRecord extends WatchRecord<_Handler> {
final currentValue;
Expand All @@ -134,7 +134,7 @@ class _ConstantWatchRecord extends WatchRecord<_Handler> {
: currentValue = currentValue,
handler = new _ConstantHandler(watchGroup, expression, currentValue);

ChangeRecord<_Handler> check() => null;
bool check() => false;
void remove() => null;

get field => null;
Expand Down
24 changes: 7 additions & 17 deletions lib/change_detection/change_detection.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ abstract class ChangeDetectorGroup<H> {
* Parameters:
* - [object] to watch.
* - [field] to watch on the [object].
* - [handler] an opaque object passed on to [ChangeRecord].
* - [handler] an opaque object passed on to [Record].
*/
WatchRecord<H> watch(Object object, String field, H handler);

Expand All @@ -43,18 +43,18 @@ abstract class ChangeDetectorGroup<H> {
* predictable performance, and the developer can implement `.equals()` on top
* of identity checks.
*
* - [H] A [ChangeRecord] has associated handler object. The handler object is
* - [H] A [Record] has associated handler object. The handler object is
* opaque to the [ChangeDetector] but it is meaningful to the code which
* registered the watcher. It can be a data structure, an object, or a function.
* It is up to the developer to attach meaning to it.
*/
abstract class ChangeDetector<H> extends ChangeDetectorGroup<H> {
/**
* This method does the work of collecting the changes and returns them as a
* linked list of [ChangeRecord]s. The [ChangeRecord]s are returned in the
* linked list of [Record]s. The [Record]s are returned in the
* same order as they were registered.
*/
ChangeRecord<H> collectChanges({ EvalExceptionHandler exceptionHandler,
Iterator<Record<H>> collectChanges({ EvalExceptionHandler exceptionHandler,
AvgStopwatch stopwatch });
}

Expand Down Expand Up @@ -93,24 +93,14 @@ abstract class WatchRecord<H> extends Record<H> {
set object(value);

/**
* Check to see if the field on the object has changed. Returns [null] if no
* change, or a [ChangeRecord] if a change has been detected.
* Check to see if the field on the object has changed. Returns [true] if
* change has been detected.
*/
ChangeRecord<H> check();
bool check();

void remove();
}

/**
* Provides information about the changes which were detected in objects.
*
* It exposes a `nextChange` method for traversing all of the changes.
*/
abstract class ChangeRecord<H> extends Record<H> {
/** Next [ChangeRecord] */
ChangeRecord<H> get nextChange;
}

/**
* If the [ChangeDetector] is watching a [Map] then the [currentValue] of
* [Record] will contain an instance of this object. A [MapChangeRecord]
Expand Down
60 changes: 40 additions & 20 deletions lib/change_detection/dirty_checking_change_detector.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,6 @@ class GetterCache {
* Each property to be watched is recorded as a [DirtyCheckingRecord] and kept
* in a linked list. Linked list are faster than Arrays for iteration. They also
* allow removal of large blocks of watches in an efficient manner.
*
* [ChangeRecord]
*
* When the results are delivered they are a linked list of [ChangeRecord]s. For
* efficiency reasons the [DirtyCheckingRecord] and [ChangeRecord] are two
* different interfaces for the same underlying object this makes reporting
* efficient since no additional memory allocation is performed.
*/
class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
/**
Expand Down Expand Up @@ -177,6 +170,10 @@ class DirtyCheckingChangeDetectorGroup<H> implements ChangeDetectorGroup<H> {
nextGroup._prev = prevGroup;
}
_parent = null;
_prev = _next = null;
_recordHead._prevRecord = null;
_recordTail._nextRecord = null;
_recordHead = _recordTail = null;
assert(root._assertRecordsOk());
}

Expand Down Expand Up @@ -281,7 +278,7 @@ class DirtyCheckingChangeDetector<H> extends DirtyCheckingChangeDetectorGroup<H>
return true;
}

DirtyCheckingRecord<H> collectChanges({ EvalExceptionHandler exceptionHandler,
Iterator<Record<H>> collectChanges({ EvalExceptionHandler exceptionHandler,
AvgStopwatch stopwatch}) {
if (stopwatch != null) stopwatch.start();
DirtyCheckingRecord changeHead = null;
Expand All @@ -291,11 +288,11 @@ class DirtyCheckingChangeDetector<H> extends DirtyCheckingChangeDetectorGroup<H>
int count = 0;
while (current != null) {
try {
if (current.check() != null) {
if (current.check()) {
if (changeHead == null) {
changeHead = changeTail = current;
} else {
changeTail = changeTail.nextChange = current;
changeTail = changeTail._nextChange = current;
}
}
if (stopwatch != null) count++;
Expand All @@ -308,16 +305,39 @@ class DirtyCheckingChangeDetector<H> extends DirtyCheckingChangeDetectorGroup<H>
}
current = current._nextRecord;
}
if (changeTail != null) changeTail.nextChange = null;
if (changeTail != null) changeTail._nextChange = null;
if (stopwatch != null) stopwatch..stop()..increment(count);
return changeHead;
return new _ChangeIterator(changeHead);
}

void remove() {
throw new StateError('Root ChangeDetector can not be removed');
}
}

class _ChangeIterator<H> implements Iterator<Record<H>>{
DirtyCheckingRecord _current;
DirtyCheckingRecord _next;
DirtyCheckingRecord get current => _current;

_ChangeIterator(this._next);

bool moveNext() {
_current = _next;
if (_next != null) {
_next = _current._nextChange;
/*
* This is important to prevent memory leaks. If we don't reset then
* a record maybe pointing to a deleted change detector group and it
* will not release the reference until it fires again. So we have
* to be eager about releasing references.
*/
_current._nextChange = null;
}
return _current != null;
}
}

/**
* [DirtyCheckingRecord] represents as single item to check. The heart of the
* [DirtyCheckingRecord] is a the [check] method which can read the
Expand All @@ -327,7 +347,7 @@ class DirtyCheckingChangeDetector<H> extends DirtyCheckingChangeDetectorGroup<H>
* removing efficient. [DirtyCheckingRecord] also has a [nextChange] field which
* creates a single linked list of all of the changes for efficient traversal.
*/
class DirtyCheckingRecord<H> implements ChangeRecord<H>, WatchRecord<H> {
class DirtyCheckingRecord<H> implements Record<H>, WatchRecord<H> {
static const List<String> _MODE_NAMES =
const ['MARKER', 'IDENT', 'REFLECT', 'GETTER', 'MAP[]', 'ITERABLE', 'MAP'];
static const int _MODE_MARKER_ = 0;
Expand All @@ -350,7 +370,7 @@ class DirtyCheckingRecord<H> implements ChangeRecord<H>, WatchRecord<H> {
var currentValue;
DirtyCheckingRecord<H> _nextRecord;
DirtyCheckingRecord<H> _prevRecord;
ChangeRecord<H> nextChange;
Record<H> _nextChange;
var _object;
InstanceMirror _instanceMirror;

Expand Down Expand Up @@ -416,12 +436,12 @@ class DirtyCheckingRecord<H> implements ChangeRecord<H>, WatchRecord<H> {
}
}

ChangeRecord<H> check() {
bool check() {
assert(_mode != null);
var current;
switch (_mode) {
case _MODE_MARKER_:
return null;
return false;
case _MODE_REFLECT_:
current = _instanceMirror.getField(_symbol).reflectee;
break;
Expand All @@ -435,9 +455,9 @@ class DirtyCheckingRecord<H> implements ChangeRecord<H>, WatchRecord<H> {
current = object;
break;
case _MODE_MAP_:
return (currentValue as _MapChangeRecord)._check(object) ? this : null;
return (currentValue as _MapChangeRecord)._check(object);
case _MODE_ITERABLE_:
return (currentValue as _CollectionChangeRecord)._check(object) ? this : null;
return (currentValue as _CollectionChangeRecord)._check(object);
default:
assert(false);
}
Expand All @@ -454,10 +474,10 @@ class DirtyCheckingRecord<H> implements ChangeRecord<H>, WatchRecord<H> {
} else {
previousValue = last;
currentValue = current;
return this;
return true;
}
}
return null;
return false;
}


Expand Down
24 changes: 12 additions & 12 deletions lib/change_detection/linked_list.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,19 +92,19 @@ abstract class _EvalWatchList {

static _EvalWatchRecord _add(_EvalWatchList list, _EvalWatchRecord item) {
assert(item._nextEvalWatch == null);
assert(item._previousEvalWatch == null);
assert(item._prevEvalWatch == null);
var prev = list._evalWatchTail;
var next = prev._nextEvalWatch;

if (prev == list._marker) {
list._evalWatchHead = list._evalWatchTail = item;
prev = prev._previousEvalWatch;
prev = prev._prevEvalWatch;
}
item._nextEvalWatch = next;
item._previousEvalWatch = prev;
item._prevEvalWatch = prev;

if (prev != null) prev._nextEvalWatch = item;
if (next != null) next._previousEvalWatch = item;
if (next != null) next._prevEvalWatch = item;

return list._evalWatchTail = item;
}
Expand All @@ -113,21 +113,21 @@ abstract class _EvalWatchList {

static void _remove(_EvalWatchList list, _EvalWatchRecord item) {
assert(item.watchGrp == list);
var prev = item._previousEvalWatch;
var prev = item._prevEvalWatch;
var next = item._nextEvalWatch;

if (list._evalWatchHead == list._evalWatchTail) {
list._evalWatchHead = list._evalWatchTail = list._marker;
list._marker
.._nextEvalWatch = next
.._previousEvalWatch = prev;
.._prevEvalWatch = prev;
if (prev != null) prev._nextEvalWatch = list._marker;
if (next != null) next._previousEvalWatch = list._marker;
if (next != null) next._prevEvalWatch = list._marker;
} else {
if (item == list._evalWatchHead) list._evalWatchHead = next;
if (item == list._evalWatchTail) list._evalWatchTail = prev;
if (prev != null) prev._nextEvalWatch = next;
if (next != null) next._previousEvalWatch = prev;
if (next != null) next._prevEvalWatch = prev;
}
}
}
Expand All @@ -137,11 +137,11 @@ class _WatchGroupList {

static WatchGroup _add(_WatchGroupList list, WatchGroup item) {
assert(item._nextWatchGroup == null);
assert(item._previousWatchGroup == null);
assert(item._prevWatchGroup == null);
if (list._watchGroupTail == null) {
list._watchGroupHead = list._watchGroupTail = item;
} else {
item._previousWatchGroup = list._watchGroupTail;
item._prevWatchGroup = list._watchGroupTail;
list._watchGroupTail._nextWatchGroup = item;
list._watchGroupTail = item;
}
Expand All @@ -151,10 +151,10 @@ class _WatchGroupList {
static bool _isEmpty(_WatchGroupList list) => list._watchGroupHead == null;

static void _remove(_WatchGroupList list, WatchGroup item) {
var previous = item._previousWatchGroup;
var previous = item._prevWatchGroup;
var next = item._nextWatchGroup;

if (previous == null) list._watchGroupHead = next; else previous._nextWatchGroup = next;
if (next == null) list._watchGroupTail = previous; else next._previousWatchGroup = previous;
if (next == null) list._watchGroupTail = previous; else next._prevWatchGroup = previous;
}
}
Loading

0 comments on commit 75fbded

Please sign in to comment.