implement dirty checking for @observable objects

implement dirty checking for @observable objects
This means MDV can be used without a compile step.
It continues to support implementation by-hand, so we can transform these objects automatically at deployment time.

R=sigmund@google.com

Committed: https://code.google.com/p/dart/source/detail?r=25326

[Jennifer Messerly, 2013-07-19 01:32:58]

Hey Siggi, mind taking a look?

fyi, some of my self-comments are on the patchset #1. patchset #2 has some
cleanups, mostly to comments.

https://codereview.chromium.org/19771010/diff/1/pkg/mdv/test/mdv_test_utils.dart
File pkg/mdv/test/mdv_test_utils.dart (right):

https://codereview.chromium.org/19771010/diff/1/pkg/mdv/test/mdv_test_utils.d...
pkg/mdv/test/mdv_test_utils.dart:47: void performMicrotaskCheckpoint() {
went with this name to match change_summary & MDV tests... should make our job
of staying in sync a tad easier.

https://codereview.chromium.org/19771010/diff/1/pkg/observe/lib/observe.dart
File pkg/observe/lib/observe.dart (left):

https://codereview.chromium.org/19771010/diff/1/pkg/observe/lib/observe.dart#...
pkg/observe/lib/observe.dart:239: void queueChangeRecords(void deliverChanges())
{
the global queueChangeRecords/deliverChangeRecords is gone, yay!

https://codereview.chromium.org/19771010/diff/1/pkg/observe/lib/src/change_no...
File pkg/observe/lib/src/change_notifier.dart (right):

https://codereview.chromium.org/19771010/diff/1/pkg/observe/lib/src/change_no...
pkg/observe/lib/src/change_notifier.dart:11: abstract class ChangeNotifier
extends Observable {
splitting Observable and ChangeNotifier into distinct implementations was the
key idea behind making automatic dirty-checking the default, while still easily
allowing observable objects that perform manual notification.

https://codereview.chromium.org/19771010/diff/1/pkg/observe/lib/src/change_no...
pkg/observe/lib/src/change_notifier.dart:28: typedef ChangeNotifierBase = Object
with ChangeNotifierMixin;
I'm not sure about the split here into
ChangeNotifier|ChangeNotifierBase|ChangeNotifierMixin, but kept for consistency
with Observable*. Definitely something we should revisit.

https://codereview.chromium.org/19771010/diff/1/pkg/observe/lib/src/change_no...
pkg/observe/lib/src/change_notifier.dart:36: abstract class ChangeNotifierMixin
implements ChangeNotifier {
this is essentially the old ObservableMixin impl

https://codereview.chromium.org/19771010/diff/1/pkg/observe/lib/src/change_re...
File pkg/observe/lib/src/change_record.dart (right):

https://codereview.chromium.org/19771010/diff/1/pkg/observe/lib/src/change_re...
pkg/observe/lib/src/change_record.dart:8: abstract class ChangeRecord {
no changes here yet. just moved into its own file

https://codereview.chromium.org/19771010/diff/2001/pkg/observe/lib/src/path_o...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/19771010/diff/2001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:25: class PathObserver extends
ChangeNotifierBase {
this is just a normal observable object now, instead of having a custom value
stream.

https://codereview.chromium.org/19771010/diff/2001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:32: List<Object> _values;
I completely redid this implementation using arrays instead of the linked list
of PropertyObservers. The old way had the downside of each link storing both its
object and cached value. Hopefully new impl is simpler and easier to follow!

https://codereview.chromium.org/19771010/diff/2001/pkg/observe/lib/src/to_obs...
File pkg/observe/lib/src/to_observable.dart (right):

https://codereview.chromium.org/19771010/diff/2001/pkg/observe/lib/src/to_obs...
pkg/observe/lib/src/to_observable.dart:8: * Converts the [Iterable] or [Map] to
an [ObservableList] or [ObservableMap],
no changes here, just moved to its own file

https://codereview.chromium.org/19771010/diff/2001/pkg/observe/lib/src/watche...
File pkg/observe/lib/src/watcher.dart (right):

https://codereview.chromium.org/19771010/diff/2001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:19: int get allObservablesCount =>
_allObservablesCount;
this is public so tests can get at it. it's not used much, but seems good to
know objects are being correctly removed from the watchers pool when no longer
observed.

[Siggi Cherem (dart-lang), 2013-07-22 21:47:22]

this is pretty awesome John!

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
File pkg/observe/lib/src/observable.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
pkg/observe/lib/src/observable.dart:28: * [deliverChanges] can be called to
force synchronousdelivery.
add space to split words: synchronous delivery

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
pkg/observe/lib/src/observable.dart:113: new Completer().completeError(e, s);
is the idea that this exception gets attached to the current zone, but keep
going until this loop finishes?

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
File pkg/observe/lib/src/observable_list.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
pkg/observe/lib/src/observable_list.dart:12: // TODO(jmesserly): remove
implements List<E> once we can extend ListBase<E>
remove todo =)?

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:33: List<Object> _segments;
make all 3 final?

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:233: return new RegExp('^$path\$',
caseSensitive: false);
now that A-Z is included above, do we still need 'caseSensitive: false'?

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
File pkg/observe/lib/src/watcher.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:19: int get allObservablesCount =>
_allObservablesCount;
could we simply use _allObservables.length here or you have this to make sure
you don't show numbers in between while you are cycling through the
dirty-checks?

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:56: anyChanged = anyChanged ||
observer.deliverChanges();
I thought you were going to deliverChanges regardless:

anyChanged = observer.deliverChanges() || anyChanged;

otherwise there is only 1 delivery per cycle?

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:62: // TODO(jmesserly): warn if limit is
reached?
+1, might be worth adding a simple print for now.

[Jennifer Messerly, 2013-07-22 21:55:11]

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
File pkg/observe/lib/src/watcher.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:19: int get allObservablesCount =>
_allObservablesCount;
On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> could we simply use _allObservables.length here or you have this to make sure
> you don't show numbers in between while you are cycling through the
> dirty-checks?

I think that's the idea, yeah. Keeps it consistently correct.

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:62: // TODO(jmesserly): warn if limit is
reached?
On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> +1, might be worth adding a simple print for now.

or logging? :)

[Siggi Cherem (dart-lang), 2013-07-22 21:57:25]

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
File pkg/observe/lib/src/watcher.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:62: // TODO(jmesserly): warn if limit is
reached?
On 2013/07/22 21:55:11, John Messerly wrote:
> On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> > +1, might be worth adding a simple print for now.
> 
> or logging? :)

sure =)

[Jennifer Messerly, 2013-07-22 22:38:39]

PTAL. New logging and a test for loop-in-dirty-checking

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
File pkg/observe/lib/src/observable.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
pkg/observe/lib/src/observable.dart:28: * [deliverChanges] can be called to
force synchronousdelivery.
On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> add space to split words: synchronous delivery

Done.

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
pkg/observe/lib/src/observable.dart:113: new Completer().completeError(e, s);
On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> is the idea that this exception gets attached to the current zone, but keep
> going until this loop finishes?

yeah, essentially if we end up running user code (e.g. a getter) it shouldn't
cause us to get in an inconsistent state.

BUT -- since we won't reach this spot with getters -- I'm not sure it's possible
for this to fail. I'll removed the try+catch and add a comment that we don't
expect this to execute user code.

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
File pkg/observe/lib/src/observable_list.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/observ...
pkg/observe/lib/src/observable_list.dart:12: // TODO(jmesserly): remove
implements List<E> once we can extend ListBase<E>
On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> remove todo =)?

Done.

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:33: List<Object> _segments;
On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> make all 3 final?

done for segments. it's hard for the other 2 as they're initialized during
constructor code.

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:233: return new RegExp('^$path\$',
caseSensitive: false);
On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> now that A-Z is included above, do we still need 'caseSensitive: false'?

Done. Good catch, I missed that in the updated code.

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
File pkg/observe/lib/src/watcher.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:56: anyChanged = anyChanged ||
observer.deliverChanges();
On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> I thought you were going to deliverChanges regardless:
> 
> anyChanged = observer.deliverChanges() || anyChanged;
> 
> otherwise there is only 1 delivery per cycle?

gooooood catch! Totally messed that up when refactoring it.

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/watche...
pkg/observe/lib/src/watcher.dart:62: // TODO(jmesserly): warn if limit is
reached?
On 2013/07/22 21:57:25, Siggi Cherem (dart-lang) wrote:
> On 2013/07/22 21:55:11, John Messerly wrote:
> > On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> > > +1, might be worth adding a simple print for now.
> > 
> > or logging? :)
> 
> sure =)

