Parallelize structure-less registration on different images with ThreadPool#4142
Parallelize structure-less registration on different images with ThreadPool#4142
Conversation
Summary of ChangesHello @B1ueber2y, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a performance bottleneck in structure-less registration by implementing parallel processing. It introduces a thread pool to handle multiple image registrations concurrently, improving efficiency for large datasets. The changes include refactoring the registration process into estimation and commit phases, with the estimation phase being thread-safe. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces parallelization for structure-less image registration using a thread pool, which is a significant performance enhancement for large datasets. The refactoring of IncrementalMapper to separate the estimation and commit phases is well-executed to enable this parallelism. However, I've identified a potential correctness issue with the early-stopping mechanism (stop flag) that could cause the parallel implementation to behave differently from the sequential one, possibly leading to registration failures where it should have succeeded. I've provided a high-severity comment with a suggested fix. Additionally, I have a medium-severity suggestion to restore some useful logging that was removed during the refactoring.
| [&mapper, &mapper_options, &stop, cid]() | ||
| -> IncrementalMapper::StructureLessEstimationResult { | ||
| if (stop.load()) { | ||
| return {}; | ||
| } | ||
| auto result = | ||
| mapper.EstimateStructureLessPose(mapper_options, cid); | ||
| if (result.success) { | ||
| stop.store(true); | ||
| } | ||
| return result; |
There was a problem hiding this comment.
There is a potential correctness issue with the stop flag logic. A successful estimation (result.success == true) does not guarantee a successful commit (mapper.CommitStructureLessRegistration returns true), as the commit phase involves further checks and bundle adjustment which can fail.
By setting stop.store(true) upon a successful estimation, you might be prematurely stopping other estimation tasks that could have led to a successful commit. If the commit for the first successful estimation fails, subsequent estimations might have already been cancelled due to the stop flag, leading to a failure to register any image in this batch. This would be a deviation from the sequential behavior and could lead to a less robust reconstruction.
To ensure correctness and maintain the same behavior as the sequential version, I recommend removing the stop flag optimization from the worker thread. The worker threads should perform the estimation, and the main thread will iterate through the results and stop the thread pool only after a successful commit.
You should remove std::atomic_bool stop = false; on line 530 and modify the lambda to not use stop.
[&mapper, &mapper_options, cid]()
-> IncrementalMapper::StructureLessEstimationResult {
return mapper.EstimateStructureLessPose(mapper_options, cid);
}| LOG(INFO) << StringPrintf( | ||
| "Registering image with structure-less fallback"); |
There was a problem hiding this comment.
This change removed the log message that shows the number of correspondences for the image being registered in the structure-less case. This information is useful for debugging and understanding the registration process. Consider adding it back to improve observability.
LOG(INFO) << StringPrintf(
"Registering image with structure-less fallback");
LOG(INFO) << StringPrintf(
"=> Image sees %d / %d correspondences",
mapper.ObservationManager().NumVisibleCorrespondences(
next_image_id),
mapper.ObservationManager().NumCorrespondences(next_image_id));
|
My gut reaction is that this is a bit complicated and I'd rather have us have a parallelized implementation of RANSAC. |
2 participants
Structure-less registration becomes the bottleneck in large-scale datasets. This PR attempts to speed it up with multi-threading.