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

[jbedard]

This reduces GC (KB) by ~10% in the added benchmark create step.

I'm tempted to add a helper such as $interpolate.$$eval(exp, scope) to avoid the ugly duplication. A helper could also be used for spots such as https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1922 (the helper can pass mustHaveExpression=true to avoid creating the interp result function even though it has no interpolation...). Worth it?

[googlebot]

CLAs look good, thanks!

[lgalfaso]

I would be happier if the change was at $interpolate itself to detect this case... in this way, if there are other parts of the code (or other people using $interpolate) would benefit from it

[jbedard]

I was imagining something such as..

function $$interp(str, scope) {
  var interped = (str.indexOf(startSymbol) !== -1) && $interpolate(str, true);
  return interpped ? interpped(scope) : str;
}

Then the ng-form/model would do this.$name = $$interp($attrs.name || ... || '', $scope) and the other case I linked to would do isolateBindingContext[scopeName] = $$interp(attrs[attrName], scope). I don't think I found any other cases in a quick search. The only place I can think of putting this method is $interpolate.$$exec though.

Were you thinking of modifying $interpolate itself instead of wrapping it?

[lgalfaso]

I was thinking about modifying $interpolate

[jbedard]

Changing the meaning of $interpolate() based on a param sounds confusing to me, and would be modifying a public API. But I could try it if you'd prefer that option... did you have any ideas or preference for what the API would look like?

[lgalfaso]

in your description you state that not calling interpolate with an empty string causes some GC benefits. The question is why $interpolate cannot have this logic built-in so it would fail fast when the string is the empty string

[jbedard]

I assume it is from all the arrays/functions/closures created just to be returned, used once, and then thrown out.

One way to avoid that is to only $interpolate once within compile instead of per link. That will benefit every use case, not only empty or non-interpolated strings, but that can be considered breaking (jbedard@7d61a98 from #9772). Unless there is a better non-breaking way to do that.

Maybe just changing $interpolate and adding a simple case at the start will have enough gain though:

if (text.indexOf(startSymbol) === -1) {
  return mustHaveExpression ? undefined : extend(valueFn(text), {constant: true});
}

That will still create one closure just to call it once (for the form/model use case) but that seems much cleaner and will benefit more use cases.

[lgalfaso]

@jbedard the idea of adding

if (text.indexOf(startSymbol) === -1) {
  return mustHaveExpression ? undefined : extend(valueFn(text), {constant: true});
}

to $interpolate makes most sense. Do you have some numbers that shows that this makes the GC happier?

[jbedard]

Any combination of the existing PR, jbedard@7d61a98, and adding that quick return to $interpolate seem to all reduce GC by ~10% and reduce the time by a couple %. I'll update the PR to just add the quick return to $interpolate.

[jbedard]

Updated. However I've been having trouble trying to measure the results of this using the largetable benchmark. I would still say it is normally ~10% less GC, but the memory usage is inconsistent and never seems to stabilize.

So instead I started benchmarking $interpolate on its own which gave consistent numbers, although this is starting to be micro benchmarking...

Each operation below was done 1000x in a benchpress step, the steps were executed until the numbers normalized and are very consistent between runs.
(the numbers on the left are master => with this PR)

operation	time (ms)	memory GCed (kb)
a = $interpolate('')	20 => 10	11000 => 4400
b = $interpolate('foo')	45 => 10	12600 => 4450
c = $interpolate('foo {{i}}')	75 => 77	3500 => 3525
d = $interpolate(BIG)	155 => 160	9480 => 9550

where BIG = '{{i}} {{j + 123}} asdf {{i%2==0 ? 'foo' : 'bar'}}'

I don't understand why the constant ones produce more GC, but this PR doesn't change it so I'll ignore it for now.

The increase in time/memory for non-constants is fairly minor but it is consistent. The constants show a pretty big drop in GC (6kb per call!?) which might add up when done within ng-repeat.

Then executing the result function (the run times were all too close/small to care):

operation	memory GCed (kb)
a({})	800 => ~0
b({})	800 => ~0
c({})	882 => 885
d({})	1203 => 1206

The increase in memory for c/d is also weird, but that seems very minor.

HOWEVER, this change got a bit bigger then I'd like it to be (see TODO). Maybe we should just add the tests and benchmark for now? :|

[lgalfaso]

Numbers look promising, let me do some testing and if everything looks
fine. Will merge it later today

[Narretz]

@lgalfaso do you want to give this another shot?

[lgalfaso]

@Narretz will look at this this WE

[jbedard]

This PR is pretty ugly as is. I still think the best solution is to avoid interpolating the same thing per link (7d61a98). Although a more general quick-exit in $interpolate for strings with no interpolation would still be good as well.

[petebacondarwin]

@jbedard, @lgalfaso and @Narretz - do you fancy getting this improvement in this week?

[lgalfaso]

@petebacondarwin will take another look this week