Improved discovery time [4661] #411
Conversation
|
Build status:
|
| initialAcknackDelay.fraction = 200*1000*1000; | ||
| heartbeatResponseDelay.fraction = 20*1000*1000; | ||
| initialAcknackDelay.fraction = 300*1000*1000; | ||
| heartbeatResponseDelay.fraction = 50*1000*1000; |
There was a problem hiding this comment.
This was reduced from 500 to 20 in commit 201a393 to improve performance on some video tests. How will this affect this kind of cases?. Perhaps we should leave the defaults as they were and use specific values for discovery builtin readers.
There was a problem hiding this comment.
Done. Changes only applied to SEDP endpoints.
| heartbeatPeriod.seconds = 3; | ||
| nackResponseDelay.fraction = 20*1000*1000; | ||
| nackResponseDelay.fraction = 400*1000*1000; |
There was a problem hiding this comment.
This was reduced from 200 to 20 in commit 201a393 to improve performance on some video tests. How will this affect this kind of cases? I think this may produce a lot of bursts. Perhaps we should leave the defaults as they were and use specific values for discovery builtin writers.
There was a problem hiding this comment.
Done. Changes only applied to SEDP endpoints.
| * @param[in] writer Writer which creates this event. | ||
| * @param[in] interval_millisec Interval of the event in milliseconds. | ||
| */ | ||
| NackResponseDelay(StatefulWriter* writer, double interval_millisec); |
There was a problem hiding this comment.
New styling requires parameters on different lines
| * @param code Code representing the status of the event | ||
| * @param msg Message associated to the event | ||
| */ | ||
| void event(EventCode code, const char* msg= nullptr); |
There was a problem hiding this comment.
New styling requires parameters on different lines
|
Build status:
|
Co-Authored-By: richiware <correoricky@gmail.com>
|
Build status:
|
|
I did some additional testing with this PR yesterday, and it certainly seems to improve the situation. In the ROS2 application I'm working on, there about 2 dozen topics publishing at around 20Hz. Before this patch, it could take up to 30 seconds to do discovery. Afterwards, the worst case I saw was about 2 seconds, though the mean time for discovery was in the 10s-of-milliseconds range. We may still have to do some debugging on those long outliers (particularly since everything is localhost), but this is a huge improvement. Thanks! |
|
@clalancette, @richiware, @MiguelCompany It looks like a couple ROS 2 tests are failing after this PR was merged (See ros2/build_farmer#166, Tests pass in bc61fa3 but fail in cb0a6e2 ). Specifically the tests are timing out. What information should I gather to help debug? Failing tests |
Causing test failures (ros2/build_farmer#166)
Improved the time needed for discovering remote participants and theirs entities.