Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

Commit 24c36d3

Browse files
gkalpakgkalpak
committed
fix(scopes): correctly meassure perf impact of one-time expressions
Previously, one-time expressions were supposed to not trigger perf timers. There were a couple of issues with the implementation: 1. One-time expressions were not detected as such if there was whitespace before the `::`. 2. It depended on internal AngularJS implementation details in a way that would fail to identify certain usecases (e.g. multiple interpolations in a single attribute value or node text). 3. Time spent on the initial evaluation of the one-time expressions would not be accounted for, but could still impact the duration of the first `$digest`. 4. While it is not common, there are cases were one-time expressions don't settle immediately. In such cases, their impact on the `$digest` should not be ignored. This commit fixes the above issues by ensuring that only the "final" call to `$watch` is intercepted (in contrast to intermediate calls that might call a `$$watchDelegate`, which will then create the actual `$watch`er). One-time expressions are intercepted normally and stop triggering perf times as soon as their `unwatch` function is called. Although the dependency on internal AngularJS implementation details could not be avoided, the new implementation is a little more robust, as it relies on fewer and less brittle details :D Fixes #109 Closes #122 Closes #124
1 parent a24fe1a commit 24c36d3

File tree

3 files changed

+188
-159
lines changed

3 files changed

+188
-159
lines changed

Diff for: ‎dist/hint.js

+37-66
Original file line numberDiff line numberDiff line change
@@ -1634,51 +1634,20 @@ function decorateRootScope($delegate, $parse) {
16341634
var _digestEvents = [];
16351635
var skipNextPerfWatchers = false;
16361636
scopePrototype.$watch = function (watchExpression, reactionFunction) {
1637-
// if `skipNextPerfWatchers` is true, this means the previous run of the
1638-
// `$watch` decorator was a one time binding expression and this invocation
1639-
// of the $watch function has the `oneTimeInterceptedExpression` (internal angular function)
1640-
// as the `watchExpression` parameter. If we decorate it with the performance
1641-
// timers function this will cause us to invoke `oneTimeInterceptedExpression`
1642-
// on subsequent digest loops and will update the one time bindings
1643-
// if anything mutated the property.
1644-
if (skipNextPerfWatchers) {
1645-
skipNextPerfWatchers = false;
1646-
return _watch.apply(this, arguments);
1647-
}
1648-
1649-
if (typeof watchExpression === 'string' &&
1650-
isOneTimeBindExp(watchExpression)) {
1651-
skipNextPerfWatchers = true;
1652-
return _watch.apply(this, arguments);
1653-
}
1654-
var watchStr = humanReadableWatchExpression(watchExpression);
1655-
var scopeId = this.$id;
1656-
var expressions = null;
1657-
if (typeof watchExpression === 'function') {
1658-
expressions = watchExpression.expressions;
1659-
if (Object.prototype.toString.call(expressions) === '[object Array]' &&
1660-
expressions.some(isOneTimeBindExp)) {
1661-
skipNextPerfWatchers = true;
1662-
return _watch.apply(this, arguments);
1663-
}
1664-
1665-
arguments[0] = function () {
1666-
var start = perf.now();
1667-
var ret = watchExpression.apply(this, arguments);
1668-
var end = perf.now();
1669-
_digestEvents.push({
1670-
eventType: 'scope:watch',
1671-
id: scopeId,
1672-
watch: watchStr,
1673-
time: end - start
1674-
});
1675-
return ret;
1676-
};
1677-
} else {
1678-
var thatScope = this;
1679-
arguments[0] = function () {
1637+
// Convert the `watchExpression` to a function (if not already one).
1638+
// This is also the first thing `Scope.$watch()` does.
1639+
var parsedExpression = $parse(watchExpression);
1640+
1641+
// Only intercept this call if there is no `$$watchDelegate`.
1642+
// (With `$$watchDelegate` there will be subsequent calls to `$watch` (if necessary)).
1643+
if (!parsedExpression.$$watchDelegate) {
1644+
var scopeId = this.$id;
1645+
var watchStr = humanReadableWatchExpression(watchExpression);
1646+
1647+
// Intercept the `watchExpression` (if any).
1648+
arguments[0] = simpleExtend(function() {
16801649
var start = perf.now();
1681-
var ret = thatScope.$eval(watchExpression);
1650+
var ret = parsedExpression.apply(this, arguments);
16821651
var end = perf.now();
16831652
_digestEvents.push({
16841653
eventType: 'scope:watch',
@@ -1687,22 +1656,23 @@ function decorateRootScope($delegate, $parse) {
16871656
time: end - start
16881657
});
16891658
return ret;
1690-
};
1691-
}
1692-
1693-
if (typeof reactionFunction === 'function') {
1694-
arguments[1] = function () {
1695-
var start = perf.now();
1696-
var ret = reactionFunction.apply(this, arguments);
1697-
var end = perf.now();
1698-
_digestEvents.push({
1699-
eventType: 'scope:reaction',
1700-
id: scopeId,
1701-
watch: watchStr,
1702-
time: end - start
1703-
});
1704-
return ret;
1705-
};
1659+
}, parsedExpression);
1660+
1661+
// Intercept the `reactionFunction` (if any).
1662+
if (typeof reactionFunction === 'function') {
1663+
arguments[1] = function() {
1664+
var start = perf.now();
1665+
var ret = reactionFunction.apply(this, arguments);
1666+
var end = perf.now();
1667+
_digestEvents.push({
1668+
eventType: 'scope:reaction',
1669+
id: scopeId,
1670+
watch: watchStr,
1671+
time: end - start
1672+
});
1673+
return ret;
1674+
};
1675+
}
17061676
}
17071677

17081678
return _watch.apply(this, arguments);
@@ -1910,14 +1880,15 @@ function humanReadableWatchExpression (fn) {
19101880
return fn.toString();
19111881
}
19121882

1913-
function isOneTimeBindExp(exp) {
1914-
// this is the same code angular 1.3.15 has to check
1915-
// for a one time bind expression
1916-
return exp.charAt(0) === ':' && exp.charAt(1) === ':';
1917-
}
1918-
19191883
function convertIdToOriginalType(scopeId) {
19201884
return (angular.version.minor < 3) ? scopeId : parseInt(scopeId, 10);
19211885
}
19221886

1887+
function simpleExtend(dst, src) {
1888+
Object.keys(src).forEach(function(key) {
1889+
dst[key] = src[key];
1890+
});
1891+
return dst;
1892+
}
1893+
19231894
},{"../lib/summarize-model":7,"debounce-on":2}]},{},[1]);

Diff for: ‎src/modules/scopes.js

+37-66
Original file line numberDiff line numberDiff line change
@@ -79,37 +79,20 @@ function decorateRootScope($delegate, $parse) {
7979
var _digestEvents = [];
8080
var skipNextPerfWatchers = false;
8181
scopePrototype.$watch = function (watchExpression, reactionFunction) {
82-
// if `skipNextPerfWatchers` is true, this means the previous run of the
83-
// `$watch` decorator was a one time binding expression and this invocation
84-
// of the $watch function has the `oneTimeInterceptedExpression` (internal angular function)
85-
// as the `watchExpression` parameter. If we decorate it with the performance
86-
// timers function this will cause us to invoke `oneTimeInterceptedExpression`
87-
// on subsequent digest loops and will update the one time bindings
88-
// if anything mutated the property.
89-
if (skipNextPerfWatchers) {
90-
skipNextPerfWatchers = false;
91-
return _watch.apply(this, arguments);
92-
}
93-
94-
if (typeof watchExpression === 'string' &&
95-
isOneTimeBindExp(watchExpression)) {
96-
skipNextPerfWatchers = true;
97-
return _watch.apply(this, arguments);
98-
}
99-
var watchStr = humanReadableWatchExpression(watchExpression);
100-
var scopeId = this.$id;
101-
var expressions = null;
102-
if (typeof watchExpression === 'function') {
103-
expressions = watchExpression.expressions;
104-
if (Object.prototype.toString.call(expressions) === '[object Array]' &&
105-
expressions.some(isOneTimeBindExp)) {
106-
skipNextPerfWatchers = true;
107-
return _watch.apply(this, arguments);
108-
}
109-
110-
arguments[0] = function () {
82+
// Convert the `watchExpression` to a function (if not already one).
83+
// This is also the first thing `Scope.$watch()` does.
84+
var parsedExpression = $parse(watchExpression);
85+
86+
// Only intercept this call if there is no `$$watchDelegate`.
87+
// (With `$$watchDelegate` there will be subsequent calls to `$watch` (if necessary)).
88+
if (!parsedExpression.$$watchDelegate) {
89+
var scopeId = this.$id;
90+
var watchStr = humanReadableWatchExpression(watchExpression);
91+
92+
// Intercept the `watchExpression` (if any).
93+
arguments[0] = simpleExtend(function() {
11194
var start = perf.now();
112-
var ret = watchExpression.apply(this, arguments);
95+
var ret = parsedExpression.apply(this, arguments);
11396
var end = perf.now();
11497
_digestEvents.push({
11598
eventType: 'scope:watch',
@@ -118,36 +101,23 @@ function decorateRootScope($delegate, $parse) {
118101
time: end - start
119102
});
120103
return ret;
121-
};
122-
} else {
123-
var thatScope = this;
124-
arguments[0] = function () {
125-
var start = perf.now();
126-
var ret = thatScope.$eval(watchExpression);
127-
var end = perf.now();
128-
_digestEvents.push({
129-
eventType: 'scope:watch',
130-
id: scopeId,
131-
watch: watchStr,
132-
time: end - start
133-
});
134-
return ret;
135-
};
136-
}
137-
138-
if (typeof reactionFunction === 'function') {
139-
arguments[1] = function () {
140-
var start = perf.now();
141-
var ret = reactionFunction.apply(this, arguments);
142-
var end = perf.now();
143-
_digestEvents.push({
144-
eventType: 'scope:reaction',
145-
id: scopeId,
146-
watch: watchStr,
147-
time: end - start
148-
});
149-
return ret;
150-
};
104+
}, parsedExpression);
105+
106+
// Intercept the `reactionFunction` (if any).
107+
if (typeof reactionFunction === 'function') {
108+
arguments[1] = function() {
109+
var start = perf.now();
110+
var ret = reactionFunction.apply(this, arguments);
111+
var end = perf.now();
112+
_digestEvents.push({
113+
eventType: 'scope:reaction',
114+
id: scopeId,
115+
watch: watchStr,
116+
time: end - start
117+
});
118+
return ret;
119+
};
120+
}
151121
}
152122

153123
return _watch.apply(this, arguments);
@@ -355,12 +325,13 @@ function humanReadableWatchExpression (fn) {
355325
return fn.toString();
356326
}
357327

358-
function isOneTimeBindExp(exp) {
359-
// this is the same code angular 1.3.15 has to check
360-
// for a one time bind expression
361-
return exp.charAt(0) === ':' && exp.charAt(1) === ':';
362-
}
363-
364328
function convertIdToOriginalType(scopeId) {
365329
return (angular.version.minor < 3) ? scopeId : parseInt(scopeId, 10);
366330
}
331+
332+
function simpleExtend(dst, src) {
333+
Object.keys(src).forEach(function(key) {
334+
dst[key] = src[key];
335+
});
336+
return dst;
337+
}

Diff for: ‎test/scopes.spec.js

+114-27
Original file line numberDiff line numberDiff line change
@@ -72,33 +72,120 @@ describe('ngHintScopes', function() {
7272
});
7373

7474
if (angular.version.minor >= 3) {
75-
it('should not run perf timers for one time bind expressions passed to watch', function() {
76-
var calls = hint.emit.calls;
77-
scope.$watch('::a.b', function() {});
78-
expect(calls.count()).toBe(0);
79-
80-
scope.$apply();
81-
var evt = calls.mostRecent().args[1].events[0];
82-
expect(calls.count()).toBe(1);
83-
expect(evt).toBeUndefined();
84-
});
85-
86-
it('should not run perf timers for one time template bindings', function() {
87-
var elt = angular.element(
88-
'<div>' +
89-
'<span>{{::a}}</span>' +
90-
'<button ng-click="a = \'bar\'">Set</button>' +
91-
'</div>'
92-
);
93-
scope.a = 'foo';
94-
var view = $compile(elt)(scope);
95-
scope.$apply();
96-
var $binding = view.find('span');
97-
var $button = view.find('button');
98-
99-
$button.triggerHandler('click');
100-
scope.$apply();
101-
expect($binding.text()).toBe('foo');
75+
describe('one-time expressions', function() {
76+
// Helpers
77+
function getDigestCallArgs() {
78+
var allArgs = hint.emit.calls.allArgs();
79+
var digestCallArgs = allArgs.filter(function(args) {
80+
return args[0] === 'scope:digest';
81+
});
82+
83+
return digestCallArgs;
84+
}
85+
86+
function countWatchEvents(args) {
87+
var events = args[1].events;
88+
var watchEvents = events.filter(function(evt) {
89+
return evt.eventType === 'scope:watch';
90+
});
91+
92+
return watchEvents.length;
93+
}
94+
95+
it('should correctly trigger perf timers when passed to `$watch`', function() {
96+
var calls = hint.emit.calls;
97+
var args;
98+
99+
var reactions = [0, 0, 0];
100+
var exps = [
101+
'::c.d',
102+
' ::c.d ',
103+
' :: c.d '
104+
].forEach(function(exp, idx) {
105+
scope.$watch(exp, function(v) { reactions[idx]++; });
106+
});
107+
108+
expect(calls.count()).toBe(0);
109+
110+
scope.$apply();
111+
args = getDigestCallArgs();
112+
expect(args.length).toBe(1);
113+
expect(countWatchEvents(args[0])).toBe(6); // Initial $digest: 2 loops
114+
expect(reactions).toEqual([1, 1, 1]); // Initial $digest always calls listeners
115+
116+
calls.reset();
117+
scope.$apply();
118+
args = getDigestCallArgs();
119+
expect(args.length).toBe(1);
120+
expect(countWatchEvents(args[0])).toBe(3); // No change: 1 loop
121+
expect(reactions).toEqual([1, 1, 1]); // No change: listeners not called
122+
123+
calls.reset();
124+
scope.$apply('c.d = "foo"');
125+
args = getDigestCallArgs();
126+
expect(args.length).toBe(1);
127+
expect(countWatchEvents(args[0])).toBe(6); // First change: 2 loops
128+
expect(reactions).toEqual([2, 2, 2]); // First change to defined value calls listeners
129+
130+
calls.reset();
131+
scope.$apply('c.d = "bar"');
132+
args = getDigestCallArgs();
133+
expect(args.length).toBe(1);
134+
expect(countWatchEvents(args[0])).toBe(0); // Already settled: 0 loops
135+
expect(reactions).toEqual([2, 2, 2]); // Already settled: listeners not called
136+
});
137+
138+
it('should correctly trigger perf timers when used in template bindings', function() {
139+
var calls = hint.emit.calls;
140+
var args;
141+
142+
$compile(
143+
'<div>' +
144+
// Interpolation in node text: 6 bindings (1 + 1 + 1 + 3)
145+
'<span>{{::c.d}}</span>' +
146+
'<span>{{ ::c.d }}</span>' +
147+
'<span>{{ :: c.d }}</span>' +
148+
'<span>{{::c.d}}{{ ::c.d }}{{ :: c.d }}</span>' +
149+
150+
// Interpolation in attribute value: 6 bindings (1 + 1 + 1 + 3)
151+
'<span class="{{::c.d}}"></span>' +
152+
'<span class="{{ ::c.d }}"></span>' +
153+
'<span class="{{ :: c.d }}"></span>' +
154+
'<span class="{{::c.d}}{{ ::c.d }}{{ :: c.d }}"></span>' +
155+
156+
// Expressions: 3 bindings (1 + 1 + 1)
157+
'<span ng-if="::c.d"></span>' +
158+
'<span ng-if=" ::c.d "></span>' +
159+
'<span ng-if=" :: c.d "></span>' +
160+
161+
// Total: 15 watchers (6 + 6 + 3)
162+
'</div>'
163+
)(scope);
164+
165+
calls.reset();
166+
scope.$apply();
167+
args = getDigestCallArgs();
168+
expect(args.length).toBe(1);
169+
expect(countWatchEvents(args[0])).toBe(30); // Initial $digest: 2 loops
170+
171+
calls.reset();
172+
scope.$apply();
173+
args = getDigestCallArgs();
174+
expect(args.length).toBe(1);
175+
expect(countWatchEvents(args[0])).toBe(15); // No change: 1 loop
176+
177+
calls.reset();
178+
scope.$apply('c.d = "foo"');
179+
args = getDigestCallArgs();
180+
expect(args.length).toBe(1);
181+
expect(countWatchEvents(args[0])).toBe(30); // First change: 2 loops
182+
183+
calls.reset();
184+
scope.$apply('c.d = "bar"');
185+
args = getDigestCallArgs();
186+
expect(args.length).toBe(1);
187+
expect(countWatchEvents(args[0])).toBe(0); // Already settled: 0 loops
188+
});
102189
});
103190
}
104191

0 commit comments

Comments
 (0)