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

Speedup component path traversal #3955

Merged
merged 2 commits into from
Oct 31, 2024

Conversation

adamkewley
Copy link
Contributor

@adamkewley adamkewley commented Oct 30, 2024

Adds (internal) Component::forEachImmediateSubcomponent, Component::findImmediateSubcomponentByName APIs and uses them to reimplement Component (so that it isn't manually looping as much) and reimplement traversePathToComponent (which was over-templated and allocated memory for subcomponent vectors.

I noticed that path traversal in OpenSim takes significant time. One of the reasons that this is the case is because traversePathToComponent ends up allocating and pushing pack pointers to subcomponents in order to perform a linear name search.

Fixes issue N/A

Brief summary of changes

  • Adds a private helper function: Component::forEachImmediateSubcomponent, which can be used in cases where code wants to iterate over the three collections of subcomponents stored by Component.
  • Adds a private helper function: Component::findImmediateSubcomponentByName, which can be used to find an immediate subcomponent by its name (currently, via linear probing).
  • Rolls out forEachImmediateSubcomponent the parts of Component where it makes sense, so that the code is ready for a next-stage refactor (e.g. of adding a Subcomponents class that encapsulates the subcomponent mess).
  • Rolls out findImmediateSubcomponentByName to traversePathToComponent, so that it can find an immediate subcomponent by name without having to first allocate a vector of pointers to subcomponents
  • Cleans up traversePathToComponent

Testing I've completed

  • Ran the existing test suite, which uses this code very very extensively

Looking for feedback on...

CHANGELOG.md

  • updated.

This change is Reviewable

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

LGTM.

Nice refactor! How much speed up are you seeing with this?

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @adamkewley)

@nickbianco nickbianco merged commit 132a2d9 into main Oct 31, 2024
13 checks passed
@nickbianco nickbianco deleted the feature_faster-component-path-traversal branch October 31, 2024 22:38
@adamkewley
Copy link
Contributor Author

When I measured it pre-PR, it was spending around 5 % of CPU here, However, I have subsequently found , while working on OBJ/VTP parsing changes in opensim-models, that the test suite I'm profiling gives very different results between running Visual Studio's explicit profiler (which takes a little bit of setup) and just pausing + viewing the debug profiler when running the test suite during a slow-running test (i.e. how I usually spot-check perf problems when working on OpenSim Creator).

In retrospect, i should've written a bash/python script that exercises this bottleneck so there's a clear before/after metric, but I saw the code and thought "surely this can be cleaned up" and hopped right in. OpenSim Creator now has an inlined build mode which, combined with the unified osim (Simbody+OpenSim in one target) build means that I can make these changes and then recompile+test the entire stack in one step.

@adamkewley
Copy link
Contributor Author

Not particularly scientific, but here's a VS profile (full) on a test suite that is continually deleting + refinalizing, etc. components in Rajagopal2015.osim. It's one of my slower tests because there's a lot of components in the model:

image

So ~2.5 % in that case. Nothing huge, but it did bug me that it was popping up (considering what it's doing).

@nickbianco
Copy link
Member

Thanks for the follow-up @adamkewley. I was mostly about a rough number, so this is great. It is a clear performance improvement just looking at the code change.

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.

2 participants