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

Layout rendering dynamic optimizations #2221

Merged
merged 6 commits into from
Feb 20, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Feb 9, 2017

In response to #1188:

Most relevant optimizations

  • Translation/scaling of the nodes is now fully managed on the NodeContainer component level, so nodes are only rerendered when they change internally and not when they are repositioned in the layout.
  • Caching the metrics that is displayed inside the nodes in the selectors.
  • The search matching logic has also been partly moved to selectors and the difference can be felt when searching through the big graphs. Also, excessive recalculations in some special circumstances (e.g. when resfreshing the page with a broad search query) have been eliminated.

Layout changes

  • Slightly decreased the distances between the nodes in the graph layout (to me it looks better this way).
  • Doubled the treshold in the graph complexity heuristics as I believe we can now handle bigger graphs :)

Other changes

  • Renamed subLabel to labelMinor everywhere for consistency with the grid mode and the backend naming.

Some ideas for the future

@fbarl fbarl self-assigned this Feb 9, 2017
@fbarl fbarl force-pushed the layout-rendering-dynamic-optimizations branch 7 times, most recently from c4cd37f to 20118b3 Compare February 14, 2017 15:42
@fbarl fbarl changed the title [WIP] Layout rendering dynamic optimizations Layout rendering dynamic optimizations Feb 14, 2017
@fbarl fbarl requested a review from davkal February 14, 2017 15:43
} else {
this.setState({ matched: false });
}
}

This comment was marked as abuse.

Copy link
Contributor

@davkal davkal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent use of selectors. If anything I'd look into extracting adjacency for nodes into its own selector. Currently the graphLayout() selector is called on every received delta. Also, might be worth revisiting the deepEqual selectors that have been removed with this PR.

Otherwise LGTM

[
allNodesSelector,
],
allNodes => allNodes.filter(node => !node.get('filtered'))

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

);


export const nodeAdjacenciesSelector = createSelector(

This comment was marked as abuse.

This comment was marked as abuse.

}

export default connect(mapStateToProps)(NodesChartElements);
export default connect()(NodesChartElements);

This comment was marked as abuse.

This comment was marked as abuse.

@@ -64,7 +64,7 @@ describe('RootReducer', () => {
}
],
stats: {
edge_count: 319,
edge_count: 379,

This comment was marked as abuse.

This comment was marked as abuse.

@fbarl fbarl force-pushed the layout-rendering-dynamic-optimizations branch from 20118b3 to 19854d5 Compare February 16, 2017 18:38
@fbarl
Copy link
Contributor Author

fbarl commented Feb 16, 2017

@davkal PTAL, maybe just at the last commit, just to check if I've identified all the points you raised in the comments.

@fbarl fbarl requested a review from jpellizzari February 17, 2017 10:29
Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some quick pseudo-science around wasted renders and found a significant improvement. I executed the same actions on both branches; here is the result:
Master:
scope-master

This PR:
layout

The code looks fine. I was not able to do a functional test, because we don't have a great way to roll out an experiment to dev, then roll it back with flux.

One comment around isEmpty in node.js, otherwise LGTM!


const color = getNodeColor(rank, label, pseudo);
const truncate = !focused && !hovered;
const labelOffsetY = (showingNetworks && networks) ? 40 : 28;

const nodeClassName = classnames('node', {
// NOTE: Having a CSS animation here might not be the best idea.
// See https://github.com/weaveworks/scope/issues/2255
matched: !matches.isEmpty(),

This comment was marked as abuse.

This comment was marked as abuse.

@fbarl fbarl force-pushed the layout-rendering-dynamic-optimizations branch from 19854d5 to e5c655a Compare February 20, 2017 10:10
@fbarl
Copy link
Contributor Author

fbarl commented Feb 20, 2017

Thanks for the review and the benchmarks @jpellizzari! I didn't expect the numbers to be so much better with my PR, but I think some of it is compensated with the slowness of deep comparison of the props (which now happens more often I think).

@fbarl fbarl merged commit 76fec79 into master Feb 20, 2017
@fbarl fbarl deleted the layout-rendering-dynamic-optimizations branch March 24, 2017 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants