Potential clock duplication in TimeSource

[luca-della-vedova]

Hello! I was working on an rcl client library and was using rclcpp (and rclpy) as reference implementation, and when working on TimeSource I found a potential issue.

Specifically, when looking at the TimeSource implementation for both rclpy and rclcpp, I noticed that for each time source, its associated clocks are stored in a vector.

Now apart from the premature-optimization-root-of-all-evil issue of having finds be linear in time here, I believe the fact that clocks are naively appended in attachClock would allow duplicates in the vector. Now the drawback is not the end of the world since it's just that the same clock would be updated more than once for each subscription callback but would it make sense to either:

• Do an std::find call in attachClock and print a warning + avoid appending if the clock already exists
• Make the attached clocks storage structure a unordered_set?

If any of these makes sense I'll be happy to open upstream PRs in both rclpy and rclcpp since they have the same implementation (rclpy is here)

[alsora]

I'm not very familiar with TimeSource, but it seems to me that changing the vector into an unordered_set would be a good improvement.

[clalancette]

I will say that from a performance POV, use of a std::vector probably isn't a problem. I don't think there are common cases where we are attaching more than one Clock to a TimeSource (though if you have counter-examples, I'm happy to be proven wrong).

However, I do agree that storing a clock more than once in that vector seems unnecessary. From that perspective, I'm also fine with switching that to be a std::unordered_set.

[fujitatomoya]

I do agree that storing a clock more than once in that vector seems unnecessary. From that perspective, I'm also fine with switching that to be a std::unordered_set.

So do I.

I don't think there are common cases where we are attaching more than one Clock to a TimeSource

I dont quite figure out the use case for this either, any thoughts?

[luca-della-vedova]

I don't think there are common cases where we are attaching more than one Clock to a TimeSource

I dont quite figure out the use case for this either, any thoughts?

I honestly don't know and I can't come up with any either, I was just reviewing the API and bumped into this corner case.
If having multiple clocks attached to a time source is a use case that isn't supported then another possibility could be for the time source to just store a single clock pointer in it and have the attach / detach APIs overwrite it.

[clalancette]

If having multiple clocks attached to a time source is a use case that isn't supported then another possibility could be for the time source to just store a single clock pointer in it and have the attach / detach APIs overwrite it.

While I sympathize with this idea, the use of a vector seems deliberate (and has been there in all incarnations of this code). Maybe @tfoote can answer why associated_clocks is a vector, since he wrote the original code.

My suggestion is to switch this to a std::unordered_set for now. It may be overkill (depending on the results of the conversation above), but it at least keeps the possibility to have multiple clocks attached to a TimeSource.

[tfoote]

Using multiple clocks is not our default use case, but is important for libraries which may need to have non-trivial interactions with the clock interface. This library should be able to have an internal clock instance and use that. And when used in a node just attach that clock instance to the nodes time source to be able to use ROS time without changing any of the library's implementation.

It should be documented that an individual clock should not be multiply associated. Right now the failure mode is that if that's done the clock will get multiple callbacks. And then when removed it will be doubly removed. I think that making it an unordered_set makes sense. It will avoid the duplicated callbacks and be clear when removed that it will be unique. I don't think that it would make sense to add the overhead of trying to track the clocks with handles for deduplicating the callbacks, while supporting multiple associations of clocks.

[fujitatomoya]

@luca-della-vedova are you willing to work on PR? i am happy to do review.