bind perf #9369
bind perf #9369
Conversation
|
I think all of the supported browsers use textContent now, so there may be a few places where we can remove conditionally assigning to textContent / innerText (as an additional improvement) --- such as in ngSanitize |
|
That's unrelated to the issue, just something that may be worth doing. I think we might want to iterate over each element in thee jquery object though, in case there's more than one (ng-bind-start / ng-bind-end) |
|
I'm wrong about the multi-element thing, i guess we disabled multiElement for ngBind |
|
Didn't think there was any senario where there would be more then one element in a directive. |
|
there used to be scenarios where ng-bind would have multiple elements, and tests which used it, but we changed the tests to use a custom directive since ngBind had multiElement disabled. (see e8066c4#diff-348c2f3781ed66a24894c2046a52c628L5552) |
|
In practice I don't think this is likely to make much of a difference since interpolation is used way more frequently than ng-bind --- but you should measure the difference in the largetable with ng-bind mode and report screenshots |
|
I did do it with a large table... Don't have a single page with both measure or a screenshot but here is two results: Firefox: Chrome: |
|
specifically I meant https://github.com/angular/angular.js/tree/master/benchmarks/largetable-bp (there are instructions to run it in the benchmarks folder) |
|
Since we no longer need the jqLite/jQuery object we can let it get GCed... Personally I use ng-bind quite often instead of interpolation since it is faster (at least when in large ng-repeats, etc.). |
|
most people won't do that since it's much more convenient and more powerful to use interpolation. I don't have much of an opinion on letting the wrapper get GC'd, if you have a lot of these it's probably less helpful than you might think, but I don't care one way or the other there. Anyways, please post largetable-bp results |
|
I think it saved me ~100k or so with an average dataset in a table, and avoids the |
|
I'm all for it, I just want to supplement the PR with data explaining why we made the change |
|
fwiw, it basically looks good to me, but I think it's important to measure. If you won't, then I'll do it quickly myself |
|
https://www.dropbox.com/s/76wxqbvduade52s/big_table_benchmark_b1ee5396_vs_d580a954.zip?dl=0 so what I'm seeing from this (and I did retry the benches a number of times, this is with a single tab open in the latest Canary and it's pretty consistent), it doesn't make a significant difference, but it actually looks like the master branch is winning very slightly. This is with the ngBind + function call version of the test. Anyways, it doesn't look like it makes a significant difference one way or the other, WDYT @IgorMinar |
|
Wasn't available ATM. but thanks for the benchmark |
GC jqlite wrapper. Same for BindTemplate
|
lgtm. it doesn't make a huge difference but there are no downsides. so why not. |
|
will merge after fixing lint issues |
| $compile.$$addBindingInfo(element, interpolateFn.expressions); | ||
| attr.$observe('ngBindTemplate', function(value) { | ||
| element.text(value); | ||
| element.textContent = value; |
There was a problem hiding this comment.
Why are we not checking for an undefined value as we are above, in this case? gecko and webkit will render undefined, which we probably don't want (do we? Chrome won't, fwiw)
There was a problem hiding this comment.
I think we should use a condition here too, so that the behaviour is consistent across browsers
| @@ -57,12 +57,13 @@ var ngBindDirective = ['$compile', function($compile) { | |||
| compile: function ngBindCompile(templateElement) { | |||
| $compile.$$addBindingClass(templateElement); | |||
| return function ngBindLink(scope, element, attr) { | |||
| var element = element[0]; | |||
| $compile.$$addBindingInfo(element, attr.ngBind); | |||
| scope.$watch(attr.ngBind, function ngBindWatchAction(value) { | |||
| // We are purposefully using == here rather than === because we want to | |||
| // catch when value is "null or undefined" | |||
| // jshint -W041 | |||
There was a problem hiding this comment.
This comment is nonsense, because all supported browsers will output the empty string in the case of null --- only undefined is an issue. I am going to remove the comment and use strict equals
There was a problem hiding this comment.
I just realized you didn't add the comment =) but yeah... removing it
"Speeds up chrome with ~10% firefox by ~5%" We don't really see this result in benchmarks (https://www.dropbox.com/s/76wxqbvduade52s/big_table_benchmark_b1ee5396_vs_d580a954.zip?dl=0) However, it's basically harmless. Side effects: Use strict equality check for `undefined` to replace with empty string. Most target browsers will output `undefined` rather than the empty string if we don't do this. Previously, ngBindTemplate did not perform this check. However the change has been made to make behaviour consistent across all target browsers (chrome does output the empty string). Closes angular#9369
... and removed the comment
Speeds up chrome with ~10% firefox by ~5%