Victory: Performance issues?

[bclinkinbeard]

I was curious about how Victory would perform compared to vanilla D3, so I threw together a quick and dirty test. The code is here, and it attempts to make equivalent bar charts with each approach. You can switch the useVictory flag to specify which version is rendered.

I also published each version for convenience. The D3 version performs noticeably better than the Victory version, so I am just curious if anyone knows why. The differences on mobile are even more pronounced.

Again, this is simply a matter of curiosity. I don't mean to knock the project in any way.

[exogen]

@bclinkinbeard: Thanks for the demo! We're trying to improve perf all the time.

Can you post the (webpack?) config that generates bundle.js? If it wasn't generated in "production" mode, then React and Radium are doing tons of extra checks that they are only supposed to do in "development" mode. During my own testing, I've seen switching the build environment to production make some Victory animations go from around 25 fps to around ~60 fps.

[bclinkinbeard]

Hey @exogen, I used hjs-webpack for convenience, so the config is just this.

var getConfig = require('hjs-webpack')

module.exports = getConfig({
  in: 'index.js',
  out: 'public',
  clearBeforeBuild: true
})

The builds were generated by running webpack with no args, so I think they should be prod builds. In fact, I noticed a significant improvement when moving from local testing with webpack-dev-server to the deployed builds.

[exogen]

If using webpack, something like this in your config should do the trick:

  plugins: [
    new webpack.DefinePlugin({
      "process.env": {
        NODE_ENV: JSON.stringify("production")
      }
    })
  ]

If using browserify, you can try envify.

[exogen]

@bclinkinbeard Gotcha, thanks for the info!

[bclinkinbeard]

Hmm, I take that back. Switching to a proper webpack config, even without the env definition, results in much better performance.

var path = require('path');
var webpack = require('webpack');

module.exports = {
  entry: [
    './index'
  ],
  output: {
    path: path.join(__dirname, 'public'),
    filename: 'bundle.js',
    publicPath: '/public/'
  },
  plugins: [
    new webpack.DefinePlugin({
      "process.env": {
        NODE_ENV: JSON.stringify("production")
      }
    })
  ],
  module: {
    loaders: [
      {
        test: /\.js$/,
        loaders: ['babel']
      }
    ]
  }
};

I've updated the deployed Victory example to demonstrate. (Built with prod env.)

[bclinkinbeard]

Performance is now identical, AFAICT, even on mobile. Nice work!

[exogen]

@bclinkinbeard Hold up, I'm seeing d3Container populated in that updated demo and not victoryContainer – sure the useVictory flag was set? I'd be thrilled if they were identical but want to make sure! :)

[bclinkinbeard]

Whoops lol, I guess I screwed that up as I was walking out the door. Updated the examples and pushed the code to a repo.

D3 Example
Victory Example
Repo

Performance differences are now very clear, especially on mobile. :|

[coopy]

I did some quick profiling on https://github.com/coopy/d3-victory-comparison, and most CPU cycles are spent in Radium's resolveStyles method. Removing the Radium wrapping makes the victory version as performant as the D3 version.

Profiling with react-perf-addon of one setState render cycle gave these numbers:

Inclusive render time ​_with_​ Radium: 2926ms
Inclusive render time ​_without_​ Radium: 755ms

I removed Radium by commenting out this line in node_modules/victory-bar/lib/components/victory-bar.js:

// VictoryLabel = (0, _radium2["default"])(VictoryLabel) || VictoryLabel;

This is how I set up react-addons-perf:

var Perf = window.Perf = require('react-addons-perf');
Perf.start();

function createVictory () {
  var Bar = React.createClass({
    componentDidMount: function () {
      var self = this
      setInterval(function () {
        self.setState({data: getData()}, function () {
          if (!perfPrinted) {
            Perf.stop();
            var measurements = Perf.getLastMeasurements();
            console.log("INCLUSIVE")
            Perf.printInclusive(measurements);
            console.log("EXCLUSIVE")
            Perf.printExclusive(measurements);
            Perf.printWasted(measurements);
            Perf.printDOM(measurements);
            perfPrinted = true;
          }
        })
      }, updateInterval)
    },

We need to research a solution for this. As I see it:

• Option A: Fix usage of Radium in Victory to be performant
  
  
  
  • PureRender optimization can be a solution: https://github.com/FormidableLabs/radium/issues/389
  
  
• Option B: Fix perf issues in Radium
  
  
  
  • Early return if styles haven't changed?
  
  
• Option C: Don't use radium
  
  
  
  • :(
  
  

[EvHaus]

I would strongly advise looking into Option C first. I think a strong case needs to be made for inline styles. There are many case studies showing that inline styles cause performance issues (ie. http://ctheu.com/2015/08/17/react-inline-styles-vs-css-stupid-benchmark/). Our internal experiments show that inline styles are an awful drain on a browser's rendering queue once you start having a UI where a lot of duplicate components are needed (tables, lists, charts, etc...). You end up generating a ton of duplicated wasted code and dumping it into the DOM.

We already know that SVG isn't as performant as a direct canvas solution for charting, so adding the additional overhead of inline styles is only going to make Victory less compelling to use.

It would be better if Victory was agnostic about the way styling is done. So those that want to use CSS classes can specify them, and those that want to use inline styling can use their preferred inline style library.

[dandelany]

^ this is a really valuable comment, thanks @globexdesigns

In light of this I think I would be in favor of:

• Support inline styles via the style prop, like it is now or close to it, for those that want them. But maybe not via Radium if it's causing perf issues.
• Have a sensible default inline style, so that the beginner's "getting started" workflow still allows you to just render a <VictoryWhatever /> and have it look good.
• Make it easy to override the default style in favor of no inline styles, I'm thinking style={null}. This would prevent any inline styles from being attached to any victory components and avoiding the performance overhead.

[jmaguirrei]

@divmain Is it possible today to use Victory.js without Radium? I have a strategy for CSS that combines inline styles (for representing states and themes defined by user) with regular CSS classes (for all the rest). So I am not interested in having Radium, but I can see it's a dependency for Victory and may cause performace issues. Thanks.

[boygirl]

@jmaguirrei it is not possible to use Victory without Radium today, but we are looking into that option, and we should have an answer by the end of the week. Even without Radium however, Victory will expect / rely on inline styling.

[divmain]

@jmaguirrei, it looks like we're going to remove Radium from the bundle and provide documentation for users that require the functionality. We are working on a few other changes that will shave off some additional time.

Compared to d3, the combination of these changes should bring us from a 12x performance deficit to a 3x performance deficit (where React alone introduces a ~1.8x performance hit because of differences in VirtualDOM performance vs D3). These changes should land sometime this week.

[divmain]

PR: https://github.com/FormidableLabs/victory-chart/pull/93

[jmaguirrei]

@divmain Thanks for your answer, I will be looking for next release. One thing I dont understand is why Victory is still slower than D3 (after removing radium). If Victory took the d3 mathematical engine, and instead of manipulating the DOM uses React to compute the minimum required updates after changing props, how is it possible to be d3 still more performant? Have you found the explanation?

[divmain]

@jmaguirrei the most significant contributor (after the changes) is differences in how D3 and React interact with the DOM. Without anything Victory, and using D3 for all the math, React is approx 1.75 times slower than D3.

I'm sure that we can make additional perf improvements as time goes on - but this initial effort was to get us in the same ballpark. Those attempts will be easier to make once Victory has stabilized somewhat.

[divmain]

Going to close this ticket - performance is significantly improved, and the changes will be included in the next release. Will revisit perf once as we approach a 1.0 release.