perf: Optimized out shared_ptr copies#3079
Conversation
|
Pulls: #3079
|
wjwwood
left a comment
wjwwood
left a comment
There was a problem hiding this comment.
In general a big 👍 from me.
I did have a question about one class of change, but I think it probably has a reason.
The whole "not using const ref to shared ptr" is due to Dirk not wanting to use them due to some class of dangling ownership bug he ran into once, but I always thought it was fine and then later the cpp core guidelines clarified it and we started doing it the "right" way in new code, but we never cleaned up the existing interfaces so thanks for doing that.
rclcpp/src/rclcpp/callback_group.cpp
Outdated
| can_be_taken_from_(true), | ||
| automatically_add_to_executor_with_node_(automatically_add_to_executor_with_node), | ||
| context_(context) | ||
| context_(std::move(context)) |
There was a problem hiding this comment.
Can you explain this one? Seems like it's not needed as context is a temporary but could be confusing if it is referenced in the constructor body instead of context_.
There was a problem hiding this comment.
There are two way of approaching the pass by value & pass by ref issue. And to make matters worse, the 'better' approach is dependent on if its a header only method or in a separate compile unit.
It is faster to pass an object as an value and and assign it to a member using std::move, if this happens in a header / same compile unit. If you play around in compiler explorer you can see that the generated code for this version is shorter, Microbenchmarks also show this.
If you already have a value as it is dictated by the interface etc, using std::move during assignment to a member variable is preferred and faster.
If you have a call chain, of objects passing on an argument, pass by reference always wins.
If you wanted to do this really clean etc you would need to generate methods and constructors for move semantics, but this is a looooot of work...
| : pre_callback(pre_callback), | ||
| post_callback(post_callback), | ||
| : pre_callback(std::move(pre_callback)), | ||
| post_callback(std::move(post_callback)), |
There was a problem hiding this comment.
Similar question here. Is there a c++ core guideline that covers this case or something?
There was a problem hiding this comment.
In this special case
JumpHandler::JumpHandler(
pre_callback_t &&pre_callback,
post_callback_t &&post_callback,
const rcl_jump_threshold_t & threshold)
would be the clean solution. See F.18 of the core guideline for reference.
See also https://clang.llvm.org/extra/clang-tidy/checks/modernize/pass-by-value.html
Or https://www.youtube.com/watch?v=56DMwqKffi0
|
|
||
| if (enable_param.get<bool>()) { | ||
| auto * rcl_node = node_base->get_rcl_node_handle(); | ||
| auto * rcl_node = node_base_->get_rcl_node_handle(); |
There was a problem hiding this comment.
Last comment about this class of change, but this is a good example of a change that is required due to the new moves in the constructor initializer statements which I don't think is necessarily a good (or bad) thing. It just seems unnecessary.
There was a problem hiding this comment.
This case is clearly faster, as the compiler can see through it and perform optimizations. I must say, that in general I am not a big fan of move semantics, as compilers (at least on ubuntu 22.04) are not warning you if you use an object that was moved away and therefore it is easy to introduce errors.
fujitatomoya
left a comment
fujitatomoya
left a comment
There was a problem hiding this comment.
the approach looks good to avoid extra copies. DCO is missing though.
jmachowinski
force-pushed
the
shared_ptr_optimization
branch
2 times, most recently
from
eda27e5 to
bf274bf
Compare
|
Pulls: #3079
|
jmachowinski
force-pushed
the
shared_ptr_optimization
branch
from
bf274bf to
571f6bc
Compare
|
Pulls: #3079
|
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
jmachowinski
force-pushed
the
shared_ptr_optimization
branch
from
571f6bc to
9eecbd7
Compare
|
Pulls: #3079
|
3 participants
Description
This is a PR that aims to eliminate overhead by unneeded copies of shared ptrs all over the code.
ATM this is a draft as it might touch external interfaces and beak user code down the line.
Fixes # (issue)
Is this user-facing behavior change?
Yes, as in theory it is an API change, but in most cases the end user will not notice it.
Did you use Generative AI?
I wish I had...