Stabilize test_record by reducing copies of executors and messages#576
Merged
emersonknapp merged 2 commits intomasterfrom Dec 4, 2020
Merged
Stabilize test_record by reducing copies of executors and messages#576emersonknapp merged 2 commits intomasterfrom
emersonknapp merged 2 commits intomasterfrom
Conversation
…he global context shutdown Signed-off-by: Emerson Knapp <eknapp@amazon.com>
thomas-moulard
approved these changes
Dec 3, 2020
jaisontj
approved these changes
Dec 3, 2020
f35e06d to
ff81785
Compare
…eference Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Collaborator
Author
|
Gist: https://gist.githubusercontent.com/emersonknapp/d47520e678e57235041ab30236612b47/raw/bce3c825c328d028c5604e18798bf07c217c9976/ros2.repos |
Member
|
@emersonknapp Thanks for the fix! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #528
Tested locally with
As well as 1400 successful sequential runs (closed at that point, not failed) of
test_record__rmw_fastrtps_cppIntroduces 2 changes which both caused issues:
Executor copies (wait_for.hpp)
Every time we called spin_some, a new executor was constructed. This would happen potentially a few thousand times per test. Given that the global context was not shutdown during this time, it would accumulate
on_shutdown_callbacks_, one from each executor that was created. When this list was very large we would occasionally getstd::bad_function_callfor one of the callbacks in the shutdown.With this change, the global
Contexthas only 2 items inon_shutdown_callbacks_, and I am not able to reproducebad_function_callany more.After this change I still see a segfault, usually after 100-500 iterations of (for example)
RecordIntegrationTestFixture.records_sensor_data. I'm still investigating this, but it is a separate error.STL container copies
The
MockSequentialWriterused intest_recordwould pass its internal containers back to viewers by value. This created many copies of theshared_ptr<SerializedBagMessage>s. Upon running repeatedly, this copy operation would occasionally end in a segfault. Since outside theMockSequentialWriter, the values are only observed, we are able to avoid that situation by never copying the values and returning const references to the existing structures.Conclusion
Though it's a little unclear exactly what caused either issue at the cause end, both changes are best-practices more correct code and avoid the situations that brought about the problems, which improves performance and stability of these tests.
Recommended followups:
rclcpp::Contextthrowsbad_function_callon shutdown whenon_shutdown_callbacks_is very large. A simple repro case should be easy enough to construct