perf(input): prevent multiple validations on compilation #16760

Narretz · 2018-11-15T18:26:31Z

This commit updates in-built validators with observers to prevent
multiple calls to $validate that could happen after compilation in
certain circumstances:

when an input is wrapped in a transclude: element directive (e.g. ngRepeat),
the order of execution between ngModel and the input / validation directives changes so that
the initial observer call happens when ngModel has already been initalized,
leading to another call to $validate, which calls all defined validators again.
Without ngRepeat, ngModel hasn't been initialized yet, and $validate does not call the validators.

When using validators with scope expressions, the expression value is not available when
ngModel first runs the validators (e.g. ngMinlength="myMinlength"). Only in the first call to
the observer does the value become available, making a call to $validate a necessity.

This commit solves the first problem by storing the validation attribute value so we can compare
the current value and the observed value - which will be the same after compilation.

The second problem is solved by parsing the validation expression once in the link function,
so the value is available when ngModel first validates.

Closes #14691

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

jbedard · 2018-11-15T19:08:22Z

src/ng/directive/input.js

+          parsedMinVal = parseObservedDateValue(val);
+          ctrl.$validate();
+        }
+        minVal = val;


These could go within the if.

Any reason to do it after the $validate call instead of along with the parsedMinValue assignment?

jbedard · 2018-11-15T19:09:49Z

src/ng/directive/validators.js

  return {
    restrict: 'A',
    require: '?ngModel',
    link: function(scope, elm, attr, ctrl) {
      if (!ctrl) return;
+      var oldVal = attr.required || $parse(attr.ngRequired)(scope);


What if attr.required is false? 🤔

I believe that attr.required === false can't occur, right? If the attribute exists at all then, being a boolean attribute, its value will be true.

That's right, the value of the required attribute is ignored.

petebacondarwin

Generally LGTM. A few minor suggestions.

petebacondarwin · 2018-11-16T08:24:42Z

test/ng/directive/ngModelSpec.js

@@ -1246,6 +1245,7 @@ describe('ngModel', function() {
        dealoc(element);
      }));

+


Unnecessary whitespace changes?

petebacondarwin · 2018-11-16T08:34:29Z

test/helpers/testabilityPatch.js

+            var validationName = attrs.validationSpy;
+
+            var originalValidator = ctrl.$validators[validationName];
+            helper.validationCounter[validationName] = 0;


Because of this line, do you need to reset the validationCounter in the middle of tests?

helper.validationCounter = {};

I do it in the middle of tests when I test the normal and the ng- variation of a validator in the same test.

Right but in those tests you are recompiling so the directive will be linked again, causing the object to be reset. But I guess it does no harm to reset it in the test as well.

petebacondarwin · 2018-11-16T08:34:51Z

src/ng/rootScope.js

@@ -828,6 +830,7 @@ function $RootScopeProvider() {
                      lastDirtyWatch = watch;
                      watch.last = watch.eq ? copy(value, null) : value;
                      fn = watch.fn;
+                      // console.log('call watch fn', watch.exp, fn);


I assume these will disappear before merging :-P

petebacondarwin · 2018-11-16T08:41:09Z

src/ng/directive/validators.js

+    regex = new RegExp('^' + regex + '$');
+  }
+
+  if (regex && !regex.test) {


Is it not possible to test for a regex using regex instanceof RegExp? Or do we support non-regex objects that happen to have a test() method?

I can dig in the code to see if there's a reason for this (not my change ;)

petebacondarwin · 2018-11-16T08:41:29Z

src/ng/directive/validators.js

+      regex, startingTag(elm));
+  }
+
+  return regex || undefined;


I think you could simplify this function by bailing out early if the regex is falsy:

function parsePatternAttr(regex, patternExp, elm) { if (!regex) return undefined; if (isString(regex)) { regex = new RegExp('^' + regex + '$'); } if (!regex.test) { throw minErr('ngPattern')('noregexp', 'Expected {0} to be a RegExp but was {1}. Element: {2}', patternExp, regex, startingTag(elm)); } return regex; }

petebacondarwin · 2018-11-16T08:48:43Z

src/ng/directive/validators.js

-            regex, startingTag(elm));
+        // ngPattern might be a scope expressions, or an inlined regex, which is not parsable.
+        // We get value of the attribute here, so we can compare the old and the new value
+        // in the observer to avoid unnessecary validations


typo: unnessecary -> unnecessary

This word is the bane of my existence.

petebacondarwin · 2018-11-16T08:50:34Z

src/ng/directive/validators.js

