performance issues with attrFunction/attrFunctionNS in attr.js

[mutsys]

I came across this while profiling one of my apps:

It seems that the function at issue here:

function attrFunction(name, value) {
  return function() {
    var v = value.apply(this, arguments);
    if (v == null) this.removeAttribute(name);
    else this.setAttribute(name, v);
  };
}

and its sibling, attrFunctionNS, are written in such a manner that the optimized code gets discarded by the v8 compiler and it falls back to the unoptimized code instead. The details can be found here:

GoogleChrome/devtools-docs#53

listed as item 4 in the section What is a deopt?

Looking at the implementation of the function, it seems that it specifically is fouling the compiler as described in this comment in the thread:

GoogleChrome/devtools-docs#53 (comment)

and the fix would seem to be to modify the function as follows:

function attrFunction(name, value) {
  return function() {
    var args = new Array(arguments.length);
    for (var i = 0, l = arguments.length; i < l; i++) {
      args[i] = arguments[i];
    }
    var v = value.apply(this, args);
    if (v == null) this.removeAttribute(name);
    else this.setAttribute(name, v);
  };
}

[mutsys]

Quick follow up: I performed this modification locally in my project and it does seem to resolve the issue. Chrome/v8 no longer deoptimizes the function.

Additionally, this issue is not limited to just this function. Many similarly written functions exist all through d3-selection. It would appear that there is an opportunity to improve performance across a significant cross-section of d3-selection without a lot of effort.

[mbostock]

Ugh.

According to GoogleChrome/devtools-docs#53 (comment) and https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#what-is-safe-arguments-usage, “STRICTLY fn.apply(y, arguments) is ok”. Is there something else going on here?

[mbostock]

Also, 0.97ms self time still isn’t a significant amount of time (though I’m not sure how long you were profiling for), so it might not be worth fixing even if there is a deopt warning.

[mutsys]

@mbostock I will provide most specific details on the overall impact of the change in my application. In the meanwhile, take a look at the difference between the timings before and after in above screenshots. They illustrate the difference (0.97ms/3.53ms -> 0.58ms/1.15ms) for **a single invocation **. One invocation out of 10s of 1000s that occured within the profiling session. This shakes out to an improvement of a couple of percentage points overall and appears to be significant in my application.

I recognize that not all applications might see this sort of improvement. My app happens to benefit from it greatly becuase I am constantly updating a number of graphs that are updated live and continuously from data streaming from multiple RESTful data sources simultaneously and it is meant to run continuously for an extended period of time. If you just draw a few hundred data points as a one-time operation, this is likely an optimization that you can live without.

I will provide you with a detailed breakdown of the impact later tonight.

[mbostock]

If you can share numbers on aggregate performance rather than a single invocation that would be more helpful in evaluating the impact of this change. Thank you!

[mbostock]

Closing to inactivity.