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

Partition optimization #443

Merged
merged 5 commits into from
Oct 5, 2022

Conversation

hellkite500
Copy link
Member

@hellkite500 hellkite500 commented Sep 13, 2022

Significant performance concerns were raised when attempting to use the partitionGenerator on a CONUS hydrofabric. After a cursory look at the code, some significant optimizations are possible by simply reducing the amount of copying that happens, especially when functions are called inside of large loops. This PR addresses some low hanging fruit for optimizing the partitionGenerator by simply using references instead of copies/values.

Preliminary analysis shows these changes decrease runtime by nearly 50% on VPU 6, and now most all the run time in that run is in reading the initial catchment data. (20 seconds of the total 23.6 seconds).

@mattw-nws this should help a bit with your CONUS partitioning.

EDIT:
Was able to run the conus partitioning on my macbook in ~15 minutes (after doing a little more memory efficiency in the geojson reader keep from crashing when I ran out of swap and RAM...I'll push those edits in another PR).

I will also note that to get to that speed, I was using a RELEASE build, not a DEBUG build. The boost ptree reading is highly sensitive to that!

cmake -DCMAKE_BUILD_TYPE=Release makes reading the geojson significantly faster!

Additions

Removals

Changes

  • Pass as many variables as possible by const reference
  • Refactor the validation algorithm from a O(n^2) to O(n log n) implementation.

Testing

Screenshots

Notes

  • The Network could be passed by reference, but not const reference due to many nested functions that were not const qualified.

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

  1. Tested on VPU 06

Target Environment support

  • Linux
  • MacOs

Copy link
Contributor

@donaldwj donaldwj left a comment

Choose a reason for hiding this comment

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

I don't see any issues

@hellkite500 hellkite500 mentioned this pull request Sep 15, 2022
12 tasks
@mattw-nws mattw-nws merged commit ba28b8b into NOAA-OWP:master Oct 5, 2022
@hellkite500 hellkite500 mentioned this pull request Apr 26, 2023
11 tasks
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