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

improve grid layout #1263

Merged
merged 11 commits into from
Apr 29, 2023
Merged

improve grid layout #1263

merged 11 commits into from
Apr 29, 2023

Conversation

gavin-ts
Copy link
Contributor

@gavin-ts gavin-ts commented Apr 29, 2023

Summary

Improves performance of grid layout with many objects and rows/columns, and improves even expanding of smaller objects in grid layout.

Details

  • adds slow_grid test previously 1.33s using 3.3GB (now <70ms <1.5MB)
  • adds grid_oom test previously would not complete and use too much memory (now <700ms, and <770MB)
  • improves evenly expanding of smaller objects when evening rows or columns
  • adds several optimizations to grid layout when searching for the best set of objects per row
  • fixes running ./ci/e2ereport.sh with -memprofile or -v being ineffective due to empty cpuprofile argument before it

new slow_grid test

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_regression_slow_grid_dagre_sketch exp svg

new grid_oom test (with improved object scaling)

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_regression_grid_oom_dagre_sketch exp svg

new grid_even test

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_stable_grid_even_dagre_sketch exp svg

grid_oom test before improving object scaling

_Users_gavinnishizawa_github_repos_d2_e2etests_testdata_regression_grid_oom_dagre_sketch got svg (2)

previously the smallest node e would grow first becoming larger than c and d

Screen Shot 2023-04-28 at 8 13 31 PM

now these are scaled evenly

Screen Shot 2023-04-28 at 8 13 23 PM

@gavin-ts gavin-ts marked this pull request as ready for review April 29, 2023 03:12
@gavin-ts gavin-ts requested a review from a team April 29, 2023 03:12
@gavin-ts gavin-ts enabled auto-merge April 29, 2023 03:17
@gavin-ts gavin-ts merged commit 7f821f2 into terrastruct:master Apr 29, 2023
// skip options with a row that is 1.2*longer or shorter
// Note: we want a low threshold to explore good options within attemptLimit,
// but the best option may require a few rows that are far from the target size.
okThreshold := 1.2
Copy link
Contributor

Choose a reason for hiding this comment

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

why 1.2?

// but the best option may require a few rows that are far from the target size.
okThreshold := 1.2
// if we don't find a layout try 25% larger threshold
thresholdStep := 0.25
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the convention is to have this as a constant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

var bestLayout [][]*d2graph.Object
bestDist := math.MaxFloat64
count := 0
attemptLimit := 100_000
Copy link
Contributor

Choose a reason for hiding this comment

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

constant

divisions = append(divisions, append(inner, index-1))
}
iterDivisions(objects[:index], nCuts-1, func(inner []int) bool {
done = f(append(inner, index-1))
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if these could end up with those issues of slice references since you're expanding it recursively

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