+        // We get value of the attribute here, so we can compare the old and the new value
+        // in the observer to avoid unnessecary validations
+        try {
+          attrVal = $parse(attr.ngPattern)(scope);


Does the error get thrown in the call to $parse() or to the call on the result of parse $parse(...)(scope)?
If the former then we could move this block to a the compile function, so it is only executed once rather than per instance in an ngRepeat, say.

petebacondarwin · 2018-11-16T09:06:39Z

src/ng/directive/validators.js

  return {
    restrict: 'A',
    require: '?ngModel',
    link: function(scope, elm, attr, ctrl) {
      if (!ctrl) return;
+      var oldVal = attr.required || $parse(attr.ngRequired)(scope);


I believe that attr.required === false can't occur, right? If the attribute exists at all then, being a boolean attribute, its value will be true.

gkalpak

I generally like the idea of not re-validating if the values haven't changed (as long as we can guarantee that no validations are skipped when they shouldn't).

I am not a big fan of manually parsing values though. Couldn't we avoid that and instead rely on the parsing done inside the observers?

gkalpak · 2018-11-20T08:57:00Z

src/ng/directive/ngRepeat.js

@@ -1,4 +1,4 @@
-'use strict';
+  'use strict';


gkalpak · 2018-11-20T08:57:08Z

src/ng/directive/ngRepeat.js

@@ -531,6 +531,8 @@ var ngRepeatDirective = ['$parse', '$animate', '$compile', function($parse, $ani

        //watch props
        $scope.$watchCollection(rhs, function ngRepeatAction(collection) {
+          // console.log('ngRepeat action')


gkalpak · 2018-11-20T09:41:16Z

src/ng/directive/input.js

    };

    attr.$observe('step', function(val) {
-      stepVal = parseNumberAttrVal(val);
      // TODO(matsko): implement validateLater to reduce number of validations


Move to before ctrl.$validate() for consistency?

gkalpak · 2018-11-20T10:04:58Z

src/ng/directive/input.js

      // TODO(matsko): implement validateLater to reduce number of validations
-      ctrl.$validate();
+      if (stepVal !== val) {


Come one! You are using val !== ... is all other places 😛

gkalpak · 2018-11-20T10:34:49Z

test/ng/directive/validatorsSpec.js

+
+      expect(helper.validationCounter.required).toBe(1);
+
+      helper.validationCounter = {};


Is this necessary?

Not really. Should I remove it?

Yes, please 😁

gkalpak · 2018-11-20T10:46:40Z

src/ng/directive/validators.js

  return {
    restrict: 'A',
    require: '?ngModel',
    link: function(scope, elm, attr, ctrl) {
      if (!ctrl) return;

-      var minlength = 0;
+      var minlength = attr.minlength || $parse(attr.ngMinlength)(scope);
+      var minlengthParsed = toInt(minlength) || 0;


I think it is safe to fall back to -1 and thus be able to re-use parseLength() here as well.
If not, then parseLength() should be renamed to make it obvious it is not suitable for all kinds of lengths.

The tests agree :)

gkalpak · 2018-11-20T10:47:48Z

src/ng/directive/validators.js

+function parsePatternAttr(regex, patternExp, elm) {
+  if (!regex) return undefined;
+
+  if (isString(regex) && regex.length > 0) {


Since there is a !regex check above, the regex.length > 0 check is redundant.

src/ng/directive/input.js

gkalpak · 2018-11-20T11:04:18Z

src/ng/directive/validators.js

+    regex = new RegExp('^' + regex + '$');
+  }
+
+  if (!(regex instanceof RegExp)) {


Checking for instanceof RegExp instead of regep.test looks like a breaking change (and an unnecessary one afaict 😁).

We only allow RegExp and regex literals, which both are identified correctly by instanceof. Or could there be another reason for having .test instead?

I mean someone might be taking advantage of the fact that we were only testing for .test and provide a custom object like {test: () => /* Do some fancy stuff */}.
I know it is not a great idea, but I also know I've done it in the past 😁

What I am saying is that I don't see any benefit in breaking this (admittedly dubious) usecase at this point (a few months into LTS already).
Is there any benefit in changing that?

gkalpak · 2018-11-20T11:23:00Z

src/ng/directive/validators.js

+      var parseFn;
+
+      if (tAttr.ngPattern) {
+        patternExp = tAttr.ngPattern;


Previously, patternExp would also be set to attr.pattern (if attr.ngPattern was undefined).
Why is this changed?

if ngPattern isn't set, there can be no pattern expression, because pattern can only have a string value.

Theoretically, it can have any value 😁
(E.g. imagine a custom myPattern directive, that sets $attrs.pattern.)

gkalpak · 2018-11-21T19:52:06Z

src/ng/directive/validators.js

@@ -395,11 +395,11 @@ var minlengthDirective = function($parse) {
      if (!ctrl) return;

      var minlength = attr.minlength || $parse(attr.ngMinlength)(scope);
-      var minlengthParsed = toInt(minlength) || 0;
+      var minlengthParsed = parseLength(minlength) || -1;


The || -1 part is redundant, since parseLength() will take care of that (if needed).

src/ng/directive/input.js

gkalpak · 2018-11-21T20:08:49Z

src/ng/directive/input.js

      // TODO(matsko): implement validateLater to reduce number of validations
-      ctrl.$validate();
+      if (val !== stepVal) {


I suspect that we should be also checking for minVal here, because the step validation depends on that.

You mean ensure that the minVal has changed? That doesn't work, because we need to run this also if the minVal is the same.
This is a good point though - the changes in this PR only prevent multiple calls to $validate immediately after compilation / linking. If you have a range or number with more than one modifier, i.e. step, max, min and change all values in a single digest, every observer willcall $validate.
I don't think there's an easy way to prevent this, because the observers are independent of each other. Maybe that's what Matias meant with validateLater - a way to collect the $validate calls of a single "validation group" (For the record, I thought he meant a function that collects all $validate calls on an element).
Long story short, I don't think we can do anything about this without making bigger changes. I can adjust the commit message to reflect that changing multiple observed values will still result in multiple validate calls. (I believe this happens much rarer though, so the PR is still worthwhile).

No, I don't meant ensure that minVal has changed. What I meant is that that validator needs to run if either of minVal or stepVal has changed. I.e. even if stepVal is the same, but minVal has changed.

I am pretty sure that indeed Matias meant avoid multiple calls to $validate() 😁
We could easily do this by running $validate() asynchronously (e.g. adding it in the $postDigest queue and only if it is not already scheduled), but that would be a breaking change (which does not make sense at this point).

We could use a similar technique in the validators as this PR uses in observers:
Keep track of the involved values (e.g. module/viewValue, validators params, etc) and do not re-run validation unless the values have changed. But that would complicate things even further, open up new possibilities for memory leaks (if not handled carefully) and would also not help with the stateful async validators (which are usually the most expensive ones). So, again, not worth it atm.

I believe this happens much rarer though, so the PR is still worthwhile.

Actually, I believe it will happen all the time 😛
Basically, afaict, saves some (half) of the re-runs during initialization, but does nothing to prevent multiple runs as the user interacts with the input (which is where the majority of runs comes from throughout the lifetime of an input).

If we can keep it clean and BC-free (with reasonable confidence 😁), then it is definitely worth landing. But I wouldn't go to great extends, complicating the internal logic too much etc., just to save a few runs 😁

No, I don't me ensure that minVal has changed. What I meant is that that validator needs to run if either of minVal or stepVal has changed. I.e. even if stepVal is the same, but minVal has changed.

Right, but if the minVal has changed, it will call $validate, which will also run the step validator. Since we are not fixing multiple runs after initialization , we don't have to deal with this,

Actually, I believe it will happen all the time

It will only call $validate multiple times if observed values change, which I think is much rarer than initialization, which happens always. ;)
If you change the input value, only one call to $validate from inside ngModel will be made.
The issue that this addresses (#14691) is also about load / initialization, not interaction primarily.
Initially I also thought about using postDigest, but as you said, it's too late for that now.

gkalpak · 2018-11-21T20:14:02Z

src/ng/directive/input.js

-    attr.$observe(htmlAttrName, changeFn);
+    var oldVal = attr[htmlAttrName];
+    attr.$observe(htmlAttrName, function wrappedObserver(val) {
+      if (val !== oldVal) {


Same as above: I believe minVal should be checked along with stepVal.

gkalpak · 2018-11-21T20:21:05Z

src/ng/directive/validators.js

+      var parseFn;
+
+      if (tAttr.ngPattern) {
+        patternExp = tAttr.ngPattern;


Theoretically, it can have any value 😁
(E.g. imagine a custom myPattern directive, that sets $attrs.pattern.)

gkalpak · 2018-11-21T20:24:41Z

src/ng/directive/validators.js

+        try {
+          parseFn = $parse(tAttr.ngPattern);
+        } catch (e) {
+          if (/^\[\$parse:lexerr\]/.test(e.message)) {


@Narretz, yes, I suggest we re-use that bit of logic:

angular.js/src/ng/directive/attrs.js

Lines 392 to 395 in 0687a4f

if (ngAttr === 'ngPattern' && attr.ngPattern.charAt(0) === '/') {

var match = attr.ngPattern.match(REGEX_STRING_REGEXP);

if (match) {

attr.$set('ngPattern', new RegExp(match[1], match[2]));

gkalpak · 2018-11-21T20:29:19Z

src/ng/directive/validators.js

+    regex = new RegExp('^' + regex + '$');
+  }
+
+  if (!(regex instanceof RegExp)) {


I mean someone might be taking advantage of the fact that we were only testing for .test and provide a custom object like {test: () => /* Do some fancy stuff */}.
I know it is not a great idea, but I also know I've done it in the past 😁

What I am saying is that I don't see any benefit in breaking this (admittedly dubious) usecase at this point (a few months into LTS already).
Is there any benefit in changing that?

gkalpak · 2018-11-21T20:44:55Z

test/ng/directive/validatorsSpec.js

+
+      expect(helper.validationCounter.required).toBe(1);
+
+      helper.validationCounter = {};


Yes, please 😁

This commit updates in-built validators with observers to prevent multiple calls to $validate that could happen after compilation in certain circumstances: - when an input is wrapped in a transclude: element directive (e.g. ngRepeat), the order of execution between ngModel and the input / validation directives changes so that the initial observer call happens when ngModel has already been initalized, leading to another call to $validate, which calls *all* defined validators again. Without ngRepeat, ngModel hasn't been initialized yet, and $validate does not call the validators. When using validators with scope expressions, the expression value is not available when ngModel first runs the validators (e.g. ngMinlength="myMinlength"). Only in the first call to the observer does the value become available, making a call to $validate a necessity. This commit solves the first problem by storing the validation attribute value so we can compare the current value and the observed value - which will be the same after compilation. The second problem is solved by parsing the validation expression once in the link function, so the value is available when ngModel first validates. Closes angular#14691

This commit updates in-built validators with observers to prevent multiple calls to $validate that could happen on initial linking of the directives in certain circumstances: - when an input is wrapped in a transclude: element directive (e.g. ngRepeat), the order of execution between ngModel and the input / validation directives changes so that the initial observer call happens when ngModel has already been initalized, leading to another call to $validate, which calls *all* defined validators again. Without ngRepeat, ngModel hasn't been initialized yet, and $validate does not call the validators. When using validators with scope expressions, the expression value is not available when ngModel first runs the validators (e.g. ngMinlength="myMinlength"). Only in the first call to the observer does the value become available, making a call to $validate a necessity. This commit solves the first problem by storing the validation attribute value so we can compare the current value and the observed value - which will be the same after compilation. The second problem is solved by parsing the validation expression once in the link function, so the value is available when ngModel first validates. Closes #14691 Closes #16760

googlebot added the cla: yes label Nov 15, 2018

Narretz force-pushed the fix-validate-multiple branch from 985dc51 to 4ca5cf2 Compare November 15, 2018 19:01

jbedard reviewed Nov 15, 2018

View reviewed changes

Narretz force-pushed the fix-validate-multiple branch from 4ca5cf2 to f2aa98a Compare November 15, 2018 20:04

petebacondarwin approved these changes Nov 16, 2018

View reviewed changes

Narretz added component: forms type: perf labels Nov 16, 2018

Narretz added this to the 1.7.x milestone Nov 16, 2018

gkalpak reviewed Nov 20, 2018

View reviewed changes

gkalpak reviewed Nov 21, 2018

View reviewed changes

Narretz added 7 commits December 5, 2018 08:32

fixup! perf(input): prevent multiple validations on compilation

0d826b8

fixup! perf(input): prevent multiple validations on compilation

f2e1bd6

fixup! perf(input): prevent multiple validations on compilation

b4d7b61

fixup! perf(input): prevent multiple validations on compilation

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

b069361

fixup! perf(input): prevent multiple validations on compilation

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

57b45e1

fixup! perf(input): prevent multiple validations on compilation

4d82020

Narretz force-pushed the fix-validate-multiple branch from 038da95 to 4d82020 Compare December 5, 2018 07:32

Narretz merged commit 0637a21 into angular:master Dec 5, 2018

		@@ -1246,6 +1245,7 @@ describe('ngModel', function() {
		dealoc(element);
		}));


		expect(helper.validationCounter.required).toBe(1);

		helper.validationCounter = {};

	if (ngAttr === 'ngPattern' && attr.ngPattern.charAt(0) === '/') {
	var match = attr.ngPattern.match(REGEX_STRING_REGEXP);
	if (match) {
	attr.$set('ngPattern', new RegExp(match[1], match[2]));

perf(input): prevent multiple validations on compilation #16760

perf(input): prevent multiple validations on compilation #16760

Conversation

Narretz commented Nov 15, 2018

jbedard Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

gkalpak Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Nov 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard Nov 15, 2018 •

edited

Loading

gkalpak Nov 21, 2018 •

edited

Loading

Narretz Nov 23, 2018 •

edited

Loading

gkalpak Nov 26, 2018 •

edited

Loading

gkalpak Nov 21, 2018 •

edited

Loading