perf(ngmodel/compile): improve ngmodel and general compile/controller performance (#9609) #9772

jbedard · 2014-10-24T08:11:59Z

This was mainly focused around #9609 - bringing 1.3 up to speed with 1.2 with an ng-repeated ng-model, at least when not using jQuery.

Overall these changes make the added benchmark (and the jsfiddle in #9609) 2-3x faster and seem to be faster then 1.2 now.

The last 3 commits (refactoring controller stuff in $compile) can probably be squashed but I left them separate for now for nicer diffs.

The only functional change (that I know of) is that the NgModelController $name is now initialized by the directive (pre-link) instead of within the controller. I'm not sure if this an issue or considered breaking? This was already the case for the $options field...

caitp · 2014-10-24T13:48:13Z

So, we need the tree to be green (so make sure you make jshint/jscs happy) --- I want you to try and avoid observable changes to behaviour when refactoring, it's a big deal that we don't break people more than we already have in releasing 1.3. It's very hard to avoid observable changes with javascript, the language has really lent itself towards breaking applications with very seemingly harmless changes. Lets put in tests for all of the cases that have been mentioned, more tests is always good.

Otherwise, LGTM

jbedard · 2014-10-24T16:30:38Z

I'll try to make some updates tonight based on the comments. And fix the build error (appears to be from the new jscs rules).

@realityking good to know. I thought some browsers still required it. I'll make that change as well.

jbedard · 2014-10-25T05:44:50Z

Updated to address most of the issues mentioned and rebased.

@realityking removing the useCapture arg causes some tests to fail because of how some browsers default to false and other to undefined. Maybe the tests could be written to handle that but I gave up for now.

If we don't want 5183eeb at this time I can move it to a different PR.

caitp · 2014-10-27T14:38:15Z

src/ng/compile.js

@@ -1974,6 +1968,12 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
      }
    }

