Improve performance of d3_selectionPrototype.attr #2686
Conversation
Current engines (tested on Firefox and Chrome) aren't *that* smart about `arguments` yet. It is slow to assign to one while using the others, because they are bound: changing `name` also changes `arguments[0]`, which hinders the engines' ability to improve the code. The second issue is using a non-local value (and a `arguments` one at that) as the topic of a `for`-`in`, which is suboptimal. In Chrome's profil, it warns with: "Not optimized: ForInStatement with non-local each variable".
|
Thanks for the suggestion. I suspect that this is not a noticeable performance improvement, but it’s probably worth adopting this type of change D3-wide regardless. I’m not sure I’m willing to find and fix all of these places in D3 3.x, but I’d be willing to survey the new d3 modules for 4.0 and adopt this optimization. |
|
It appeared in my profiler output, that's why I changed it in the first place. We call setAttribute (through attr) very extensively, it seems... |
|
Yep, anything that touches the DOM is typically expensive; it should be much more expensive than re-assigning to an argument. |
|
Right, but at least it silences the chrome profiler warnings. The second thing I'm the most "sad" about is the array subclassing using proto, but I don't really have the time to "fork" and change everything for our codebase... Oh well, guess I'm stuck with this in the meantime. Do you want me to close this PR, or are you interesting to take it on this repo? |
|
The array subclassing is going away in 4.0; it’s not a backwards-compatible change so it has to wait until the next major release. See the d3-selection repo. I don’t plan on merging this into 3.x, but I was going to keep this issue around as a reminder to go through the new 4.0 repositories and look for similar problems. |
|
Good news, thanks :) |
|
The good news is that D3 4.0 is written in ES2015 modules and strict mode, thus this is no longer an optimization killer. |
|
Is this confirmed not to trigger perf. warnings in chrome? |
|
Also, d3-selection generally avoids re-assigning to arguments. See the new selection.attr implementation, for example. |
Current engines (tested on Firefox and Chrome) aren't that smart about
argumentsyet.It is slow to assign to one while using the others, because they are bound: changing
namealso changesarguments[0], which hinders the engines' ability to optimize the code.The second issue is using a non-local value (and a
argumentsone at that) as the topic of afor-in, which is suboptimal.In Chrome's profil, it warns with: "Not optimized: ForInStatement with non-local each variable".