perf(ngModel,ngForm): change ng-model/form controllers to use prototype methods #13286
Conversation
| element.on('blur', function(ev) { | ||
| if (modelCtrl.$touched) return; | ||
|
|
||
| if ($rootScope.$$phase) { | ||
| scope.$evalAsync(modelCtrl.$setTouched); |
There was a problem hiding this comment.
This is an example of the breaking change...
There was a problem hiding this comment.
Is it possible to add un-minified only checks to aid in the transition for these methods?
console.assert(this instanceof NgModelController, 'Some link to the breaking change message');
It's not worth these checks in the minified build imo since those should be the production assets, but it may be worth it in the debug/unminified builds.
|
I think if communicated well, this change is worth the break. For complex apps, the number of NgModelControllers being constructed and torn down on a page can represent a non-trivial amount of memory consumption and GC hits (as your tests show) Out of curiosity, would this allow engines like v8 to better optimize this controller now that it uses a real prototype, or would have the similarly structured instances been backed by the same hidden class anyway? |
|
I like it, now and then I wonder why it wasn't done like this before. I cannot estimate how many apps will be affected by the breaking change, but I think the perf gains are worth it. |
|
|
||
| if (control.$name) { | ||
| form[control.$name] = control; | ||
| this[control.$name] = control; |
There was a problem hiding this comment.
I know this is unrelated to this change, but this looks dangerous
|
Overall, I like this PR. It does need a better commit message and a proper breaking change description and migration |
|
@lgalfaso Quick thought; right now, we have access to both That should make it easier to migrate to 1.6 with this breaking change since if you don't get these warnings, it's relatively certain that you won't run into problems. |
jbedard
force-pushed
the
ngmodel-prototype
branch
2 times, most recently
from
40b47d3 to
dd09c2c
Compare
jbedard
force-pushed
the
ngmodel-prototype
branch
from
dd09c2c to
b2814db
Compare
|
I've rebased this... |
jbedard
force-pushed
the
ngmodel-prototype
branch
from
b2814db to
6b24837
Compare
|
Rebased again. Let me know if there's anything I can do to help get this into 1.6... |
|
@jbedard I think we should merge it. You've never responded to #13286 (comment), do you know what @lgalfaso means by this? @dcherman How can we log something in the unminfied build and have it removed in the minified buld? Can we use the Closure compiler for this? |
|
@Narretz I'm not that familiar with what Closure Compiler offers in the way of options, but UglifyJS has a FWIW, my idea needed to be landed before this change in order to be effective. You could stillassert something like |
|
@Narretz he is referring to the fact that we are blindly setting @dcherman I'd rather not add debug-only code like that because many people who use their own (or no) minification won't remove it. An opt-in tool might work, but then no one will use it :| |
|
We've decided that we are not gonna log any warning messages in the console before this change. It's an old-fashioned BC and just have to deal with it (TM) during migration. |
|
(I have rebase this locally, so no need to rebase it again. Just waiting if someone has any last thoughts on this) |
BREAKING CHANGE:
The use of prototype methods instead of new methods per instance removes the ability to pass NgModelController and FormController methods without context.
For example `$scope.$watch("something", myNgModelCtrl.$render)` will no longer work because the `$render` method is passed without any context. This must now be replace with `$scope.$watch("something", function() { myNgModelCtrl.$render(); })` or possibly by using `Function.prototype.bind` or `angular.bind`.
jbedard
force-pushed
the
ngmodel-prototype
branch
from
6b24837 to
2bc5980
Compare
|
I also just rebased this! 👍 for the BC without any extra warnings |
|
👏 |
This makes the largetable-bp ng-model benchmarks 10-15% faster (down 90-100ms for me). The actual controller instantiation doesn't change too much but the overall numbers seem consistently faster, I assume all due to reducing memory usage / gc. Specifically on creation there is ~40% less memory GCed, on destruction there is about ~25% less.
Looking at the heap snapshot this is entirely from having less closures (~70k => 30k closures), other counts are all equal or very close.
Looking at the chrome timeline the number of GC events is about half and the time/gc amounts roughly match the benchpress measurements.
Worth the breaking change? The diff can probably be reduced or split into 2 commits if there's interest...