-
Notifications
You must be signed in to change notification settings - Fork 349
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
Animated charts render slowly after ~200 data points #3429
Comments
I would strongly urge using Victory charts directly to compare performance. PatternFly basically wraps Victory components in order to add styling and some additional features. However, performance issues may need to be addressed upstream? I'm not familiar with pondjs. How would a pondjs adapter help performance? Would it be easy enough to create an example showing how it improves Victory chart performance? |
I think using the PatternFly wrapped version of the charts was seen as pretty desirable (because they look so darn good!) but agree it would be useful to see if there is a similar drop in frame rate by using Victory charts directly. At least then we could narrow down or maybe even begin to pinpoint the problem. The demo on that surge link definitely exhibits the slowness, about half (24fps) the frame rate on my mac as the one using pond (about 50fps), but it is worth noting that this is a purposefully slimmed down version of the implementation that @ceefit is using in his product. When you add more lines and fancier tooltips and start to view it across different platform/browser combos the lag becomes more noticeable and the experience degrades even more. I'm probably the worse to describe what pondjs does to help, but IIRC it has something to do with using a ring list/buffer implementation under the hood that may be more performant for handling these types of cases of data flowing in like we see in the demo. @ceefit plz save me from sounding silly if you know more about this side of things 😜 |
I've added a plain Victory charts page to my example, you can find it at https://spurious-attention.surge.sh/. There is a noticeable drop in performance between the Victory (50fps) and Patternfly (25fps) implementations. |
That's helpful, thanks for putting together the demo for illustration! I definitely support doing whatever we can to improve the performance on the PF charts, not sure without diving in more if it can be accomplished by modifying what we already have or if introducing a dedicated component to help would be the way to go. What I notice is that the VictoryLine component is carrying about twice the data in the PatternFly wrapped version as compared to when it's just Victory Charts on its own. The time series chart has a similar story to the Victory chart example, it's only updating/carrying about half the values and the framerate is plenty high. I wonder if there's a way to remove those extra/duplicate properties from the PF wrapped version and if that would have any noticeable impact on performance? That's just speculation though, I'm curious what others think. |
When wrapping VictoryLine, the props just pass thru. However, we do add a default container for the PatternFly theme. The components are stateless, so It's possible that the theme is being re-generated. You could provide your own container and/or custom theme to see if that speeds things up? |
That helps, thx @dlabrecq! Will give it a try and report back. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
FYI, there have been some significant performance improvements in victory@34.x, which may help. This update will be available in our breaking change release (i.e., PatternFly v4). |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Using the built in Victory charts to render an animated chart with 200 data points, the frame rate drops into the single digits. I've compared it against react-timeseries-charts (running on top of pond js) and the performance is about 3x improved.
This is reproducible by sending a frequently updating sequence of time series data (think system resource utilization dashboard). The number of points added directly impacts the performance. This is less noticeable in other charting frameworks running within Patternfly.
This looks like a bug, as I'd submit that 200 data points in an animated chart should be able to yield a higher frame rate.
I've hosted a patternfly seed app on http://didactic-blood.surge.sh/ that demonstrates this issue and caps the number of points at 200.
You'll notice the Victory-powered charts begin to render more slowly as the points are increased, and plateaus when it reaches the cap. The react-timeseries-charts do not exhibit this slowdown.
Source is available in my fork: https://github.com/ceefit/patternfly-react-seed
Would building a pondjs adapter to work with Patternfly's charts help with the performance?
The text was updated successfully, but these errors were encountered: