Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resolve chrome/v8 deoptimization issue related to Function.prototype.apply and arguments #111

Closed
wants to merge 2 commits into from

Conversation

mutsys
Copy link

@mutsys mutsys commented Jan 20, 2017

addresses the performance problems discussed in #110. these changes provide v8 with sufficient visibility into the elements of arguments, to allow it to optimize functions that follow a coding pattern that occurs frequently within d3:

before

export function example(foo, bar) {
  return function() {
    var v = bar.apply(this, arguments);
  }
}

after

export function example(foo, bar) {
  return function() {
    var args = new Array(arguments.length);
    for (var i = 0, l = arguments.length; i < l; i++) {
      args[i] = arguments[i];
    }
    var v = bar.apply(this, args);
  }
}

copying the elements of arguments to a new array prior to using it as the second argument to Function.prototype.apply resolves the issue and allows v8 to optimize the function once and continue to use the optimized code in subsequent invocations.

@curran
Copy link
Contributor

curran commented Jan 20, 2017

@mutsys The optimization seems like a big win!

One thing that strikes me is the huge amount of duplication in the code. Might it make sense to encapsulate the optimization, maybe something like this:

function toArray(args){
  var arr = new Array(args.length);
  for (var i = 0, l = arguments.length; i < l; i++) {
    arr[i] = args[i];
  }
  return arr;
}

It would introduce another level of function invocation, but would be a win for code cleanliness. Then again, I'm not 100% sure the optimization would still come into effect with this approach, as you'd need to invoke it with toArray(arguments).

Also, I'd be curious to see how much of a performance win this is, given that these instances are mostly coupled with DOM manipulation operations (e.g. this.setAttribute(name, v);), which might make the impact of the optimization negligible. Do you have any benchmarks you could share that would quantify the practical performance impact of these changes?

@mbostock
Copy link
Member

You have to inline it; if you pass the arguments to another function it again prevents optimization.

@mutsys
Copy link
Author

mutsys commented Jan 20, 2017

@curran Yes, it turns out that this is potentially a significant improvement, considering that the "join" pattern is at the heart of d3 repeatedly invoking attr with a high frequency. I will spend some time this evening to produce some hard data to back this up. In the meanwhile, if you take a look at #110 where I first reported the issue, you will see my initial measurement of the impact on a single invocation.

@mbostock
Copy link
Member

Closing to inactivity.

@mbostock mbostock closed this Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants