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

fix(Dirty Checking): fix method detection and calling #986

Merged
merged 1 commit into from
May 6, 2014
Merged

fix(Dirty Checking): fix method detection and calling #986

merged 1 commit into from
May 6, 2014

Conversation

chirayuk
Copy link
Contributor

@chirayuk chirayuk commented May 1, 2014

  • run tests against StaticFieldGetterFactory
  • do not call the result of GetterFactory.method()
    in set object(value)
  • reduce the difference between the two (get rid of
    _MODE_METHOD_INVOKE_)

@chirayuk chirayuk added cla: yes and removed cla: no labels May 1, 2014
mhevery
mhevery reviewed May 1, 2014
View reviewed changes
lib/change_detection/dirty_checking_change_detector_static.dart
var getterFn = getters[name];
if (getterFn == null) throw "Missing method: $name";
var method = getterFn(object);
return (List args, Map namedArgs) => Function.apply(method, args, namedArgs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you made the function invocation slower using this change. This was done intentionally for speed.

@mhevery
Copy link
Contributor

mhevery commented May 1, 2014

I think this change has negative impact on performance. Let's talk more in person.

@chirayuk
Copy link
Contributor Author

chirayuk commented May 2, 2014

Cleaned up/unified more stuff.  Part of the unification was also needed for cases where folks map obj.fooMethod to a property on their component that's later invoked.

TODO:

  • Restore MODE_METHOD for fast path (unlike slower MODE_CLOSURE)
  • Add additional test case

NOTE:  I've updated dartbug 13523 on this issue.  We need this for dart2js tests to work properly.

@chirayuk
Copy link
Contributor Author

chirayuk commented May 2, 2014

Closes #999

mhevery
mhevery reviewed May 6, 2014
View reviewed changes
lib/change_detection/dirty_checking_change_detector.dart
@@ -479,12 +478,24 @@ class DirtyCheckingRecord<H> implements Record<H>, WatchRecord<H> {
return false;
case _MODE_GETTER_:
current = _getter(object);
if (_checkForVaryingClosure) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a problem. _checkForVaryingClosure is a field which will incur a field lookup, which is not free. Since MODE_GETTER is the most common mode, I don't think we should be making it slower by incurring additional cost.

Why do we need to do this check here? can't we do it in object setter? if not Can we have a new mode

case _MODE_GETTER_ONCE_:
  current = _getter(object);
  mode = _MODE_IDENTITY_

@mhevery
Copy link
Contributor

mhevery commented May 6, 2014

I am concerned about the extra field you introduced, both from performance as well as memory footprint. I think we can solve it without it. All else looks good.

@chirayuk
Copy link
Contributor Author

chirayuk commented May 6, 2014

Removed the extra field and checks.  Perf should be restored.

@chirayuk
Copy link
Contributor Author

chirayuk commented May 6, 2014

Tests in Travis queue – https://travis-ci.org/angular/angular.dart/builds/24565362

@chirayuk
fix(Dirty Checking): fix watching methods/closures
d71c7fa 
-  BUG: DynamicFieldGetterFactory::isMethod did not handle methods
   defined in superclasses.
-  BUG: Upon detecting a method, the code assumed that you would only
   invoke it.  This broke application code that watched a method (e.g.
   by way of Component mapping) just to get the closurized value and
   store it somewhere to invoke later (test case and stack trace at end
   of this commit message.)
-  BUG: StaticFieldGetterFactory::method() and
   DynamicFieldGetterFactory::method() differed.  There was no
   difference between StaticFieldGetterFactory::method() and
   StaticFieldGetterFactory::getter(). 
   DynamicFieldGetterFactory::method(), as mentioned before, assumed
   that the only thing you could do with it was to invoke it (i.e. not a
   leaf watch.)
-  There was very little testing for StaticFieldGetterFactory.  This
   meant that, though it was out of sync with DynamicFieldGetterFactory,
   no tests were failing.

Changes in this commit:

- run the same tests against StaticFieldGetterFactory that are run
  against DynamicFieldGetterFactory
- do not call the result of GetterFactory.method()
  in "set object(value)"
- reduce the difference between the two different factories. 
  GetterFactory now only has one method in it's interface definition -
  the getter function.  `_MODE_METHOD_INVOKE_`, `isMethod`,
  `isMethodInvoke`, etc. are gone.

**Bug Details:**

Refer to the repro case at
issue_999^...issue_999

```dart
// Given this object.
class Foo {
  bar(x) => x+1;
}

// This test case (in an appropriate file like `scope_spec.dart`) fails
// with a traceback

it('should watch closures', (RootScope rootScope, Logger log) {
  rootScope.context['foo'] = new Foo();
  rootScope.context['func'] = null;
  rootScope.watch('foo.bar', (v, _) { rootScope.context['func'] = v; });
  rootScope.watch('func(1)', (v, o) => log([v, o]));
  rootScope.apply();
  expect(log).toEqual([[null, null], [2, null]]);
});
```

**Stack Trace:**

    Chrome 34.0.1847 (Mac OS X 10.9.2) scope watch/digest should watch closures FAILED
    Test failed: Caught Closure call with mismatched arguments: function 'DynamicFieldGetterFactory.method.<anonymous closure>'

    NoSuchMethodError: incorrect number of arguments passed to method named 'DynamicFieldGetterFactory.method.<anonymous closure>'
    Receiver: Closure: (List, Map) => dynamic
    Tried calling: DynamicFieldGetterFactory.method.<anonymous closure>(Instance of 'Foo')
    Found: DynamicFieldGetterFactory.method.<anonymous closure>(args, namedArgs)
    #0      Object.noSuchMethod (dart:core-patch/object_patch.dart:45)
    #1      DirtyCheckingRecord.object= (package:angular/change_detection/dirty_checking_change_detector.dart:465:78)
    dart-archive#2      _FieldHandler.acceptValue (package:angular/change_detection/watch_group.dart:630:17)
    dart-archive#3      WatchGroup.addFieldWatch (package:angular/change_detection/watch_group.dart:171:29)
    dart-archive#4      FieldReadAST.setupWatch (package:angular/change_detection/ast.dart:67:31)
    dart-archive#5      WatchGroup.watch.<anonymous closure> (package:angular/change_detection/watch_group.dart:144:40)
    dart-archive#6      _HashMap.putIfAbsent (dart:collection-patch/collection_patch.dart:124)
    dart-archive#7      WatchGroup.watch (package:angular/change_detection/watch_group.dart:143:27)
    dart-archive#8      Scope.watch (package:angular/core/scope.dart:240:31)
    dart-archive#9      main.<anonymous closure>.<anonymous closure>.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1308:24)
    dart-archive#10     _LocalInstanceMirror._invoke (dart:mirrors-patch/mirrors_impl.dart:440)
    dart-archive#11     _LocalInstanceMirror.invoke (dart:mirrors-patch/mirrors_impl.dart:436)
    dart-archive#12     _LocalClosureMirror.apply (dart:mirrors-patch/mirrors_impl.dart:466)
    dart-archive#13     DynamicInjector.invoke (package:di/dynamic_injector.dart:97:20)
    dart-archive#14     _SpecInjector.inject (package:angular/mock/test_injection.dart:58:22)
    dart-archive#15     inject.<anonymous closure> (package:angular/mock/test_injection.dart:100:44)
    dart-archive#16     _rootRun (dart:async/zone.dart:723)
    dart-archive#17     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#18     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#19     runZoned (dart:async/zone.dart:954)
    dart-archive#20     _syncOuter.<anonymous closure> (package:angular/mock/zone.dart:227:22)
    dart-archive#21     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:13:14)
    dart-archive#22     _run.<anonymous closure> (package:unittest/src/test_case.dart:102:27)
    dart-archive#23     _rootRunUnary (dart:async/zone.dart:730)
    dart-archive#24     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#25     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    dart-archive#26     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:488)
    dart-archive#27     _Future._propagateToListeners (dart:async/future_impl.dart:571)
    dart-archive#28     _Future._completeWithValue (dart:async/future_impl.dart:331)
    dart-archive#29     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:393)
    dart-archive#30     _rootRun (dart:async/zone.dart:723)
    dart-archive#31     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#32     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#33     _BaseZone.runGuarded (dart:async/zone.dart:574)
    dart-archive#34     _BaseZone.bindCallback.<anonymous closure> (dart:async/zone.dart:599)
    dart-archive#35     _asyncRunCallbackLoop (dart:async/schedule_microtask.dart:23)
    dart-archive#36     _asyncRunCallback (dart:async/schedule_microtask.dart:32)
    dart-archive#37     _handleMutation (file:///Volumes/data/b/build/slave/dartium-mac-full-dev/build/src/dart/tools/dom/src/native_DOMImplementation.dart:588)

    DECLARED AT:#0      inject (package:angular/mock/test_injection.dart:97:5)
    #1      _injectify (/Users/chirayu/work/angular.dart/test/_specs.dart:236:25)
    dart-archive#2      iit (/Users/chirayu/work/angular.dart/test/_specs.dart:244:53)
    dart-archive#3      main.<anonymous closure>.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1305:10)
    dart-archive#4      describe.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:62:9)
    dart-archive#5      group (package:unittest/unittest.dart:396:9)
    dart-archive#6      describe (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:60:15)
    dart-archive#7      describe (/Users/chirayu/work/angular.dart/test/_specs.dart:248:46)
    dart-archive#8      main.<anonymous closure> (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:1009:13)
    dart-archive#9      describe.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:62:9)
    dart-archive#10     group (package:unittest/unittest.dart:396:9)
    dart-archive#11     describe (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:60:15)
    dart-archive#12     describe (/Users/chirayu/work/angular.dart/test/_specs.dart:248:46)
    dart-archive#13     main (/Users/chirayu/work/angular.dart/test/core/scope_spec.dart:14:11)
    dart-archive#14     main.<anonymous closure> (http://localhost:8765/__adapter_dart_unittest.dart:169:15)
    dart-archive#15     _rootRun (dart:async/zone.dart:723)
    dart-archive#16     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#17     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#18     runZoned (dart:async/zone.dart:954)
    dart-archive#19     main (http://localhost:8765/__adapter_dart_unittest.dart:146:11)

    #0      _SpecInjector.inject (package:angular/mock/test_injection.dart:60:7)
    #1      inject.<anonymous closure> (package:angular/mock/test_injection.dart:100:44)
    dart-archive#2      _rootRun (dart:async/zone.dart:723)
    dart-archive#3      _rootRun (dart:async/zone.dart:724)
    dart-archive#4      _rootRun (dart:async/zone.dart:724)
    dart-archive#5      _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#6      _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#7      runZoned (dart:async/zone.dart:954)
    dart-archive#8      _syncOuter.<anonymous closure> (package:angular/mock/zone.dart:227:22)
    dart-archive#9      _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:13:14)
    dart-archive#10     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:14:5)
    dart-archive#11     _withSetup.<anonymous closure> (/Users/chirayu/work/angular.dart/test/jasmine_syntax.dart:14:5)
    dart-archive#12     _run.<anonymous closure> (package:unittest/src/test_case.dart:102:27)
    dart-archive#13     _rootRunUnary (dart:async/zone.dart:730)
    dart-archive#14     _ZoneDelegate.runUnary (dart:async/zone.dart:462)
    dart-archive#15     _CustomizedZone.runUnary (dart:async/zone.dart:667)
    dart-archive#16     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:488)
    dart-archive#17     _Future._propagateToListeners (dart:async/future_impl.dart:571)
    dart-archive#18     _Future._completeWithValue (dart:async/future_impl.dart:331)
    dart-archive#19     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:393)
    dart-archive#20     _rootRun (dart:async/zone.dart:723)
    dart-archive#21     _ZoneDelegate.run (dart:async/zone.dart:453)
    dart-archive#22     _CustomizedZone.run (dart:async/zone.dart:663)
    dart-archive#23     _BaseZone.runGuarded (dart:async/zone.dart:574)
    dart-archive#24     _BaseZone.bindCallback.<anonymous closure> (dart:async/zone.dart:599)
    dart-archive#25     _asyncRunCallbackLoop (dart:async/schedule_microtask.dart:23)
    dart-archive#26     _asyncRunCallback (dart:async/schedule_microtask.dart:32)
    dart-archive#27     _handleMutation (file:///Volumes/data/b/build/slave/dartium-mac-full-dev/build/src/dart/tools/dom/src/native_DOMImplementation.dart:588)

Closes dart-archive#999
@jbdeboer jbdeboer merged commit d71c7fa into dart-archive:master May 6, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chirayuk @mhevery @jbdeboer