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

Avoid using Function constructor. #26

Closed
wants to merge 2 commits into from
Closed

Avoid using Function constructor. #26

wants to merge 2 commits into from

Conversation

vrana
Copy link

@vrana vrana commented May 2, 2017

Using the Function constructor is not compatible with CSP without
'unsafe-eval' as it evals the passed code. Use a function literal
instead.

vrana added 2 commits May 2, 2017 13:31
Using the Function constructor is not compatible with CSP without
'unsafe-eval' as it evals the passed code. Use a function literal
instead.
@mbostock
Copy link
Member

mbostock commented May 2, 2017

This behavior is noted in the README along with the suggestion to use parseRows if a CSP is in place. Changing the implementation as proposed here had a substantial negative performance impact, at least last time I measured it, and so I am wary of changing the default behavior. If you want to verify that this behavior has been fixed in modern browsers for large input files, then I will reconsider this request. Thank you!

@mbostock mbostock closed this May 2, 2017
@vrana
Copy link
Author

vrana commented May 2, 2017

Please give me instructions about how you want to measure this.

@vrana
Copy link
Author

vrana commented May 2, 2017

I was able to reproduce the slowness in Chrome by parsing Appearances.csv from http://seanlahman.com/baseball-archive/statistics/. I'll debug what's going on and update this pull request with a faster version.

Browser Old code New Code
Chrome 58 376 ms 1462 ms

@vrana
Copy link
Author

vrana commented May 2, 2017

I was able to bring it down with this code:

function objectConverter(columns) {
  var result = {};
  return function(d) {
    for (var i = 0; i < columns.length; i++) {
      result[columns[i]] = d[i];
    }
    return result;
  };
}
Browser Old code New Code
Chrome 58 Linux 376 ms 539 ms
Firefox 53 Linux 941 ms 725 ms

It's slightly faster in Firefox and slightly slower in Chrome. Is this acceptable for you?

I've also updated the README which should appear if you re-open this pull request.

@mbostock
Copy link
Member

mbostock commented May 2, 2017

That implementation returns the same object for every row rather than a new object for each row.

@vrana
Copy link
Author

vrana commented May 2, 2017

You are right.

@vrana
Copy link
Author

vrana commented May 2, 2017

I will not be able to make this fast. Thanks for your feedback.

@vrana
Copy link
Author

vrana commented Oct 13, 2021

This is still biting us. People keep using this function and when we want to deploy CSP, we need to refactor. Would you accept a patch using the CSP-compatible version if CSP is enabled? I can use a try-catch around the Function constructor.

This is a benchmark from today:

Code Chrome 94
Original 0.026 ms
try-catch with CSP disabled 0.026 ms
try-catch with CSP enabled 0.098 ms

Also, this is so quick these days that maybe it's not worth optimizing anymore?

@vrana
Copy link
Author

vrana commented Oct 14, 2021

Created #87.

@vrana
Copy link
Author

vrana commented Oct 19, 2021

This is the original benchmark with this code:

function objectConverter(columns) {
  return function(d) {
    var result = {};
    for (var i = 0; i < columns.length; i++) {
      result[columns[i]] = d[i] || '';
    }
    return result;
  };
}
Browser Original CSP compatible
Chrome 94 178 ms 371 ms
Firefox 78 445 ms 647 ms

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.

2 participants