added logging. also it prints object index and toString, which hopefully helps
when in debugging.

[Siggi Cherem (dart-lang), 2013-07-22 23:06:14]

LGTM!

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:33: List<Object> _segments;
On 2013/07/22 22:38:39, John Messerly wrote:
> On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> > make all 3 final?
> 
> done for segments. it's hard for the other 2 as they're initialized during
> constructor code.

makes sense. One possibility here is to use a factory + private ctor... probably
not worth it tough =)

https://codereview.chromium.org/19771010/diff/17001/pkg/observe/lib/src/watch...
File pkg/observe/lib/src/watcher.dart (right):

https://codereview.chromium.org/19771010/diff/17001/pkg/observe/lib/src/watch...
pkg/observe/lib/src/watcher.dart:14: library observe.src.watcher;
minor suggestion: consider renaming this to dirty_check?

[Jennifer Messerly, 2013-07-23 02:45:14]

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
File pkg/observe/lib/src/path_observer.dart (right):

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:24: // TODO(jmesserly): find a better
home for this type.
removed this after noticing it is obsolete.

https://codereview.chromium.org/19771010/diff/9001/pkg/observe/lib/src/path_o...
pkg/observe/lib/src/path_observer.dart:33: List<Object> _segments;
On 2013/07/22 23:06:14, Siggi Cherem (dart-lang) wrote:
> On 2013/07/22 22:38:39, John Messerly wrote:
> > On 2013/07/22 21:47:22, Siggi Cherem (dart-lang) wrote:
> > > make all 3 final?
> > 
> > done for segments. it's hard for the other 2 as they're initialized during
> > constructor code.
> 
> makes sense. One possibility here is to use a factory + private ctor...
probably
> not worth it tough =)

yeah, it's not quite equivalent either, as a factory+private ctor makes it
impossible to extend the class, whereas a normal constructor doesn't. Then
again, one could make the argument that we should lock down PathObserver, as I'm
not sure the use case of extending it, other than to add some more convenience
APIs.

I'm inclined to just leave it for now, but it is sometihng thinking about
changing.

https://codereview.chromium.org/19771010/diff/17001/pkg/observe/lib/src/watch...
File pkg/observe/lib/src/watcher.dart (right):

https://codereview.chromium.org/19771010/diff/17001/pkg/observe/lib/src/watch...
pkg/observe/lib/src/watcher.dart:14: library observe.src.watcher;
On 2013/07/22 23:06:14, Siggi Cherem (dart-lang) wrote:
> minor suggestion: consider renaming this to dirty_check?

Done.

[Jennifer Messerly, 2013-07-23 02:48:28]

Committed patchset #4 manually as r25326 (presubmit successful).