Don't reparse parent elements when cloning. #1484

arjo129 · 2024-10-01T08:01:09Z

🦟 Bug fix

Fixes #

Summary

Improves performance on sdf::Element::Clone by not requiring the parent elements to be reparsed. This was originally part of #1479. But it makes sense to seperate out the work.

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

construction I'm just checking if adding a cache makes any sense. Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

…osim/sdformat into arjo/fix/clone_parent_only

scpeters · 2024-10-14T07:27:37Z

I think macOS build failures are due to osrf/homebrew-simulation#2834

scpeters · 2024-10-14T07:38:04Z

I ran the PERFORMANCE_parser_urdf test several times with std_err redirected to /dev/null and see about a 18% reduction in average test time (1 - 0.928 / 1.133) ~= 0.18

sdf15

Ran the test 10 times:

Minimum	Average	Maximum
1.110 s	1.133 s	1.149 s

$ time for i in $(seq 10); do bin/PERFORMANCE_parser_urdf 2> /dev/null ; done
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1103 ms)
[----------] 1 test from URDFParser (1103 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1103 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1123 ms)
[----------] 1 test from URDFParser (1123 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1123 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1107 ms)
[----------] 1 test from URDFParser (1107 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1108 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1149 ms)
[----------] 1 test from URDFParser (1149 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1149 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1117 ms)
[----------] 1 test from URDFParser (1117 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1117 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1100 ms)
[----------] 1 test from URDFParser (1100 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1100 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1109 ms)
[----------] 1 test from URDFParser (1109 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1109 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1112 ms)
[----------] 1 test from URDFParser (1112 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1112 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1116 ms)
[----------] 1 test from URDFParser (1116 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1116 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (1118 ms)
[----------] 1 test from URDFParser (1118 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1118 ms total)
[  PASSED  ] 1 test.

real	0m11.335s
user	0m8.720s
sys	0m2.549s

this branch

Ran the test 10 times:

Minimum	Average	Maximum
0.890 s	0.928 s	0.945 s

$ time for i in $(seq 10); do bin/PERFORMANCE_parser_urdf 2> /dev/null ; done
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (906 ms)
[----------] 1 test from URDFParser (906 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (906 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (894 ms)
[----------] 1 test from URDFParser (894 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (894 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (920 ms)
[----------] 1 test from URDFParser (920 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (920 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (892 ms)
[----------] 1 test from URDFParser (892 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (892 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (923 ms)
[----------] 1 test from URDFParser (923 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (923 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (921 ms)
[----------] 1 test from URDFParser (921 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (921 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (908 ms)
[----------] 1 test from URDFParser (908 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (908 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (945 ms)
[----------] 1 test from URDFParser (945 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (945 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (903 ms)
[----------] 1 test from URDFParser (903 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (903 ms total)
[  PASSED  ] 1 test.
Running main() from /Users/scpeters/ws/ionic/src/sdformat/test/gtest_vendor/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from URDFParser
[ RUN      ] URDFParser.AtlasURDF_5runs_performance
[       OK ] URDFParser.AtlasURDF_5runs_performance (890 ms)
[----------] 1 test from URDFParser (890 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (890 ms total)
[  PASSED  ] 1 test.

real	0m9.280s
user	0m6.680s
sys	0m2.490s

arjo129 and others added 3 commits September 6, 2024 07:42

Attempting to see if adding a cache will speed up the sdf::Root

0868abf

construction I'm just checking if adding a cache makes any sense. Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

Remove cacheing code. Make faster.

91228de

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

Style

0977059

Signed-off-by: Arjo Chakravarty <arjoc@google.com>

arjo129 requested review from azeey and scpeters as code owners October 1, 2024 08:01

Merge branch 'sdf15' into arjo/fix/clone_parent_only

f544cc7

github-actions bot added the 🏛️ ionic Gazebo Ionic label Oct 1, 2024

arjo129 changed the title ~~Arjo/fix/clone parent only~~ Don't reparse parent elements when cloning. Oct 1, 2024

arjo129 added 2 commits October 10, 2024 09:41

Docstring

08f157d

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>

Merge branch 'arjo/fix/clone_parent_only' of https://github.com/gazeb…

fc7a54c

…osim/sdformat into arjo/fix/clone_parent_only

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't reparse parent elements when cloning. #1484

Don't reparse parent elements when cloning. #1484

arjo129 commented Oct 1, 2024

scpeters commented Oct 14, 2024

scpeters commented Oct 14, 2024

Don't reparse parent elements when cloning. #1484

Are you sure you want to change the base?

Don't reparse parent elements when cloning. #1484

Conversation

arjo129 commented Oct 1, 2024

🦟 Bug fix

Summary

Checklist

scpeters commented Oct 14, 2024

scpeters commented Oct 14, 2024

sdf15

this branch