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

Graph layout optimizations #2128

Merged
merged 3 commits into from
Feb 2, 2017
Merged

Graph layout optimizations #2128

merged 3 commits into from
Feb 2, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Jan 13, 2017

This PR aims at improving the performance of rendering graph layouts for a high number of nodes (see #1188). The changes made here are supposed to affect only performance, not in any way the layout being rendered - for discussion about alternative layouts, check out #1614 and the corresponding experimental branch #2119.

While a bunch of small optimizations have been applied successfully, I should stress out that the real bottleneck for dynamic layouts lays in https://github.com/cpettitt/dagre library, which we are using to get the nodes' coordinates for a nice layout. This is generally a difficult problem, but in case we'd want to stick with the current layout, we should consider alternatives to dagre for any further significant performance improvements, as this library is not maintained anymore.

Some of the changes include:

  • The node shapes are now rendered at a unit scale and their size and position is now controlled only at the top level - that enables both panning and zooming to be seamless for very big graphs as no nodes get re-rendered.
  • Moving all of the zooming logic to the top also simplified a lot of code and resolved the tiny rendering bug where the node borders were rendered with different thickness depending on the size of the graph.
  • Motion animations for both nodes and edges are disabled for a high node count graphs to
    prevent lagging effects.
  • The layout and zooming logic in nodes-chart.js has been completely refactored by moving it to selectors (see https://github.com/reactjs/reselect).

Suggestions for future optimizations:

  • When clicking on a node for details, or doing a search query, most of the nodes get faded out, while a couple of them get in focus. Fading out happens on individual node's level, so it gets very expensive on big graphs. Two ways around this issue might be:
    • Trying to control opacity through some kind of group element - not sure how this could work for search though, as massive regrouping of nodes might also be expensive.
    • Simplify the node rendering or cache them as SVG objects to render them faster.
  • Selectors seem like a nice thing to use in many complex scenarios, both for optimization (as a way to avoid unnecessary recalculations) and as a way to make the code more readable and 'linear'.

NOTE: To test the contrast mode, I'll need to rebase with master again once #2165 gets fixed.

@fbarl fbarl self-assigned this Jan 13, 2017
@rade
Copy link
Member

rade commented Jan 17, 2017

This is meant to help with #1188, right?

@fbarl fbarl force-pushed the graph-layout-optimizations branch 7 times, most recently from 4d6f757 to 8925c66 Compare January 25, 2017 11:23
@fbarl fbarl force-pushed the graph-layout-optimizations branch 2 times, most recently from 5bc21ed to 7bbc6a8 Compare January 30, 2017 10:44
@fbarl fbarl requested a review from davkal January 30, 2017 13:38
@fbarl fbarl changed the title [WIP] Graph layout optimizations Graph layout optimizations Jan 30, 2017
@fbarl fbarl requested a review from foot January 30, 2017 13:42
@fbarl
Copy link
Contributor Author

fbarl commented Jan 30, 2017

@rade Yes, I finally explained the PR in the comment above.

@rade
Copy link
Member

rade commented Jan 30, 2017

dagre ...is not maintained anymore.

ouch!

@rade
Copy link
Member

rade commented Jan 30, 2017

please file an issue to look for alternatives.

@foot
Copy link
Contributor

foot commented Jan 30, 2017

Nice one!

Small UI nits

  • zoom extents should be re-calculated on filter change (noticeable on pods, going from default -> all namespaces (~10 nodes -> ~100) and now I can't zoom all the way out!)
  • edges are bit faint on larger layouts (but edges start getting a bit useless on large layouts too so maybe this is okay)
  • lack of animation is quite jarring but its really nice having the snappiness!
    • maybe disable the details panel animation too so everything pops up together?`
  • font size when overlaying metrics is a smidge too big. maybe drop it from 12px to 11, or even 10?

@fbarl fbarl force-pushed the graph-layout-optimizations branch from 7bbc6a8 to 3ae0b9d Compare February 1, 2017 11:41
@fbarl
Copy link
Contributor Author

fbarl commented Feb 1, 2017

Thanks for your comments @foot, here's my response:

  • Instead of just changing the zoom extents when filters are changed, I treat it now as different graph views with their own caching state. So for the zooming behaviour, there is no difference between switching topologies and switching filters within a topology. Your 10 -> 100 nodes example gives a good argument why.
  • I made the edges 25% thicker, but this is something we can always change easily if we get such feedback.
  • Even though the details panel transition is set with one line of CSS, controlling the animation would require the details panel component to be aware of the graph context to assess whether it's complex or not, which is currently not the case. So I decided not to do anything about it for now, as proper solution would require moving some code up and possibly unifying with the graphExceedsComplexityThresh criterion used for switching to grid mode automatically and I'd rather if we did this refactoring in a separate story.
  • Dropped it to 10px and looks better indeed!

@foot
Copy link
Contributor

foot commented Feb 1, 2017

Code looks really good tbh. Nice work!

LGTM


For another PR perhaps: I think it would be interesting to explore making the selectors "purer" in a sense, making them dependent only on on state (rather than props too), and that being the global state rather then local.

  • nodes-chart.js doesn't really need to know about the layout.
  • node-container.js could have a mapStateToProps: getCoords(nodeId)(state) which could be a chain of selectors: globalState -> nodes -> nodeAdj -> nodeLayout -> nodeLayout[nodeId]
  • I think window width / height would have to be pushed up into the globalstate for this to work (maybe something else).

Would give quite a nice separation of cmp tree and selector/data/state tree.

@fbarl fbarl force-pushed the graph-layout-optimizations branch from 7b31b8c to c802c8c Compare February 1, 2017 15:27
@fbarl fbarl merged commit 95c688d into master Feb 2, 2017
@fbarl
Copy link
Contributor Author

fbarl commented Feb 2, 2017

@foot, about the changes you are suggesting to make selectors purer by using them only on the global state - my understanding is that that's precisely the way they were intended to be used, but I don't really understand why we should restrict this pattern to the global state only. In this particular case, it would work fine by moving some code up the hierarchy because we are using nodes-chart as a singleton component, but how would we go about if one day we wanted to use them on e.g. the states of individual nodes? I'm not sure if moving everything complex to the global state is always a way to go, but then again, I'm still learning the ways of React :)

For the particular suggestions you made above, I think it's actually a very good idea and we could further benefit from it if we cached all the layout states globally, so that switching between them would never require layout recalculation.

@fbarl fbarl deleted the graph-layout-optimizations branch February 2, 2017 10:18
@fbarl fbarl restored the graph-layout-optimizations branch February 2, 2017 10:41
fbarl added a commit that referenced this pull request Feb 2, 2017
fbarl added a commit that referenced this pull request Feb 3, 2017
@fbarl fbarl deleted the graph-layout-optimizations branch February 8, 2017 17:49
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