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

VictorySelectionContainer is too slow #873

Closed
4 tasks done
oplantalech opened this issue Dec 15, 2017 · 15 comments
Closed
4 tasks done

VictorySelectionContainer is too slow #873

oplantalech opened this issue Dec 15, 2017 · 15 comments
Assignees
Labels
Type: Performance 📈 Issues related to performance

Comments

@oplantalech
Copy link

oplantalech commented Dec 15, 2017

Bugs and Questions

Checklist

  • This is not a victory-native specific issue. (Issues that only appear in victory-native should be opened here)

  • I have read through the FAQ and Guides before asking a question

  • I am using the latest version of Victory

  • I've searched open issues to make sure I'm not opening a duplicate issue

The Problem

I am working in a plot with size around 20,000 points that can be selected. However, it takes a lot of time to see the selected points on the plot.

Reproduction

I've created a simple Victory Scatter Plot with 20,000 random points that change their color when selected (https://codesandbox.io/s/13x6436k34). You can try to select an area and count the time it takes to see the points in a different color.

@hrmoller
Copy link

I've got the same experience but with way less samples (~4500). Don't know why.

@pieterlukasse
Copy link

any news here? I think this is a serious issue / limitation.

@oplantalech
Copy link
Author

I've updated the issue with the new template you have added. @boygirl can you take a look at it? Thank you.

@pieterlukasse
Copy link

hi @boygirl, can you please let us know if this is an issue that you are planning to solve? We are currently blocked by this and need to decide on whether to continue with Victory or look for an alternative. Thanks!

@ryan-roemer
Copy link
Member

So, I've done a little preliminary perf analysis here.

First thing is a lot of time came from using react dev instead of react prod (measuring lots of points):

screen shot 2018-02-02 at 11 35 23 am

After I switched to react-dom and react prod versions, the remaining huge thing is prop-types work.

screen shot 2018-02-02 at 12 10 50 pm

There are babel plugins to just strip these -- can y'all try that and report back if it works better? If not, can you provide a sandbox / repository example that does strip those for further analysis by us? Thanks!!!

@oplantalech
Copy link
Author

Hi @ryan-roemer,

I've set up a sandbox here: https://codesandbox.io/s/2z3l51q79y, where I have initialized a webpack project with the code with the previous plot, I've set NODE_ENV to production, and I've added the following babel plugins: transform-react-remove-prop-types, transform-react-constant-elements, and transform-react-inline-elements. However, when I try to select points, it still takes a lot of time to see the selected points changing from grey to red. I would appreciate if you can take a look at it.

@ryan-roemer
Copy link
Member

@oplantalech -- For some reason, the thing actually running on CodeSandbox at that link is spending most of it's time still in propTypes validation.

I've pulled down the zip of the file and am attempting a manual build from the command line to make sure all the correct optimizations are applied.

@boygirl -- I can share out my version of the project if that's helpful to you...

@ryan-roemer
Copy link
Member

And does anyone have an example of another React library efficiently handling 20,000 points similarly as this behaves but with a lot more speed? I'd love to see a baseline to be able to separate the noise of what React is doing with all those points vs. Victory... Thanks!

@ryan-roemer
Copy link
Member

@boygirl -- I've got https://github.com/FormidableLabs/victory-perf-points-experiment up which is current and works for the issue. You can do perf analysis there hopefully, and if I get some more bandwidth, I can jump back in there...

@boygirl
Copy link
Contributor

boygirl commented Feb 6, 2018

Sorry for the delay on weighing in on this issue. Victory renders svg, so with 20,000 points you're going to have lag. There are almost certainly inefficiencies in Victory, but with 20,000 points, the bulk of slowness will be due the the number of svg nodes. One way to separate that issue out would be to compare the perf of a component like VictoryScatter that renders a bunch of individual paths, and a component like VictoryArea or VictoryLine that renders a single path for the entire dataset

@oplantalech
Copy link
Author

oplantalech commented Feb 7, 2018

@boygirl @ryan-roemer thank you for replying so fast.

@boygirl regarding the high amount of svg nodes making the plot slow, here you have an example of a plot with a lot of svg nodes that is fast: https://jsfiddle.net/oplantalech/dm8hfrau/

@ryan-roemer ryan-roemer added the Type: Performance 📈 Issues related to performance label Feb 23, 2018
@chrisbolin chrisbolin self-assigned this Apr 13, 2018
@chrisbolin
Copy link
Contributor

just a quick update: I think we found the bottleneck here. we'll keep you posted as we work on solutions

@chrisbolin
Copy link
Contributor

@oplantalech @pieterlukasse @hrmoller
We've made a few improvements in victory core and chart. if you would like to test them out, they are

victory-core@21.1.12
victory-chart@25.2.5

@chrisbolin
Copy link
Contributor

We still have work do to, but we saw big improvements we these changes:

a test with VictoryScatter and VictorySelectionContainer

1,500 points
before: 3.2 FPS
after: 6.4 FPS

4,000 points
before: 0.0 FPS (didn't update; froze)
after: 2.4 FPS

@pieterlukasse
Copy link

nice @chrisbolin , thanks for following up! We will test it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Performance 📈 Issues related to performance
Projects
None yet
Development

No branches or pull requests

6 participants