Perf optimizations #93

divmain · 2016-02-18T21:17:23Z

This PR includes some performance improvements. We will also be dropping Radium from Victory, and providing docs for how to use the two together. But that change will occur in a separate PR.

Here's what performance looks like after the changes (including removal of Radium). This is a ~3-4x improvement over the original performance.

/cc @boygirl @coopy

exogen · 2016-02-18T22:04:48Z

src/helpers/domain.js

@@ -73,8 +73,8 @@ module.exports = {
      return this.getDomainFromCategories(props, axis);
    }
    // find the global min and max
-    const rawDatasets = (props.stacked || this.shouldGroup(props)) ? props.data : [props.data];
-    const datasets = Data.formatDatasets(rawDatasets, props)
+    const hasMultipleDatasets = props.stacked || this.shouldGroup(pro


Line cut off?

coopy · 2016-02-23T17:41:48Z

This might work for statically styled components (I'll let @boygirl comment on the new API); we still need to address perf in cases where styles are dynamic.

ryan-roemer · 2016-02-25T00:16:39Z

src/components/victory-bar/victory-bar.jsx

@@ -251,6 +252,12 @@ export default class VictoryBar extends React.Component {

  static getDomain = BarHelpers.getDomain.bind(BarHelpers);

+  componentWillMount() {
+    this.memoized = {
+      getStyles: memoizerific(1)(Helpers.getStyles)


Can we get a code comment as to what memoizerific(1) is doing?

divmain · 2016-02-25T00:17:21Z

Hmm... tests are passing locally but not in CI. I'm investigating.

coopy · 2016-02-25T00:39:24Z

src/components/victory-axis/tick.jsx

-import { VictoryLabel } from "victory-label";
-import { Helpers } from "victory-util";
+import { VictoryLabel } from "victory-core";
+import { Helpers } from "victory-core";


import { Helpers, VictoryLabel } from "victory-core";

coopy · 2016-02-25T00:41:06Z

src/components/victory-line/victory-line.jsx

-import { PropTypes as CustomPropTypes, Helpers } from "victory-util";
-import { VictoryAnimation } from "victory-animation";
+import { PropTypes as CustomPropTypes, Helpers } from "victory-core";
+import { VictoryAnimation } from "victory-core";


coopy · 2016-02-25T00:43:03Z

Looks good - just some style / trivial comments. PERF++!

boygirl · 2016-02-25T01:00:42Z

@divmain I think there are still a few references to victory-label in the demos

boygirl · 2016-02-25T01:06:30Z

hmm, actually all of the demos are broken except for the axis demo. Now I'm sad that CI is passing :( Are you able to reproduce this problem @divmain

divmain · 2016-02-25T01:15:23Z

@boygirl I didn't think to check the demos. Figured builder run check was adequate. I'll do that now!
Edit: Not sure what's wrong. I'm going to pick this back up tomorrow.

divmain · 2016-02-25T22:28:58Z

Alright, this should be good to go. Still passing builder run check locally, and the demo is working as expected. Ping for final review @boygirl and @coopy.

boygirl · 2016-02-25T22:42:00Z

cc/ @divmain some of the demos are still importing VictoryLabel from victory-label. It's probably still hanging around in your node_modules. Please change these to depend on victory-core. Offenders are demos/victory-axis-demo, demos/victory-line-demo, demos/victory-scatter-demo and docs/victory-bar/docs.jsx. I think that's it. Approved once these are resolved.

divmain · 2016-02-25T22:43:52Z

Meh. Sorry about that.

divmain · 2016-02-25T22:45:57Z

@boygirl is there a way to check health of demos from CI? That would be a helpful change.

boygirl · 2016-02-25T22:48:25Z

Currently no. Since they are all their own components though, we could at least be trying to render them in tests. What I would ideally like would be screenshot comparison, but there still isn't a super good solution for that as far as I can tell. I'll open an issue for the easy win though.

boygirl · 2016-02-25T22:50:07Z

https://github.com/FormidableLabs/victory-chart/issues/110

divmain · 2016-02-25T22:50:25Z

thxmuch :)

Perf optimizations

divmain mentioned this pull request Feb 18, 2016

Perf optimizations FormidableLabs/victory-util#27

Closed

exogen reviewed Feb 18, 2016
View reviewed changes

divmain force-pushed the perf-optimizations branch from 83ac4fd to 30ed2e3 Compare February 18, 2016 22:39

divmain force-pushed the perf-optimizations branch 3 times, most recently from c10f113 to 1453c11 Compare February 24, 2016 23:39

divmain added 11 commits February 24, 2016 16:03

Use seriesIndex correctly for react subcomponent key.

763a7d8

Split props calculation into separate method.

124c970

Memoize victory-bar style calculation.

3a25c37

Improve render perf by ~2x; avoid expensive merge operation.

aeee75a

Replace _.merge with _.defaults, for minor optimization benefits.

7e99869

Add note about hot code path and future optimizations.

910a37c

Lint.

d88e873

Fix parse error.

3d5cca1

All components use memoized Helpers.getStyles with new fn signature.

cc239ee

Use _.assign and conditional objs to avoid undefined properties.

e9dfd89

Fix tests for formatDatasets after function signature change.

125ccd2

divmain force-pushed the perf-optimizations branch from 1453c11 to 125ccd2 Compare February 25, 2016 00:04

divmain mentioned this pull request Feb 25, 2016

Performance issues? FormidableLabs/victory#141

Closed

ryan-roemer reviewed Feb 25, 2016
View reviewed changes

Transition to using victory-core.

55689b1

divmain force-pushed the perf-optimizations branch from a7589a0 to 55689b1 Compare February 25, 2016 00:20

divmain added 2 commits February 24, 2016 16:22

Add code comments for memoizerific.

c225554

Update victory-core to v0.0.1.

fcc200a

coopy reviewed Feb 25, 2016
View reviewed changes

Consolidate imports from victory-core.

525096e

divmain closed this Feb 25, 2016

divmain reopened this Feb 25, 2016

boygirl mentioned this pull request Feb 25, 2016

[WIP] fix regressions FormidableLabs/victory-core#6

Merged

divmain added 2 commits February 25, 2016 14:21

Fix regression in createStringMap due to typo.

aae5be3

Update victory-core dependency.

f76c9a9

Pull in VictoryLabel from victory-core.

3905873

divmain added a commit that referenced this pull request Feb 25, 2016

Merge pull request #93 from FormidableLabs/perf-optimizations

256a305

Perf optimizations

divmain merged commit 256a305 into master Feb 25, 2016

divmain deleted the perf-optimizations branch February 25, 2016 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Perf optimizations #93

Perf optimizations #93

divmain commented Feb 18, 2016

exogen Feb 18, 2016

divmain Feb 18, 2016

coopy commented Feb 23, 2016

ryan-roemer Feb 25, 2016

divmain commented Feb 25, 2016

coopy Feb 25, 2016

coopy Feb 25, 2016

coopy commented Feb 25, 2016

boygirl commented Feb 25, 2016

boygirl commented Feb 25, 2016

divmain commented Feb 25, 2016

divmain commented Feb 25, 2016

boygirl commented Feb 25, 2016

divmain commented Feb 25, 2016

divmain commented Feb 25, 2016

boygirl commented Feb 25, 2016

boygirl commented Feb 25, 2016

divmain commented Feb 25, 2016

Perf optimizations #93

Perf optimizations #93

Conversation

divmain commented Feb 18, 2016

exogen Feb 18, 2016

Choose a reason for hiding this comment

divmain Feb 18, 2016

Choose a reason for hiding this comment

coopy commented Feb 23, 2016

ryan-roemer Feb 25, 2016

Choose a reason for hiding this comment

divmain commented Feb 25, 2016

coopy Feb 25, 2016

Choose a reason for hiding this comment

coopy Feb 25, 2016

Choose a reason for hiding this comment

coopy commented Feb 25, 2016

boygirl commented Feb 25, 2016

boygirl commented Feb 25, 2016

divmain commented Feb 25, 2016

divmain commented Feb 25, 2016

boygirl commented Feb 25, 2016

divmain commented Feb 25, 2016

divmain commented Feb 25, 2016

boygirl commented Feb 25, 2016

boygirl commented Feb 25, 2016

divmain commented Feb 25, 2016