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

Fix calcTicksY formatting #2144

Merged
merged 2 commits into from
Apr 6, 2018
Merged

Conversation

ovvn
Copy link
Contributor

@ovvn ovvn commented Mar 2, 2018

No description provided.

@liquidpele
Copy link
Contributor

I took the liberty of running a quick test on my macbook pro on chrome.

d1 = Date();
for(var k1 in series) {
   for (var k2 in series[k1]) {
      var t = series[k1][k2].y < 34343;
   }
}
d2 = Date();
"Fri Mar 02 2018 08:11:28 GMT-0500 (EST)"
d1
"Fri Mar 02 2018 08:11:16 GMT-0500 (EST)"

For 100 series with 1 million points each (which is worst case granted), it takes about 12 seconds to loop through all the values.. and that's even without the extra function call overhead.

Cutting it down the a more standard size (100 points on 20 series) still takes about 2 seconds:

d1 = Date(); var x = y = 0;
for(var k1 in series) {
   if (x>20) break;
   x++;
   for (var k2 in series[k1]) {
      if (y>100) break;
      y++;
      var t = series[k1][k2].y < 34343;
   }
}
d2 = Date();
"Fri Mar 02 2018 08:15:03 GMT-0500 (EST)"
d1
"Fri Mar 02 2018 08:15:01 GMT-0500 (EST)"

I'm not seeing this as very efficient. If you can come up with a better way then that's fine, but otherwise I think I may need to revert your PR... @ovvn @jeznag

@ovvn
Copy link
Contributor Author

ovvn commented Mar 2, 2018

@liquidpele sure, you may revert the PR. The test isn't accurate though. We discussed O(n)difficulty, but the test has O(n^2). How the series is initialized? Here's more accurate test:

  var arr = [];
  for (var i = 0; i < 1000000; i++) {
    arr[i] = {value: i};
  }
  function getValue(i) {
    return arr[i].value;
  }
  function calculate(i) {
    return getValue(i) * 5 + 1 / 3 * 8;
  }

  console.time('Function #1');
  for (var i = 0; i < arr.length; i++) {
     calculate(i)
  }
  console.timeEnd('Function #1')

But all such tests are artificial. In reality nvd3 will do poorly when rendering a dataset if its length just hits something like 5000. In my experience every time I need to draw something that has thousands of elements then I would use a pure d3.

@liquidpele
Copy link
Contributor

Sorry, but maybe I don't see how my test was wrong... you have nested for loops in your solution so I copied that.

I do understand that there are some performance issues with nvd3 and many points, but that's all the more reason to not ADD to that issue... especially for something so trivial as deciding how many ticks to use.

@ovvn
Copy link
Contributor Author

ovvn commented Mar 2, 2018

Yes, it's nested, but as we discussed due to the discrete bar chart's dataset structure (1xN matrix) it is really just one for loop which gives us O(n). So the current PR's code does not do any harm. Performance test result is below:

  var arr = [];
  var len = 1000000;
  for (var i = 0; i < len; i++) {
    arr[i] = {value: i};
  }
  function getValue(i) {
    return arr[i].value;
  }
  function calculate(i) {
    return getValue(i) * 5 + 1 / 3 * 8;
  }

  console.time('Function #1');
  for (var i = 0; i < arr.length; i++) {
     calculate(i)
  }
  console.timeEnd('Function #1')

N = 1000000 Function #1: 4.701904296875ms

Ok, let's pretend that we extended the functionality for other chart types where dataset's structure is NxN array. In this case we indeed get a O(n^2) difficulty. Performance tests are below:

  var arr = [];
  var len = 10000;
  for (var i = 0; i < len; i++) {
    arr[i] = {value: i};
  }
  function getValue(i) {
    return arr[i].value;
  }
  function calculate(i) {
    return getValue(i) * 5 + 1 / 3 * 8;
  }

  console.time('Function #2');
  for (var i = 0; i < arr.length; i++) {
     for (var j = 0; j < arr.length; j++) {
        calculate(i)
     }     
  }
  console.timeEnd('Function #2')
N = 10000 Function #2: 235.738037109375ms
N = 20000 Function #2: 901.088134765625ms
N = 50000 Function #2: 5637.0390625ms
N = 100000 Function #2: 23155.084228515625ms
N = 200000 Function #2: 91066.65307617188ms

Conclusion:
As expected, for 1xN case (e.g. discrete bar chart, PR's case) everything's fine. For NxN case y ticks calculation will take seconds if there are more than 20000 elements.

@liquidpele
Copy link
Contributor

okay fair enough. I'll leave it and someone can come up with something better if it's a problem.

@liquidpele liquidpele merged commit 254bb45 into novus:master Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants