opensim-org / opensim-core

SimTK OpenSim C++ libraries and command-line applications, and Java/Python wrapping.
https://opensim.stanford.edu
Apache License 2.0
801 stars 324 forks source link

Add ComponentPath overrides to Component interface #3782

Closed adamkewley closed 4 months ago

adamkewley commented 6 months ago

[perf-win]

… and integrate into Millard muscles and scalar actuators

Brief summary of changes

Test Name lhs [secs] σ [secs] rhs [secs] σ [secs] Speedup
ToyDropLanding_nomuscles 0.04 0.00 0.04 0.00 1.07
RajagopalModel 3.69 0.00 3.45 0.00 1.07
Gait2354 0.19 0.00 0.19 0.00 1.02
passive_dynamic_noanalysis 1.21 0.00 1.21 0.00 1.00
passive_dynamic 1.81 0.00 1.81 0.00 1.00
Arm26 0.16 0.00 0.16 0.00 1.02
ToyDropLanding 0.04 0.00 0.04 0.00 1.01

Testing I've completed

Looking for feedback on...

CHANGELOG.md (choose one)

Performance analysis

Platform: Windows, self-hosted runner

Test Name lhs [secs] stderr [secs] rhs [secs] stderr [secs] Speedup
Arm26 0.37 0.00 0.34 0.00 1.11
ellipsoid_wrap 4.10 0.01 4.10 0.00 1.00
ellipsoid_wrap_function_based_paths 3.44 0.01 3.45 0.01 1.00
Gait2354 0.41 0.00 0.42 0.00 0.97
MocoSlidingMass 1.48 0.01 1.45 0.01 1.02
MocoSquatToStand 14.19 0.09 13.47 0.04 1.05
passive_dynamic 5.67 0.02 5.60 0.00 1.01
passive_dynamic_noanalysis 3.42 0.01 3.43 0.00 1.00
RajagopalModel 8.89 0.02 8.19 0.00 1.09
ToyDropLanding 13.45 0.01 12.31 0.06 1.09
ToyDropLanding_fbp_stepwisereg 12.32 0.01 11.32 0.02 1.09
ToyDropLanding_function_based_paths 12.65 0.09 11.42 0.03 1.11
ToyDropLanding_nomuscles 0.57 0.00 0.58 0.00 0.99

This change is Reviewable

adamkewley commented 6 months ago

Speedup appears to be roughly 5 % on Rajagopal, which uses MillardEquilibriumMuscle:

Test Name lhs [secs] σ [secs] rhs [secs] σ [secs] Speedup
ToyDropLanding_nomuscles 0.04 0.00 0.04 0.00 1.11
RajagopalModel 3.82 0.00 3.59 0.00 1.06
Gait2354 0.20 0.00 0.19 0.00 1.03
passive_dynamic_noanalysis 1.22 0.00 1.23 0.00 1.00
passive_dynamic 1.83 0.00 1.82 0.00 1.00
Arm26 0.17 0.00 0.16 0.00 1.03
ToyDropLanding 0.04 0.00 0.04 0.00 1.02
adamkewley commented 4 months ago

New approach: check if it's a path/name at runtime

Doing it this way means that no downstream (from OpenSim::Component) code needs to change - for the cost of having to check if the string contains a directory separator.

If the string check is too expensive then we'd need to add overrides like getStateVariableValueByName so that code can explicitly state that the thing being provided is a name, not a path. However, I have avoided doing this here because it makes the overall API more confusing (some methods would be byName others would be suffix-less, but implicitly require a name (e.g. getOutput), and so on).

adamkewley commented 4 months ago
Test Name lhs [secs] σ [secs] rhs [secs] σ [secs] Speedup
ToyDropLanding_nomuscles 0.04 0.00 0.04 0.00 1.07
RajagopalModel 3.69 0.00 3.45 0.00 1.07
Gait2354 0.19 0.00 0.19 0.00 1.02
passive_dynamic_noanalysis 1.21 0.00 1.21 0.00 1.00
passive_dynamic 1.81 0.00 1.81 0.00 1.00
Arm26 0.16 0.00 0.16 0.00 1.02
ToyDropLanding 0.04 0.00 0.04 0.00 1.01
adamkewley commented 4 months ago

If you get a chance @nickbianco / @pepbos then this is ready for review.

It's mostly an isolated shortcut that handles the situation where (e.g.) getStateVariableValue is called with something like "somevar". The previous implementation assumes that all incoming strings might be paths to other components, so it spends a fair bit of CPU time (5-10 % when using Rajagopal model) converting to a ComponentPath, checking how many elements there are in the path, etc.

Instead, this version just checks if the path contains a ComponentPath::separator() (added in this PR - it's why the diffs are bigger than necessary, but I was getting sick of seeing "/" everywhere) and, if there's no separator, it does a direct lookup.

Note: the biggest gains for RajagopalModel are actually on getModelingInfo, which is called a lot more, and suffers from the same logic.

nickbianco commented 4 months ago

@adamkewley, I should have clarified: the perf suite will run on the next commit after adding [perf-win].