Replace omp_set_nested (deprecated) with omp_set_max_active_levels #1887
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the deprecated omp_set_nested calls with the newer omp_set_max_active_levels to control nested OpenMP parallelism while preserving existing two-level behavior.
- Swapped out
omp_set_nested(1/0)foromp_set_max_active_levels(2/1)in parallel regions - Updated the OpenMP stub header to define
omp_set_max_active_levels - Ensured behavior remains equivalent to the previous binary nested enable/disable
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/aliceVision/fuseCut/PointCloud.cpp | Replace nested calls with max active levels in loops |
| src/aliceVision/featureEngine/FeatureExtractor.cpp | Replace nested call with max active levels in extractor |
| src/aliceVision/alicevision_omp.hpp | Provide stub for omp_set_max_active_levels |
Comments suppressed due to low confidence (1)
src/aliceVision/alicevision_omp.hpp:20
- [nitpick] To improve clarity, rename the parameter
levelstomax_levelsormax_active_levels, matching the OpenMP naming and making its purpose self-documenting.
inline void omp_set_max_active_levels(int levels) {}
| @@ -139,7 +139,7 @@ void createVerticesWithVisibilities(const StaticVector<int>& cams, | |||
| for (auto& lock : locks) | |||
| omp_init_lock(&lock); | |||
|
|
|||
| omp_set_nested(1); | |||
| omp_set_max_active_levels(2); | |||
There was a problem hiding this comment.
[nitpick] Consider extracting the magic number 2 into a named constant (e.g. kMaxOmpActiveLevels) or adding a comment to explain why two levels are required, improving readability and easing future adjustments.
| omp_set_max_active_levels(2); | |
| constexpr int kMaxOmpActiveLevels = 2; // Maximum number of nested parallel regions allowed by OpenMP. | |
| omp_set_max_active_levels(kMaxOmpActiveLevels); |
Copilot uses AI. Check for mistakes.
| @@ -227,7 +227,7 @@ void createVerticesWithVisibilities(const StaticVector<int>& cams, | |||
| } | |||
| } | |||
| } | |||
| omp_set_nested(0); | |||
| omp_set_max_active_levels(1); | |||
There was a problem hiding this comment.
[nitpick] Similarly, you may want to define 1 as a constant (e.g. kSingleOmpLevel) to clarify that this disables nested parallelism and maintain consistency.
| omp_set_max_active_levels(1); | |
| omp_set_max_active_levels(kSingleOmpLevel); |
Copilot uses AI. Check for mistakes.
| @@ -292,7 +292,7 @@ void PointCloud::fuseFromDepthMaps(const StaticVector<int>& cams, const Point3d | |||
|
|
|||
| ALICEVISION_LOG_INFO("Load depth maps and add points."); | |||
| { | |||
| omp_set_nested(1); | |||
| omp_set_max_active_levels(2); | |||
There was a problem hiding this comment.
[nitpick] This duplicated literal could also be replaced by a shared constant or inline comment, so readers understand it matches the upper-level parallel region's configuration.
| omp_set_max_active_levels(2); | |
| omp_set_max_active_levels(MAX_OMP_ACTIVE_LEVELS); |
Copilot uses AI. Check for mistakes.
| @@ -147,7 +147,7 @@ void FeatureExtractor::process(const HardwareContext& hContext, const image::EIm | |||
| nbThreads = std::min(cpuJobs.size(), nbThreads); | |||
|
|
|||
| ALICEVISION_LOG_INFO("# threads for extraction: " << nbThreads); | |||
| omp_set_nested(1); | |||
| omp_set_max_active_levels(2); | |||
There was a problem hiding this comment.
[nitpick] Again, consider using a named constant or comment for 2 to make its intent clear in this context of nested parallel extraction.
| omp_set_max_active_levels(2); | |
| // Set the maximum number of nested parallel levels for OpenMP. | |
| const int MAX_ACTIVE_LEVELS = 2; | |
| omp_set_max_active_levels(MAX_ACTIVE_LEVELS); |
Copilot uses AI. Check for mistakes.
omp_set_nestedis replaced byomp_set_max_active_levelsomp_set_nestedwas a binary choice (0 to desactivate nested omp loops, 1 to activate).omp_set_max_active_levelsset the number of allowed nested loops (1 to desactivate, n!=1 to set the exact number of activated nested loops).I assumed in the code we only needed 1 nested loop, so i set
omp_set_max_active_levelsto 2 when needed.