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

[KED-2212] Improve graph performance #343

Merged
merged 10 commits into from
Jan 8, 2021
Merged

[KED-2212] Improve graph performance #343

merged 10 commits into from
Jan 8, 2021

Conversation

bru5
Copy link
Contributor

@bru5 bru5 commented Jan 5, 2021

Description

Improves performance of the graph layout engine.

Development notes

In my tests on a large graph (860 nodes, 1500 edges), reduces its render time by about 60% / 2.5x boost.

This refactoring improves graph performance by:

  • using static base constraints and constants
  • using fully defined constraint values rather than defaults to avoid run time cost
  • reducing assignments for creating constraint objects
  • reducing memory usage for constraint objects
  • removing spread operator when creating constraints (it compiles to a slower polyfill)

This is a refactoring only, the algorithm and the output remains the same.

Another benefit is that these changes also generally improve clarity being more explicit and with more abstraction too.

QA notes

No change to behaviour or results other than performance.

Test using a very large pipeline and compare render time.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

/**
* Constraint in Y for separating rows
*/
export const rowConstraint = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be really helpful to add a comment to explain what each field under the Constraint object is used for and which parts of the graph it affects - would definitely be helpful to provide clarity for future development work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes the docs for these fields are on the solve function in solver.js, I'll add a comment in this file to make note of that ten

@studioswong
Copy link
Contributor

interesting work @bru5 - as someone new to this codebase, it would be really helpful if you could provide some context to the graph layout engine setup in the PR description, particularly the reasoning behind some of the refactoring work done here that results in the improvement of the graph performance - is this based on any algorithms / graphing theory? would definitely love to understand and explore the ideas behind the current graph setup.

Wondering if we could also add in some tests to test the expected constraint values based on the animals dataset? The test case setup would also help to provide clarity on the expected behaviour of the graph layout engine as well.

All in all, very interesting stuff!

@bru5
Copy link
Contributor Author

bru5 commented Jan 6, 2021

@studioswong thanks for looking over the PR

Yeah I'd love to discuss the approach in more detail but I think discussion of the algorithm is better left out of this particular PR as it's purely a code refactoring of the existing algorithm.

The only changes made are already in the PR description, but that's a good point I've added a note to clarify that there is no algorithmic change.

See graph.test.js for the unit tests, which includes the constraint and solver system which the layout algorithm is built on.

Testing against a specific dataset like animals would require a sophisticated verification / evaluation algorithm which is not currently implemented because it is not straightforward, so evaluating against datasets is manual for now but this would be a nice future addition.

@bru5 bru5 requested a review from studioswong January 7, 2021 12:29
Copy link
Contributor

@richardwestenra richardwestenra left a comment

Choose a reason for hiding this comment

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

Can confirm, significantly faster. Graph shape looks about the same. Spectacular work!

@richardwestenra
Copy link
Contributor

Note: JS tests are failing.

Also I recommend merging main as the Python tests on that branch have been fixed.

@bru5 bru5 merged commit a2f18a9 into main Jan 8, 2021
@bru5 bru5 deleted the refactor/optimise-graph branch January 8, 2021 11:50
@richardwestenra richardwestenra mentioned this pull request Feb 17, 2021
1 task
richardwestenra added a commit that referenced this pull request Feb 19, 2021
# Release 3.9.0

## Major features and improvements

- Add code panel (#346)
- Improve view panning behaviour when a node is selected (#356, #363, #370, #373, #374)
- Improve layout performance for large graphs (#343)
- Save tag state to localStorage (#353)

## Bug fixes and other changes

- Improve graph layout code quality, performance and docs (#347)
- Update docs to remind on compatibility of Kedro-Viz 3.8.0 with Kedro 0.17 (#367)
- Update dependencies (#360, #364, #369)
- Fix failing CircleCI build on Windows (#365, #366)
- Refactor node-list-row CSS, fixing hover and focus states (#355, #358, #362)
- Update iconography (#357, #359)
- Fix missing indicator Chrome zoom bug (#349)
- Fix sidebar border/box-shadow CSS rules (#351)
- Fix server.py to work with versions >0.17 and update contributing docs (#348)
- Fix errors when scrolling with empty pipeline (#342)
- Ignore coverage on some branches and fix e2e tests (#345)
- Fix icon-button tooltips on mobile (#344)
- Update SVG-Crowbar to fix errors (#339)
- Update contributing docs for new dev server (#341)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants