pureRender totally useless

[Cloudo]

I just mentioned that shallowCompare method is totally useless for current version.
I slightly changed pureRender decorator to store information about results of shouldComponentUpdate.

window.perf = {};
function shouldComponentUpdate(nextProps, nextState) {
  var displayName = this.constructor.displayName;
  if (!window.perf[displayName]) {
    window.perf[displayName] = {
      yes: 0,
      no: 0
    }
  }

  var result = shallowCompare(this, nextProps, nextState);
  if (result) {
    window.perf[displayName].yes += 1;
  } else {
    window.perf[displayName].no += 1;
  }
  return result;
}

After render LineChart and mouseovering for a while I got results:

Line              yes: 1963      no: 1
Layer             yes: 3527      no: 0
Animate           yes: 1971      no: 0
Curve             yes: 2350      no: 0
Surface           yes: 2346      no: 0
CartesianGrid     yes: 391       no: 0
CartesianAxis     yes: 782       no: 0
Legend            yes: 782       no: 0
Tooltip           yes: 391       no: 0
DefaultTooltip    yes: 391       no: 0
Dot               yes: 1579      no: 316

The chart is almost totally rerender on any event yet it has the same data.

First of all, functions created in render should be refactored, cause it totally breaks pureRender.
More info here: https://medium.com/@esamatti/react-js-pure-render-performance-anti-pattern-fb88c101332f#.30axfnwr2
http://benchling.engineering/performance-engineering-with-react/

I tried to use another approach for some components and it works much faster:

function notDeepEqual(a, b) {
  return JSON.stringify(a) !== JSON.stringify(b);
}

...but I don't think that's a good solution.
guess we should find better way to deal with that problem...

[xile611]

It's really a problem.The state of LineChart is changing when mouse is moving in the chart, because we keep the location of mouse in state.

[arcthur]

The most condition is the reference of array or object. I use lodash isEqual function to re-write pureRender. It works faster.

Line              yes: 1023      no: 0
Layer             yes: 1955      no: 2197
Curve             yes: 1245      no: 253
LineChart         yes: 556       no: 81
Surface           yes: 556       no: 0
CartesianGrid     yes: 0         no: 354
CartesianAxis     yes: 1215      no: 0
Legend            yes: 0         no: 47
Tooltip           yes: 481       no: 7
DefaultTooltip    yes: 481       no: 0
Dot               yes: 493       no: 497

[Cloudo]

Thanks, @arcthur.

Yeah, it work faster now.
Actually isEqual from lodash is yet another way to perform deep comparison. In terms of performance, this approach is pretty expensive.

The problem is not only for tooltip, but in managing state in common sense.
Every time LineChart is rerendering, it creates a bunch of new objects for children:

const items = findAllByType(children, Line);

let xAxisMap = this.getAxisMap('xAxis', items);
let yAxisMap = this.getAxisMap('yAxis', items);
const offset = this.getOffset(xAxisMap, yAxisMap);

xAxisMap = this.getFormatAxisMap(xAxisMap, offset, 'xAxis');
yAxisMap = this.getFormatAxisMap(yAxisMap, offset, 'yAxis');

I think it would be better to store such auxiliary objects (like axisMaps and offset) to state, and perform shallow comparison for own LineChart props (nextProps) and children props (nextProps.children[].props) in componentWillReceiveProps. If that comparison will be false, we could recalculate these objects, otherwise just reuse previous state.
This approach will allow us to use shallowEqual check for other pure components.

Deep comparison seems like a brute force to achieve some performance boost, but it hides problem rather than solves it.
What do you think?

[arcthur]

Thanks @Cloudo to support the project, and welcome to provide the better codes to improve the recharts. 😄

Actually, isEqual isn't a better method that I may not spend much time to think about the problem. but the original pureRender is too simple that didn't satisfied most conditions, such as complex object. I will custom the better pureRender function to divisive the difference for next few days.

[xile611]

Thanks @Cloudo for your enthusiasm.
I plan to refactor the chart components in the next period of time.
I think the components can be optimized in the following two points:

1. Store some auxiliary objects (axisMaps, offset and so on)
2. Don't store the position of cursor in state

The job involves so much components, may take longer.
P.S. I'm sorry I didn't reply to you at the first time. We are on vacation for Chinese New Year, and we will start work next week.

[arcthur]

We optimized the pureRender function above official pureRender, that may be solved more problems. Always thanks!