+    function invokeEach(funcs) {


why make this a separate helper when it's only used once?

Mainly just to group all the controller initialization into a single call when debugging/profiling. I'd be fine removing it though...

jeffbcross · 2014-10-29T20:16:06Z

@IgorMinar is going to take a look at this now. Assigning to him now.

jeffbcross · 2014-10-29T20:16:49Z

Sorry, he's going to take a look at it "today" not "now"

jbedard · 2014-10-29T20:40:42Z

Note that if we don't want 5183eeb (which can be considered a breaking change) then we can still improve most use cases by simply avoiding $interpolate when ngmodel is used without a name attribute (basically jbedard@05fac02 - which is currently in a different WIP branch with similar avoid-$interpolate changes).

googlebot · 2014-11-28T06:20:06Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

gkalpak · 2014-11-28T12:23:48Z

test/ng/directive/formSpec.js

@@ -283,7 +283,7 @@ describe('form', function() {
        submitted = true;
      };

-      addEventListenerFn(doc[0], 'submit', assertPreventDefaultListener);
+      doc[0].addEventListener('submit', assertPreventDefaultListener);


The 3rd argument is missing in this file.
(Not sure if it is needed, but if not we could/should remove it from the other files as well.)

Good point. I'd rather be consistent...

gkalpak · 2014-11-28T12:28:31Z

I wonder what is the purpose of removing jqLite's add/removeChangeListener().

If we can get rid of the useCapture argument, then it is fine (I guess).
But if not, then having to add it "manually" every time is error-prone.
(Besides, there shouldn't be any real performance penalty in using a helper function, right ?)

Is there any other reason I might not be aware of ?

realityking · 2014-11-28T18:32:16Z

useCapture defaults to false in all modern browser (see MDN) so that should be fine. I'd remove it everywhere for consistency.

jbedard · 2014-11-28T18:39:18Z

See my comment above. Tests break because different browsers have different defaults for useCapture (false vs undefined). I'm tempted to just drop that commit for now though...

googlebot · 2014-12-11T05:48:48Z

CLAs look good, thanks!

jbedard · 2015-01-26T01:30:23Z

I've rebased this and removed some out of date and debatable commits (see https://github.com/jbedard/angular.js/tree/9609-ngmodel-perf-ORIG for all the originals). Maybe I should create a new PR? This PR no longer has the form/ngmodel change (sort of breaking) or controller change (bf6a79c was good enough) or more minor ones.

What's remaining is only the benchmark and controller methods in $compile:

fix a TODO to merge elementControllers/controllers (7a1edda)
avoid forEach at link time and only call .data once when setting up controllers (0196730)
move the refactored code into a method (e2ed092)

This improves the added benchmark ~20% and seems to reduce memory a lot (I haven't looked into why or verified this, but running the benchmark it is about half the memory during the create step!?). I really think this is worth merging...

jbedard · 2015-02-01T22:17:33Z

Rebased again. Previous results still hold (ngmodel benchmark being ~20% faster and half the memory usage).

@petebacondarwin is this on anyone's radar anymore? Should I create a new PR since half the commits were removed/done elsewhere?

lgalfaso · 2015-02-16T11:50:04Z

@jbedard can you close this PR and move the remaining parts into new PRs?

jbedard · 2015-02-17T05:43:00Z

Sounds good, closing this one...

mary-poppins added cla: yes and removed cla: no labels Oct 24, 2014

caitp added type: perf component: $compile component: forms needs: test labels Oct 24, 2014

caitp added this to the 1.3.x milestone Oct 24, 2014

caitp self-assigned this Oct 24, 2014

jbedard force-pushed the 9609-ngmodel-perf branch 7 times, most recently from 5e0919b to 9085099 Compare October 25, 2014 05:29

caitp reviewed Oct 27, 2014
View reviewed changes

jeffbcross assigned IgorMinar and unassigned caitp Oct 29, 2014

jbedard mentioned this pull request Nov 14, 2014

perf(*): use Object.create instead of creating temporary constructors #10058

Closed

jbedard force-pushed the 9609-ngmodel-perf branch from 9085099 to 9828412 Compare November 28, 2014 06:20

googlebot added cla: no and removed cla: yes labels Nov 28, 2014

jbedard force-pushed the 9609-ngmodel-perf branch from 9828412 to 89a4f89 Compare November 28, 2014 06:41

gkalpak reviewed Nov 28, 2014
View reviewed changes

jbedard force-pushed the 9609-ngmodel-perf branch from 89a4f89 to ff2be45 Compare December 11, 2014 05:48

googlebot added cla: yes and removed cla: no labels Dec 11, 2014

jbedard mentioned this pull request Dec 16, 2014

perf(ngModel,form): avoid invoking $interpolate for empty/unset values #10414

Closed

jbedard force-pushed the 9609-ngmodel-perf branch 7 times, most recently from 3e56d07 to b55b14b Compare January 25, 2015 10:01

jbedard added 4 commits February 1, 2015 13:47

perf(benchmark): add ngModel largetable case

21d2dc6

refactor($compile): combining elementControllers and controllers

7a1edda

perf($compile): simplifying/refactoring controller loops/methods

0196730

refactor($compile): moving controller setup into its own method

e2ed092

jbedard force-pushed the 9609-ngmodel-perf branch from b55b14b to e2ed092 Compare February 1, 2015 22:13

This was referenced Feb 14, 2015

refactor($compile): combining elementControllers and controllers vars #11058

Closed

perf($compile): avoid .data when fetching required controllers #11059

Closed

jbedard closed this Feb 17, 2015

This was referenced Feb 17, 2015

perf(benchmark): add ngmodel benchmarks to largetable-bp #11082

Closed

perf($compile): replace forEach(controller) with plain loops #11084

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(ngmodel/compile): improve ngmodel and general compile/controller performance (#9609) #9772

perf(ngmodel/compile): improve ngmodel and general compile/controller performance (#9609) #9772

jbedard commented Oct 24, 2014

caitp commented Oct 24, 2014

jbedard commented Oct 24, 2014

jbedard commented Oct 25, 2014

caitp Oct 27, 2014

jbedard Oct 27, 2014

jeffbcross commented Oct 29, 2014

jeffbcross commented Oct 29, 2014

jbedard commented Oct 29, 2014

googlebot commented Nov 28, 2014

gkalpak Nov 28, 2014

jbedard Nov 28, 2014

gkalpak commented Nov 28, 2014

realityking commented Nov 28, 2014

jbedard commented Nov 28, 2014

googlebot commented Dec 11, 2014

jbedard commented Jan 26, 2015

jbedard commented Feb 1, 2015

lgalfaso commented Feb 16, 2015

jbedard commented Feb 17, 2015

perf(ngmodel/compile): improve ngmodel and general compile/controller performance (#9609) #9772

perf(ngmodel/compile): improve ngmodel and general compile/controller performance (#9609) #9772

Conversation

jbedard commented Oct 24, 2014

caitp commented Oct 24, 2014

jbedard commented Oct 24, 2014

jbedard commented Oct 25, 2014

caitp Oct 27, 2014

Choose a reason for hiding this comment

jbedard Oct 27, 2014

Choose a reason for hiding this comment

jeffbcross commented Oct 29, 2014

jeffbcross commented Oct 29, 2014

jbedard commented Oct 29, 2014

googlebot commented Nov 28, 2014

gkalpak Nov 28, 2014

Choose a reason for hiding this comment

jbedard Nov 28, 2014

Choose a reason for hiding this comment

gkalpak commented Nov 28, 2014

realityking commented Nov 28, 2014

jbedard commented Nov 28, 2014

googlebot commented Dec 11, 2014

jbedard commented Jan 26, 2015

jbedard commented Feb 1, 2015

lgalfaso commented Feb 16, 2015

jbedard commented Feb 17, 2015