From a932fdfe56b09c65c5106649aa5ab8c75da50e29 Mon Sep 17 00:00:00 2001 From: yurekami Date: Wed, 2 Sep 2026 01:03:57 +0800 Subject: [PATCH 1/2] Balance ISS keypoint OpenMP work with dynamic scheduling Issue #5785 asks for contained checks of OpenMP loops whose per-iteration cost varies with neighbor searches. ISSKeypoint3D still used the default schedule in several such loops, so this change switches those loops to dynamic scheduling and adds a 2-thread regression on the existing ISS boundary-estimation path. Constraint: Keep the #5785 work scoped to one candidate implementation file instead of a repo-wide schedule sweep Rejected: Touching already-dynamic OMP implementations such as normal_3d_omp and shot_omp | no remaining issue value there Confidence: medium Scope-risk: narrow Directive: Revisit the fixed dynamic chunk only with benchmark data; this patch intentionally avoids adding new public tuning knobs Tested: CMake configure under Visual Studio 2022 Build Tools reached OpenMP detection in build-iss Not-tested: keypoints_iss_3d build/run blocked at configure time by missing Eigen3 package config on this machine Signed-off-by: yurekami --- .../include/pcl/keypoints/impl/iss_3d.hpp | 12 +++-- test/keypoints/test_iss_3d.cpp | 45 +++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/keypoints/include/pcl/keypoints/impl/iss_3d.hpp b/keypoints/include/pcl/keypoints/impl/iss_3d.hpp index a14a1bf966f..c6312b5d5f8 100644 --- a/keypoints/include/pcl/keypoints/impl/iss_3d.hpp +++ b/keypoints/include/pcl/keypoints/impl/iss_3d.hpp @@ -131,7 +131,8 @@ pcl::ISSKeypoint3D::getBoundaryPoints (PointCloudI default(none) \ shared(angle_threshold, boundary_estimator, border_radius, edge_points, input) \ firstprivate(u, v) \ - num_threads(threads_) + num_threads(threads_) \ + schedule(dynamic, 64) for (int index = 0; index < static_cast(input.size ()); index++) { edge_points[index] = false; @@ -313,7 +314,8 @@ pcl::ISSKeypoint3D::detectKeypoints (PointCloudOut #pragma omp parallel for \ default(none) \ shared(borders) \ - num_threads(threads_) + num_threads(threads_) \ + schedule(dynamic, 64) for (int index = 0; index < static_cast(input_->size ()); index++) { borders[index] = false; @@ -357,7 +359,8 @@ pcl::ISSKeypoint3D::detectKeypoints (PointCloudOut #pragma omp parallel for \ default(none) \ shared(borders, omp_mem, prg_mem) \ - num_threads(threads_) + num_threads(threads_) \ + schedule(dynamic, 64) for (int index = 0; index < static_cast (input_->size ()); index++) { #ifdef _OPENMP @@ -412,7 +415,8 @@ pcl::ISSKeypoint3D::detectKeypoints (PointCloudOut #pragma omp parallel for \ default(none) \ shared(feat_max) \ - num_threads(threads_) + num_threads(threads_) \ + schedule(dynamic, 64) for (int index = 0; index < static_cast(input_->size ()); index++) { feat_max [index] = false; diff --git a/test/keypoints/test_iss_3d.cpp b/test/keypoints/test_iss_3d.cpp index 901074ea01d..742faff27b7 100644 --- a/test/keypoints/test_iss_3d.cpp +++ b/test/keypoints/test_iss_3d.cpp @@ -154,6 +154,51 @@ TEST (PCL, ISSKeypoint3D_BE) tree.reset (new search::KdTree ()); } +////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +TEST (PCL, ISSKeypoint3D_BE_parallel) +{ + PointCloud keypoints; + + ISSKeypoint3D iss_detector; + + iss_detector.setSearchMethod (tree); + iss_detector.setSalientRadius (6 * cloud_resolution); + iss_detector.setNonMaxRadius (4 * cloud_resolution); + + iss_detector.setNormalRadius (4 * cloud_resolution); + iss_detector.setBorderRadius (4 * cloud_resolution); + + iss_detector.setThreshold21 (0.975); + iss_detector.setThreshold32 (0.975); + iss_detector.setMinNeighbors (5); + iss_detector.setAngleThreshold (static_cast (M_PI) / 3.0); + iss_detector.setNumberOfThreads (2); + + iss_detector.setInputCloud (cloud); + iss_detector.compute (keypoints); + + constexpr std::size_t correct_nr_keypoints = 5; + const float correct_keypoints[correct_nr_keypoints][3] = + { + {-0.052037f, 0.116800f, 0.034582f}, + { 0.027420f, 0.096386f, 0.043312f}, + {-0.011943f, 0.086771f, 0.057009f}, + {-0.070344f, 0.087352f, 0.041908f}, + {-0.030035f, 0.066130f, 0.038942f} + }; + + ASSERT_EQ (keypoints.size (), correct_nr_keypoints); + + for (std::size_t i = 0; i < correct_nr_keypoints; ++i) + { + EXPECT_NEAR (keypoints[i].x, correct_keypoints[i][0], 1e-6); + EXPECT_NEAR (keypoints[i].y, correct_keypoints[i][1], 1e-6); + EXPECT_NEAR (keypoints[i].z, correct_keypoints[i][2], 1e-6); + } + + tree.reset (new search::KdTree ()); +} + //* ---[ */ int main (int argc, char** argv) From 251e3d3aa1c82a1e69bd6e6cd1580477e6845a1b Mon Sep 17 00:00:00 2001 From: yurekami Date: Wed, 2 Sep 2026 09:48:01 +0800 Subject: [PATCH 2/2] Stabilize the ISS regression for dynamic OpenMP scheduling The dynamic-scheduling change for issue #5785 shifts iteration ordering, so the regression should validate the keypoint set rather than a thread-dependent output order. This follow-up canonicalizes the expected keypoints and repeats the 2-thread path enough times to catch flaky ordering-sensitive failures. Constraint: Keep the existing issue #5785 implementation focused on ISS instead of broadening into more scheduler tuning work Rejected: Restore the previous order-sensitive assertions | they can fail even when the dynamic-scheduling fix is correct Confidence: medium Scope-risk: narrow Directive: Parallel ISS tests should compare canonicalized keypoint sets unless the API explicitly guarantees output ordering Tested: git diff --check Not-tested: Native build/run of test_keypoints_iss_3d; local machine is still missing Eigen3 package config for the PCL build Signed-off-by: yurekami --- test/keypoints/test_iss_3d.cpp | 145 ++++++++++++++++----------------- 1 file changed, 69 insertions(+), 76 deletions(-) diff --git a/test/keypoints/test_iss_3d.cpp b/test/keypoints/test_iss_3d.cpp index 742faff27b7..1be8551afcf 100644 --- a/test/keypoints/test_iss_3d.cpp +++ b/test/keypoints/test_iss_3d.cpp @@ -41,6 +41,10 @@ #include #include +#include +#include +#include + using namespace pcl; using namespace pcl::io; @@ -51,6 +55,60 @@ double cloud_resolution (0.0058329); PointCloud::Ptr cloud (new PointCloud ()); search::KdTree::Ptr tree (new search::KdTree ()); +using KeypointCoordinates = std::array; + +std::vector +canonicalizeKeypoints (const PointCloud& keypoints) +{ + std::vector coordinates; + coordinates.reserve (keypoints.size ()); + + for (const auto& keypoint : keypoints) + coordinates.push_back ({keypoint.x, keypoint.y, keypoint.z}); + + std::sort (coordinates.begin (), coordinates.end ()); + return (coordinates); +} + +void +expectKeypointsMatch (const PointCloud& keypoints, + const std::vector& expected_keypoints) +{ + const auto actual_keypoints = canonicalizeKeypoints (keypoints); + + ASSERT_EQ (actual_keypoints.size (), expected_keypoints.size ()); + + for (std::size_t i = 0; i < expected_keypoints.size (); ++i) + { + EXPECT_NEAR (actual_keypoints[i][0], expected_keypoints[i][0], 1e-6); + EXPECT_NEAR (actual_keypoints[i][1], expected_keypoints[i][1], 1e-6); + EXPECT_NEAR (actual_keypoints[i][2], expected_keypoints[i][2], 1e-6); + } +} + +PointCloud +computeBoundaryEstimatedKeypoints (unsigned int threads) +{ + ISSKeypoint3D iss_detector; + PointCloud keypoints; + + iss_detector.setSearchMethod (tree); + iss_detector.setSalientRadius (6 * cloud_resolution); + iss_detector.setNonMaxRadius (4 * cloud_resolution); + + iss_detector.setNormalRadius (4 * cloud_resolution); + iss_detector.setBorderRadius (4 * cloud_resolution); + + iss_detector.setThreshold21 (0.975); + iss_detector.setThreshold32 (0.975); + iss_detector.setMinNeighbors (5); + iss_detector.setAngleThreshold (static_cast (M_PI) / 3.0); + iss_detector.setNumberOfThreads (threads); + + iss_detector.setInputCloud (cloud); + iss_detector.compute (keypoints); + return (keypoints); +} ////////////////////////////////////////////////////////////////////////////////////////////////////////////////// TEST (PCL, ISSKeypoint3D_WBE) @@ -75,10 +133,8 @@ TEST (PCL, ISSKeypoint3D_WBE) // // Compare to previously validated output // - constexpr std::size_t correct_nr_keypoints = 6; - const float correct_keypoints[correct_nr_keypoints][3] = + const std::vector correct_keypoints = { - // { x, y, z} {-0.071112f, 0.137670f, 0.047518f}, {-0.041733f, 0.127960f, 0.016650f}, {-0.011943f, 0.086771f, 0.057009f}, @@ -87,15 +143,7 @@ TEST (PCL, ISSKeypoint3D_WBE) {-0.048250f, 0.167480f, -0.000152f} }; - - ASSERT_EQ (keypoints.size (), correct_nr_keypoints); - - for (std::size_t i = 0; i < correct_nr_keypoints; ++i) - { - EXPECT_NEAR (keypoints[i].x, correct_keypoints[i][0], 1e-6); - EXPECT_NEAR (keypoints[i].y, correct_keypoints[i][1], 1e-6); - EXPECT_NEAR (keypoints[i].z, correct_keypoints[i][2], 1e-6); - } + expectKeypointsMatch (keypoints, correct_keypoints); tree.reset (new search::KdTree ()); } @@ -103,38 +151,11 @@ TEST (PCL, ISSKeypoint3D_WBE) ////////////////////////////////////////////////////////////////////////////////////////////////////////////////// TEST (PCL, ISSKeypoint3D_BE) { - PointCloud keypoints; - - // - // Compute the ISS 3D keypoints - By first performing the Boundary Estimation - // - - ISSKeypoint3D iss_detector; - - iss_detector.setSearchMethod (tree); - iss_detector.setSalientRadius (6 * cloud_resolution); - iss_detector.setNonMaxRadius (4 * cloud_resolution); - - iss_detector.setNormalRadius (4 * cloud_resolution); - iss_detector.setBorderRadius (4 * cloud_resolution); - - iss_detector.setThreshold21 (0.975); - iss_detector.setThreshold32 (0.975); - iss_detector.setMinNeighbors (5); - iss_detector.setAngleThreshold (static_cast (M_PI) / 3.0); - iss_detector.setNumberOfThreads (1); - - iss_detector.setInputCloud (cloud); - iss_detector.compute (keypoints); - - // // Compare to previously validated output // - constexpr std::size_t correct_nr_keypoints = 5; - const float correct_keypoints[correct_nr_keypoints][3] = + const std::vector correct_keypoints = { - // { x, y, z} {-0.052037f, 0.116800f, 0.034582f}, { 0.027420f, 0.096386f, 0.043312f}, {-0.011943f, 0.086771f, 0.057009f}, @@ -142,14 +163,8 @@ TEST (PCL, ISSKeypoint3D_BE) {-0.030035f, 0.066130f, 0.038942f} }; - ASSERT_EQ (keypoints.size (), correct_nr_keypoints); - - for (std::size_t i = 0; i < correct_nr_keypoints; ++i) - { - EXPECT_NEAR (keypoints[i].x, correct_keypoints[i][0], 1e-6); - EXPECT_NEAR (keypoints[i].y, correct_keypoints[i][1], 1e-6); - EXPECT_NEAR (keypoints[i].z, correct_keypoints[i][2], 1e-6); - } + const auto keypoints = computeBoundaryEstimatedKeypoints (1); + expectKeypointsMatch (keypoints, correct_keypoints); tree.reset (new search::KdTree ()); } @@ -157,28 +172,7 @@ TEST (PCL, ISSKeypoint3D_BE) ////////////////////////////////////////////////////////////////////////////////////////////////////////////////// TEST (PCL, ISSKeypoint3D_BE_parallel) { - PointCloud keypoints; - - ISSKeypoint3D iss_detector; - - iss_detector.setSearchMethod (tree); - iss_detector.setSalientRadius (6 * cloud_resolution); - iss_detector.setNonMaxRadius (4 * cloud_resolution); - - iss_detector.setNormalRadius (4 * cloud_resolution); - iss_detector.setBorderRadius (4 * cloud_resolution); - - iss_detector.setThreshold21 (0.975); - iss_detector.setThreshold32 (0.975); - iss_detector.setMinNeighbors (5); - iss_detector.setAngleThreshold (static_cast (M_PI) / 3.0); - iss_detector.setNumberOfThreads (2); - - iss_detector.setInputCloud (cloud); - iss_detector.compute (keypoints); - - constexpr std::size_t correct_nr_keypoints = 5; - const float correct_keypoints[correct_nr_keypoints][3] = + const std::vector correct_keypoints = { {-0.052037f, 0.116800f, 0.034582f}, { 0.027420f, 0.096386f, 0.043312f}, @@ -187,13 +181,12 @@ TEST (PCL, ISSKeypoint3D_BE_parallel) {-0.030035f, 0.066130f, 0.038942f} }; - ASSERT_EQ (keypoints.size (), correct_nr_keypoints); - - for (std::size_t i = 0; i < correct_nr_keypoints; ++i) + constexpr int repeated_runs = 12; + for (int run = 0; run < repeated_runs; ++run) { - EXPECT_NEAR (keypoints[i].x, correct_keypoints[i][0], 1e-6); - EXPECT_NEAR (keypoints[i].y, correct_keypoints[i][1], 1e-6); - EXPECT_NEAR (keypoints[i].z, correct_keypoints[i][2], 1e-6); + SCOPED_TRACE (::testing::Message () << "parallel run " << run); + const auto keypoints = computeBoundaryEstimatedKeypoints (2); + expectKeypointsMatch (keypoints, correct_keypoints); } tree.reset (new search::KdTree ());