Reduce smart pointer copies in destructors #1983
Merged
+31
−31
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#1441 introduced a new recursive destructor to avoid stack overflows. We can improve runtime a bit by avoiding copying shared pointers around when we don't need to. I tested this using a robot stationary at 0,0,0 looking at an apriltag 2.5 meters in front of me a total of 50,000 observations, updating ISAM with each observation. This decreases mean runtime by 17% in this particular toy problem (see below).
With this optimization applied and for this -particular- toy problem, I'm still seeing ~half my runtime spent in the BayesTree destructor (see attached perf data from a release build):
perf.zip
IsamBenchmark.cpp (program that generates the output below): https://github.com/mcm001/gtsam/blob/44d36c1e173fe9c9c3752eb0dab376059610272e/examples/IsamBenchmark.cpp
I suspect that there's further opportunities for optimization in these destructors, but I'll leave that to people smarter than I :)
Also fixes some stray ifdef/ifs for boost builds that squeaked in in MR #1948 and #1789 (see 6697452 ). Perhaps this is an indication that we need to add a no-boost build without boost installed in CI?