Fix integer overflow for oneshot timers.#1382
Conversation
| // On system time we can simply sleep for the rest of the wait time, since anything else requiring processing will | ||
| // signal the condition variable | ||
| int32_t remaining_time = std::max((int32_t)((sleep_end - current).toSec() * 1000.0f), 1); | ||
| int64_t remaining_time = std::max((int64_t)((sleep_end - current).toSec() * 1000.0f), (int64_t)1); |
There was a problem hiding this comment.
IMO the second cast should not be required in order for std::max to do the right thing.
Can you also use static_cast vs a c-style cast, please?
There was a problem hiding this comment.
IMO the second cast should not be required in order for std::max to do the right thing.
Gave me the following error in that case: deduced conflicting types for parameter ‘const _Tp’ (‘long int’ and ‘int’). Explicitly templating std::max<int64_t>(...) would probably have been better than casting both arguments. With my latest commit it's a static_cast<int64_t>(std_max<double>(...))
There was a problem hiding this comment.
Is there something which prevents doing std::max<int64_t>? That does indeed seem better than doing the max against a floating point value that's then cast back to an integer.
|
Looks great, thanks for figuring this out and iterating on it! |
* Template std::max instead of casting explicitly.
* Template std::max instead of casting explicitly.
I noticed that the
robot_state_publishernode takes a significant of cpu time although it's basically doing nothing (urdf with only fixed joints). After debugging a bit, I noticed that the cpu consumption stems from aros::Timerwithoneshot = true:Interestingly, when stopping the oneshot timer after its callback has been called, almost no cpu is consumed. This I could verify by creating two simple nodes, one that stops the timer and one that doesn't, and comparing their consumed cpu time:
Timer is stopped:
Timer is not stopped:
Long story short: The reason for the higher cpu usage was an integer overflow for oneshot timers in
ros_comm/clients/roscpp/include/ros/timer_manager.h
Lines 579 to 580 in e96c407
with my debug values:
So it was always sleeping for 1ms, although it could have slept much longer.