perf(ngOptions): avoid calls to element.value #15344

klieber · 2016-11-01T03:57:51Z

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

Performance

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

In some cases IE11/Edge calls to element.value are very slow when the element.value has not been set. Normally, these calls are usualy 0-3 ms but in these cases it can take 200-300 ms. This can easily add 3 or more seconds to the load time on a view that has 10 or more select tags using ngOptions.

The line this pull request is changing not only suffers from the performance issue described above but it also appears to be broken. The code is checking that option.value does not equal element.value but then sets element.value to option.selectValue. I don't believe option.value is actually defined anywhere and likely it was always intended to be option.selectValue. This means that check would always be true and since this code has been this way for quite a while and is causing a performance issue I've just removed the check. This way a call to element.value is never made prior to it's value being set.

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

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

In some cases IE11/Edge calls to element.value are very slow when the element.value has not been set. Normally, these calls are usualy 0-3 ms but in these cases it can take 200-300 ms. This can easily add 3 or more seconds to the load time on a view that has 10 or more select tags using ngOptions. The line this pull request is changing not only suffers from the performance issue described above but it also appears to be broken. The code is checking that option.value does not equal element.value but then sets element.value to option.selectValue. I don't believe option.value is actually defined anywhere and likely it was always intended to be option.selectValue. This means that check would always be true and since this code has been this way for quite a while and is causing a performance issue I've just removed the check. This way a call to element.value is never made prior to it's value being set.

petebacondarwin · 2016-11-01T10:42:36Z

Thanks @klieber

In some cases IE11/Edge calls to `element.value` are very slow when the `element.value` has not been set. Normally, these calls are usualy 0-3 ms but in these cases it can take 200-300 ms. This can easily add 3 or more seconds to the load time on a view that has 10 or more select tags using `ngOptions`. The line this pull request is changing not only suffers from the performance issue described above but it also appears to be broken. The code is checking that `option.value` does not equal `element.value` but then sets `element.value` to `option.selectValue`. I don't believe `option.value` is actually defined anywhere and likely it was always intended to be `option.selectValue`. This means that check would always be true and since this code has been this way for quite a while and is causing a performance issue I've just removed the check. This way a call to `element.value` is never made prior to it's value being set. Closes #15344

gkalpak · 2016-11-01T15:31:46Z

src/ng/directive/ngOptions.js

@@ -591,7 +591,7 @@ var ngOptionsDirective = ['$compile', '$document', '$parse', function($compile,
          element.label = option.label;
          element.textContent = option.label;
        }
-        if (option.value !== element.value) element.value = option.selectValue;
+        element.value = option.selectValue;


Interestingly, option.value will always be undefined, so this was a redundant.
I wonder what the intenton was, though.

@gkalpak I noticed the same thing. That's how I new this check would be safe to remove. My guess is it was intended to be option.selectValue !== element.value and the thought was that would prevent setting element.value if it didn't need to be. Strange thing about that though is that this function is only ever called from the addOptionElement function and it creates a new element cloned from optionTemplate everytime . So, I don't think element.value will ever actually have a value prior to this line anyway. Makes me wonder if the check a few lines above option.label !== element.label is necessary.

klieber · 2016-11-01T18:45:39Z

@petebacondarwin do you know which release this will be included in?

gkalpak · 2016-11-01T19:49:06Z

I will be included in the next 1.5.x/1.6.x releases (i.e. 1.5.9 and 1.6.0-rc.1).

klieber · 2016-11-01T20:17:55Z

Thanks!

In some cases IE11/Edge calls to `element.value` are very slow when the `element.value` has not been set. Normally, these calls are usualy 0-3 ms but in these cases it can take 200-300 ms. This can easily add 3 or more seconds to the load time on a view that has 10 or more select tags using `ngOptions`. The line this pull request is changing not only suffers from the performance issue described above but it also appears to be broken. The code is checking that `option.value` does not equal `element.value` but then sets `element.value` to `option.selectValue`. I don't believe `option.value` is actually defined anywhere and likely it was always intended to be `option.selectValue`. This means that check would always be true and since this code has been this way for quite a while and is causing a performance issue I've just removed the check. This way a call to `element.value` is never made prior to it's value being set. Closes angular#15344

In some cases IE11/Edge calls to `element.value` are very slow when the `element.value` has not been set. Normally, these calls are usualy 0-3 ms but in these cases it can take 200-300 ms. This can easily add 3 or more seconds to the load time on a view that has 10 or more select tags using `ngOptions`. The line this pull request is changing not only suffers from the performance issue described above but it also appears to be broken. The code is checking that `option.value` does not equal `element.value` but then sets `element.value` to `option.selectValue`. I don't believe `option.value` is actually defined anywhere and likely it was always intended to be `option.selectValue`. This means that check would always be true and since this code has been this way for quite a while and is causing a performance issue I've just removed the check. This way a call to `element.value` is never made prior to it's value being set. Closes #15344

In some cases IE11/Edge calls to `element.value` are very slow when the `element.value` has not been set. Normally, these calls are usualy 0-3 ms but in these cases it can take 200-300 ms. This can easily add 3 or more seconds to the load time on a view that has 10 or more select tags using `ngOptions`. The line this pull request is changing not only suffers from the performance issue described above but it also appears to be broken. The code is checking that `option.value` does not equal `element.value` but then sets `element.value` to `option.selectValue`. I don't believe `option.value` is actually defined anywhere and likely it was always intended to be `option.selectValue`. This means that check would always be true and since this code has been this way for quite a while and is causing a performance issue I've just removed the check. This way a call to `element.value` is never made prior to it's value being set. Closes angular#15344

googlebot added the cla: yes label Nov 1, 2016

petebacondarwin closed this in 3b7f29f Nov 1, 2016

gkalpak reviewed Nov 1, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(ngOptions): avoid calls to element.value #15344

perf(ngOptions): avoid calls to element.value #15344

klieber commented Nov 1, 2016

petebacondarwin commented Nov 1, 2016

gkalpak Nov 1, 2016

klieber Nov 1, 2016

klieber commented Nov 1, 2016 •

edited

Loading

gkalpak commented Nov 1, 2016

klieber commented Nov 1, 2016

perf(ngOptions): avoid calls to element.value #15344

perf(ngOptions): avoid calls to element.value #15344

Conversation

klieber commented Nov 1, 2016

petebacondarwin commented Nov 1, 2016

gkalpak Nov 1, 2016

Choose a reason for hiding this comment

klieber Nov 1, 2016

Choose a reason for hiding this comment

klieber commented Nov 1, 2016 • edited Loading

gkalpak commented Nov 1, 2016

klieber commented Nov 1, 2016

klieber commented Nov 1, 2016 •

edited

Loading