Fix integer overflow for oneshot timers.

[achim-k]

I noticed that the robot_state_publisher node 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 a ros::Timer with oneshot = 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:

// Stops the oneshot timer after callback execution. No significant cpu usage.
ros::Timer timer;
timer = nh.createTimer(ros::Duration(0.1), [&timer](const ros::TimerEvent&) {
  timer.stop();
}, true /* oneshot */);

Timer is not stopped:

// Oneshot timer not stopped, significant higher cpu usage (~10times as much).
ros::Timer timer;
timer = nh.createTimer(ros::Duration(0.1), [&timer](const ros::TimerEvent&) {
  ;  // no op
}, true /* oneshot */);

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

	int32_t remaining_time = std::max((int32_t)((sleep_end - current).toSec() * 1000.0f), 1);
	timers_cond_.timed_wait(lock, boost::posix_time::milliseconds(remaining_time));

with my debug values:

ros::Time sleep_end(2147483647, 999999999);
ros::Time current(1525190218, 443312022);

std::max((int32_t)((sleep_end - current).toSec() * 1000.0f), 1); /*
+                 +|                           |          +    +
|                 +-----    622293429.55    ---+          |    |
|                 |                                       |    |
|                 +-----    -476828920 (overflow)     ----+    |
|                                                              |
+--------------------------   1     ---------------------------+
*/

So it was always sleeping for 1ms, although it could have slept much longer.

[mikepurvis]

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?

[achim-k]

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>(...))

[mikepurvis]

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.

[mikepurvis]

Looks great, thanks for figuring this out and iterating on it!