Pre-allocate rclcpp::Time's in the main loop#640
Pre-allocate rclcpp::Time's in the main loop#640AndyZe wants to merge 1 commit intoros-controls:masterfrom
Conversation
aprotyas
left a comment
aprotyas
left a comment
There was a problem hiding this comment.
No strong opinion, but I think the code was more readable before this change, since begin_last and end are polluting the outer scope now. Compilers are probably good with optimizing these in-loop definitions too.
|
Well, I'm not big on trusting compilers so I dropped some benchmarking code in. Basically it adds up the duration of the loop internals minus the sleep() for 10k iterations (attached). I did 10 runs with the status quo vs 10 runs with this PR, ABAB style. Basically there was no difference. However, I still think we should merge the PR because this is one of the most critical bits of code and it feels weird to depend on compiler optimization for this critical bit. |
aprotyas
left a comment
aprotyas
left a comment
There was a problem hiding this comment.
Basically there was no difference. However, I still think we should merge the PR because this is one of the most critical bits of code and it feels weird to depend on compiler optimization for this critical bit.
Hmm, I think there won't be any maintainability concerns either from moving said variables to the outer scope given how small this file is (and probably will remain). I don't see a strong reason to oppose this then. LGTM
|
closing in favor of #646 |
3 participants
This is probably a negligible performance improvement but I guess it doesn't hurt.