Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add to IncrementalMapper reusage of Ceres internal context #1593

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

S-o-T
Copy link
Contributor

@S-o-T S-o-T commented Aug 9, 2022

Avoid costs for Ceres internal threadpool initialization at each bundle adjustment instance solve, by reusing Ceres internal context.

@S-o-T S-o-T force-pushed the add_ceres_context_reusage branch from 6115b28 to dc92cd1 Compare August 9, 2022 20:49
@S-o-T S-o-T marked this pull request as ready for review August 9, 2022 21:54
@ahojnnes
Copy link
Contributor

Seems like this was only introduced in Ceres 2.1.X or later. However, colmap still supports older versions as well. Do you have any profiling results of what speed gains we can expect from this change? Depending on that, we could consider doing something special for newer ceres versions to benefit from this already today.

@S-o-T
Copy link
Contributor Author

S-o-T commented Aug 27, 2022

All the difference essentially comes from time spend on threads creation inside of ceres::internal::ContextImpl::EnsureMinimumThreads, so here are two histograms of its duration (in ns) for reconstruction of size 50 images (181 calls to ceres::internal::ContextImpl::EnsureMinimumThreads total). As can be seen, the distribution is bi-modal, where the heavier mode for the case of context reusage is much smaller.
reused_hist
original_hist

Absolute wall time speedup is a function of the ba problem size, so it is application specific (num features, num cameras, tolerances, linear solver type etc). I would argue, that for most applications with default settings we should not bother with introduction of macro to account for various ceres versions. But if you believe that this kind of speedup can be valuable, i will work on this.

@ahojnnes
Copy link
Contributor

Thanks for the profiling. I tend to agree that the speed gains are marginal compared to the total runtime of a BA. Maybe it looks more favorable if we reuse the context also across pose refinement. However, even there, the max runtime of 1 millisecond is negligible compared to multiple hundreds of milliseconds of total runtime. I'll keep this PR open as a reminder that we can make this improvement whenever support for older ceres versions is dropped (for now, Ubuntu 20.04 ships with ceres 1.14 and is still quite recent and widely used).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants