feat(dccd):ObservableList / ObservableMap & Observable

[vicb]

For now this is a first batch of changes for Observable support to use notifications for ObservableList & ObservableMap

Things missing from this PR:

• Test switching back & forth from List to ObservableList (same for Map) - pending on [dccd] Changing ref to Iterables cause an exception #1158
• Test the canceling of stream subscription on record & group removal

The code is not expected to change so the PR is ready for review (only tests should be added)

Next steps (further PRs):

• Use notifications for obj.fields for objects,
• Investigate using two separate lists (notified, unnotified)

[vicb]

I have added some more commits to this PR, the latest one has an implementation for field notification.

It works in mirrors mode, not in transformed mode yet - there seems to be an issue with the analyzer version required for observe vs the one required for angular, di and some other core pkgs.

Next steps:

• Try to solve the issue with analyzer & transformer (at least where I can, angular & DI),
• Plug the smoke & observe transformers to generate / update the code,
• Investigate whether we can mix @observable and non-@observable field in an Observable object. non-@observable should fallback to field getters but this implies having access to the annotations.

I would also like to update the code for my 2nd TODO item given in the PR header.
One of the solution I can imagine for that is to refactor the current code:

• set the record _mode to null when a record a removed (either single record or batch remove) - this would be done in an assert to keep top perfs,
• throw in check() when the _mode is unknown - instead of an assert(false) currently.

I think this would allow easier testing.

[vicb]

This does not work on transformed code (it would require to write a custom transformer, the polymer script_compactor could be used as a starting point).

An easy option is to dirty check all fields when a Notification is triggered on an object (we would not get as much as by checking the field).

[vicb]

I have just updated the PR by adding:

• The Observable transformer, to allow using @observable on a field - the transformed code will make use of a ChangeNotifier
• A smoke transformer that allow retrieving Symbols on transformed code

The code works. The things left to do:

• polish the smoke transformer & write tests,
• write test for cancelling notifications

[vicb]

Tests have been added (and yes, that's true, what you don't test is broken)

The PR should be ready to go if Travis is happy

[vicb]

the notification shoule be added here - that's not important but more correct

[vicb]

Some idea to create a benchmark:

• Observe 10 fields on an object. How faster (vs dccd) is it when
  • no fields change,
  • a single field change (there is room for improvement after this PR: by not calling the getter),
  • all fields change
• Benchmark group removal (with some child groups) - this is expected to be slower than before because we have to walk all the child group to remove the listeners
• For lists / maps, benchmark:
  • no change
  • change (this is expected to be slower as we will run both the observe change detection and the dccd change detection - the benchmark could help deciding it it would be worth using the output of the observe cd after transformation)

Some gotchas:

• It is not yet possible to mix @observable field and non-observable fields in an Observable object (the non-observable will be considered as never changed - should be easy to implement with the smoke support, need some discussion though),
• The benchmarks must not be done on Observable object but on classes extending ChangeNotifier. Extending Observable directly will result in running the observe dirty check loop and will be much slower. The best approach is probably to transform our benchmark code before running it. An acceptable simple approach is to execute the benchmark on transformed class (see the smoke_generator_spec to get an example).

[vicb]

@tbosch the code is now stable, ready to be benchmarked
@chirayuk any update on the issue ? (see PR header)

[tbosch]

Hi @vicb,
I am trying to do a performance benchmark for adding/removing a WatchGroup. However, the version with observables has a memory leak. I use the following test below. If I run this test with _groupRemoval(observable:false) it exits normally. If I run it with _groupRemoval(observable:true) it gives me a heap error.

I started to debug this with Observatory and added test code that runs one test loop controlled by hitting the key on the keyboard. I am not so firm in Dart and the change detection code to find the problem, could you take a look?

_groupRemoval({bool observable}) {
  var description = '';
  if (observable) {
    description += 'observable';
  } else {
    description += 'dc';
  }
  var rootWatchGrp = new RootWatchGroup(_staticFieldGetterFactory,
        new DirtyCheckingChangeDetector(_staticFieldGetterFactory), {});
  var testRun = () {
    for (int i=0; i<10; i++) {
          WatchGroup child = rootWatchGrp.newGroup(observable ? new _ObservableObj() : new _Obj())
                ..watch(_parse('a'), _reactionFn)
                ..watch(_parse('b'), _reactionFn)
                ..watch(_parse('c'), _reactionFn)
                ..watch(_parse('d'), _reactionFn)
                ..watch(_parse('e'), _reactionFn)
                ..watch(_parse('f'), _reactionFn)
                ..watch(_parse('g'), _reactionFn)
                ..watch(_parse('h'), _reactionFn)
                ..watch(_parse('i'), _reactionFn)
                ..watch(_parse('j'), _reactionFn);      
          child.remove();          
        }
  };
  /* Uncomment this to run the testRuns controlled by hitting enter on the console
  var stream = stdin.transform(UTF8.decoder).transform(new LineSplitter());
  StreamSubscription cmdSubscription;
  cmdSubscription = stream.listen((line) { 
     if (line == 'exit') {
       cmdSubscription.cancel();
     } else {
       print('next loop');
       testRun();
     } 
    });
  */
  time(description+' add/remove 10 watchGroups with 20 watches', testRun);
}

[tbosch]

@vicb Here is a commit that adds the performance tests you mentioned: tbosch@6b93596

However, as already said, some of them crash with a heap error when used with observables (I commented those out): add/remove WatchGroups and change arrays.

[vicb]

@tbosch thanks for the benchmark, I'll have a look on next Monday

[vicb]

@tbosch

• I have cherry picked your commit in this branch. The code has also been modified to actually notify the change detection (by calling notifyPropertyChange()) without this code the bench would be much faster as no changes would ever be processed,
• I have reduced the number of iterations to prevent heap overflow,
• feel free to PR this branch on my repo for further updates.

@mhevery I have updated _perf.dart to reduce the number of iterations in order to prevent heap overflow. The code has also been modified to return more accurate measures: for loop count > 100, we would measure n * 100 function calls but use the unmodified count for the perf report (ie for 234 iterations, we would run the functions 200 times but divide the total time by 234). Could you please take a look the commit ?

Reducing the number of iterations increase the stdev:

[DIRTY CHECK] 0/20 change List: => 7,434,418 ops/sec (0 us) stdev(0.31694)
[CHANGE NOTIFIER] 0/20 change ObservableList: => 21,523,279 ops/sec (0 us) stdev(0.52509)
[DIRTY CHECK] 1/20 change List: => 1,953,144 ops/sec (1 us) stdev(0.07864)
[CHANGE NOTIFIER] 1/20 change ObservableList: => 11,237,093 ops/sec (0 us) stdev(0.54805)

[tbosch]

Ok, regarding the memory problems:

For adding/removing groups we need to do a rootWatchGrp.detectChanged() after every loop (independent of whether observables are used or not).

For _collectionRead and _fieldRead there is a known bug in Dart that leads to this memory problem when using observables, which is already fixed inside of Google but not yet public.

@vicb Could you add the call to rootWatchGrp.detectChanged() inside of watch_group_perf.darf/_groupAddRemove/testRun?

[vicb]

@tbosch is the last "fixup" commit what you need ? The commit "Update _perf to prevent heap overflow" is still required to prevent memory errors after that.

[vicb]

I have updated the PR to remove all the things related to smoke and to add an important comment:

You should include the following lines in your pubspec.yaml

    dependency_overrides:
      analyzer: '>=0.15.6 <0.16.0'

This is required because the observe package depends on analyzer:
'>=0.15.6 <0.16.0'
while all SDK packages have not yet been updated to depends on this
version.

[jbdeboer]

There hasn't been much activity on this PR. @vicb is it blocked on something? What are the next steps?

It looks like we should have the Dart team tackle that observe dependency problem, though, yes? It looks like the observe package was just updated last week, so we are probably good to go..

[vicb]

The PR is ready (some commits should be squashed).

There is still an "issue" with the dependencies as observe which is required by this PR depends on a very recent version of analyzer. Angular and di depends on an older version, this we can fix. However some core pkgs (Intl, ...) also depends on an older version.

Getting the dart team to publish a changelog for the analyzer and to make sure that the core pkgs are compatible (dependency wise) would really help. However if the analyzer keeps breaking BC (I think the latest version is now 0.18) there is nothing much we can do. (Users are starting to complain about this dependency issue).

If we merge this PR as it is, users will have to add an override in their pubspec.

[vicb]

@mhevery Could you please double-check chore(_perf.dart): lower the target time to prevent a memory overflow - we had to lower the target time to prevent a memory overflow (due to a bug in Dart).

If you're ok with this update, the PR is rebased and can be merged

[mhevery]

I thought this was merged. No?

[vicb]

@rkirov has a rebased version, there were some problems that should be fixed now.

[rkirov]

Rebased and will be merged as #1419