Open ZachG-gnu opened 2 years ago
Please feel free to open a pull request with your changes, and we can go ahead and review it.
My reason for giving the user this capability is so they can remove the first iteration which has a much higher jitter than the other iterations due to it allocating memory at that time.
@ZachG-gnu I think most extra latency from the first sample is coming from here: https://github.com/ros2/realtime_support/blob/master/rttest/src/rttest.cpp#L552-L553. Commenting these lines reduce the latency greatly for the first iteration.
My proposal would be to remove all these blocking calls from rttest
APIs that are expected to be call during real-time execution. Adding a function to clear statistics still makes sense to me to be able to run multiple experiments in the benchmark execution (i.e. whenever a lifecycle node goes into active state).
My reason for giving the user this capability is so they can remove the first iteration which has a much higher jitter than the other iterations due to it allocating memory at that time.
@ZachG-gnu I think most extra latency from the first sample is coming from here: https://github.com/ros2/realtime_support/blob/master/rttest/src/rttest.cpp#L552-L553. Commenting these lines reduce the latency greatly for the first iteration.
My proposal would be to remove all these blocking calls from
rttest
APIs that are expected to be call during real-time execution. Adding a function to clear statistics still makes sense to me to be able to run multiple experiments in the benchmark execution (i.e. whenever a lifecycle node goes into active state).
Very interesting find! I was able to reduce the latency of the first iteration by 20ns just by removing those two lines. I agree that any kind of unnecessary blocking calls should be removed, especially if it only runs for a certain iteration to avoid outliers as much as possible.
Feature request
Feature description
I would like to give my user the capability to clear the current statistics, however rttest does not expose an API to clear the sample buffer. My reason for giving the user this capability is so they can remove the first iteration which has a much higher jitter than the other iterations due to it allocating memory at that time. My proposed solution is to expose a method in rttest that the client can call to clear the current sample buffer and reset the current results.
Implementation considerations
The solution sadly isn't as straightforward as clearing the sample buffer properties since there are a few methods that try to access elements using the current iteration. There is also no way to just reset the iteration back to zero since it is defined locally here:
SInce there is no way to reset the iteration, my solution was to add a property to Rttest that stores the iteration that the sample buffer was cleared at. This way when the statistics are calculated, it could use this property as an offset causing it to ignore all statistics before this iteration. I defined a private property in Rttest as so:
The implementation logic is pretty straightforward. FIrst, it sets the cleared_iteration property with the current results iteration. It then resets some of the results properties, such as the max and min latency. The method is shown below:
The API method that the user can call:
The only method that needs to be modified is calculate_statistics(...). Since calculate_stddev(...) requires a vector to be passed in, I had to create a sliced copy of the latency_samples. The alternative solution to creating a copy would be to create another calculate_stddev(...) method that takes in the beginning and end iterators instead of a vector. The code for the modified calculate_statistics(...) is shown below:
After some testing this implementation is working good for me. It would be very inconvenient to require my user to install a different version of rttest with my patch just to enable them to clear the statistics which is why I'm asking for this feature to be added. Even if my proposed solution isn't accepted, I would like to see this feature added as I'm sure this would be useful for other users as well.