From 91d2f8cd205fb6560dc4e9a10435d40c15190817 Mon Sep 17 00:00:00 2001 From: Joey Kleingers Date: Wed, 21 Oct 2026 16:00:31 -0400 Subject: [PATCH] BUG: Read the shared face-count map without inserting in Compute Triangle Face Ids The parallel loop that fills the feature face labels and triangle counts called std::map::operator[] on a map that every worker shares. Row 0 is the reserved (0,0) boundary. When no triangle uses that pair, the row 0 lookup inserted key 0 and could rebalance the tree while other workers searched it. That is a data race. The workers now take the map by const reference and look counts up with find(). A missing entry gives a count of 0, the same value the insert produced, so the output does not change. Two tests cover the change. The first builds the map from keys 2 and 1, so that inserting key 0 would rotate the tree, and runs the filter 300 times. Under ThreadSanitizer the old code reported a write to the map from inside the parallel loop in every run; the fixed code reported none. The second uses a real (0,0) pair and checks that the reserved row keeps its count. Fixes #1772 Signed-off-by: Joey Kleingers --- .../Filters/Algorithms/SharedFeatureFace.cpp | 11 +- .../test/SharedFeatureFaceTest.cpp | 105 ++++++++++++++++++ 2 files changed, 112 insertions(+), 4 deletions(-) diff --git a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/SharedFeatureFace.cpp b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/SharedFeatureFace.cpp index 666a0ae7b6..cc4b4d9a21 100644 --- a/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/SharedFeatureFace.cpp +++ b/src/Plugins/SimplnxCore/src/SimplnxCore/Filters/Algorithms/SharedFeatureFace.cpp @@ -24,7 +24,7 @@ class SharedFeatureFaceImpl { public: SharedFeatureFaceImpl(const std::vector>& faceLabelVector, Int32AbstractDataStore& surfaceMeshFeatureFaceLabels, Int32AbstractDataStore& surfaceMeshFeatureFaceNumTriangles, - std::map& faceSizeMap, const std::atomic_bool& shouldCancel) + const std::map& faceSizeMap, const std::atomic_bool& shouldCancel) : m_FaceLabelVector(faceLabelVector) , m_SurfaceMeshFeatureFaceLabels(surfaceMeshFeatureFaceLabels) , m_SurfaceMeshFeatureFaceNumTriangles(surfaceMeshFeatureFaceNumTriangles) @@ -44,8 +44,11 @@ class SharedFeatureFaceImpl m_SurfaceMeshFeatureFaceLabels[2 * i + 1] = faceLabelMapEntry.second; // get feature triangle count - uint64 faceId64 = ConvertToUInt64(faceLabelMapEntry.first, faceLabelMapEntry.second); - m_SurfaceMeshFeatureFaceNumTriangles[i] = m_FaceSizeMap[faceId64]; + // Worker threads share the map, so only read it. Row 0 is the reserved (0,0) boundary, which has no + // entry unless a triangle uses that pair; a missing entry has no triangles. + const uint64 faceId64 = ConvertToUInt64(faceLabelMapEntry.first, faceLabelMapEntry.second); + const auto faceSizeIter = m_FaceSizeMap.find(faceId64); + m_SurfaceMeshFeatureFaceNumTriangles[i] = faceSizeIter == m_FaceSizeMap.end() ? 0 : faceSizeIter->second; if(m_ShouldCancel) { break; @@ -57,7 +60,7 @@ class SharedFeatureFaceImpl const std::vector>& m_FaceLabelVector; Int32AbstractDataStore& m_SurfaceMeshFeatureFaceLabels; Int32AbstractDataStore& m_SurfaceMeshFeatureFaceNumTriangles; - std::map& m_FaceSizeMap; + const std::map& m_FaceSizeMap; const std::atomic_bool& m_ShouldCancel; }; diff --git a/src/Plugins/SimplnxCore/test/SharedFeatureFaceTest.cpp b/src/Plugins/SimplnxCore/test/SharedFeatureFaceTest.cpp index 30c1499d65..ac129d4316 100644 --- a/src/Plugins/SimplnxCore/test/SharedFeatureFaceTest.cpp +++ b/src/Plugins/SimplnxCore/test/SharedFeatureFaceTest.cpp @@ -6,7 +6,12 @@ #include "simplnx/Parameters/GeometrySelectionParameter.hpp" #include "simplnx/UnitTest/UnitTestCommon.hpp" +#include "simplnx/DataStructure/AttributeMatrix.hpp" +#include "simplnx/DataStructure/Geometry/TriangleGeom.hpp" +#include #include +#include +#include using namespace nx::core; using namespace nx::core::UnitTest; @@ -28,6 +33,78 @@ const DataPath k_GeometryPath = DataPath({k_TriangleGeometryName}); const DataPath k_FeatureAttributeMatrixPath = k_GeometryPath.createChildPath(k_FaceFeatureName); const DataPath k_FaceLabelsPath = k_GeometryPath.createChildPath(k_FaceAttributeMatrixName).createChildPath(k_FaceLabelsName); +/** + * @brief Builds one disjoint triangle per label pair, runs the filter without randomization, and checks every output. + * @param pairs Face label pair for each triangle. + * @param expectedIds Expected shared feature face ID for each triangle. + * @param expectedLabels Expected label pair for each boundary row, including the reserved row 0. + * @param expectedCounts Expected triangle count for each boundary row, including the reserved row 0. + */ +void ExecuteAndCheckLabelPairs(const std::vector>& pairs, const std::vector& expectedIds, const std::vector>& expectedLabels, + const std::vector& expectedCounts) +{ + DataStructure ds; + auto* meshGeomPtr = TriangleGeom::Create(ds, "Mesh"); + REQUIRE(meshGeomPtr != nullptr); + auto* verticesPtr = UnitTest::CreateTestDataArray(ds, "Vertices", {pairs.size() * 3}, {3}, meshGeomPtr->getId()); + auto* trianglesPtr = UnitTest::CreateTestDataArray(ds, "Triangles", {pairs.size()}, {3}, meshGeomPtr->getId()); + auto* faceDataPtr = AttributeMatrix::Create(ds, "Faces", ShapeType{pairs.size()}, meshGeomPtr->getId()); + REQUIRE(verticesPtr != nullptr); + REQUIRE(trianglesPtr != nullptr); + REQUIRE(faceDataPtr != nullptr); + meshGeomPtr->setVertices(*verticesPtr); + meshGeomPtr->setFaceList(*trianglesPtr); + meshGeomPtr->setFaceDataId(faceDataPtr->getId()); + auto* labelsPtr = UnitTest::CreateTestDataArray(ds, "Labels", {pairs.size()}, {2}, faceDataPtr->getId()); + REQUIRE(labelsPtr != nullptr); + for(usize tupleIdx = 0; tupleIdx < pairs.size(); ++tupleIdx) + { + const auto x = static_cast(2 * tupleIdx); + const std::array xyz{x, 0, 0, x + 1, 0, 0, x, 1, 0}; + for(usize compIdx = 0; compIdx < xyz.size(); ++compIdx) + { + (*verticesPtr)[9 * tupleIdx + compIdx] = xyz[compIdx]; + } + for(usize compIdx = 0; compIdx < 3; ++compIdx) + { + (*trianglesPtr)[3 * tupleIdx + compIdx] = 3 * tupleIdx + compIdx; + } + (*labelsPtr)[2 * tupleIdx] = pairs[tupleIdx].first; + (*labelsPtr)[2 * tupleIdx + 1] = pairs[tupleIdx].second; + } + SharedFeatureFaceFilter filter; + auto args = filter.getDefaultArguments(); + args.insertOrAssign(SharedFeatureFaceFilter::k_TriGeometryDataPath_Key, std::make_any(DataPath({"Mesh"}))); + args.insertOrAssign(SharedFeatureFaceFilter::k_FaceLabelsArrayPath_Key, std::make_any(DataPath({"Mesh", "Faces", "Labels"}))); + args.insertOrAssign(SharedFeatureFaceFilter::k_RandomizeFeatures_Key, std::make_any(false)); + args.insertOrAssign(SharedFeatureFaceFilter::k_FeatureFaceIdsArrayName_Key, std::make_any("Ids")); + args.insertOrAssign(SharedFeatureFaceFilter::k_GrainBoundaryAttributeMatrixName_Key, std::make_any("Boundaries")); + args.insertOrAssign(SharedFeatureFaceFilter::k_FeatureFaceLabelsArrayName_Key, std::make_any("Labels")); + args.insertOrAssign(SharedFeatureFaceFilter::k_FeatureNumTrianglesArrayName_Key, std::make_any("Counts")); + auto result = filter.execute(ds, args); + SIMPLNX_RESULT_REQUIRE_VALID(result.result); + REQUIRE_NOTHROW(ds.getDataRefAs(DataPath({"Mesh", "Faces", "Ids"}))); + const auto& idsRef = ds.getDataRefAs(DataPath({"Mesh", "Faces", "Ids"})); + REQUIRE_NOTHROW(ds.getDataRefAs(DataPath({"Mesh", "Boundaries", "Labels"}))); + const auto& outLabelsRef = ds.getDataRefAs(DataPath({"Mesh", "Boundaries", "Labels"})); + REQUIRE_NOTHROW(ds.getDataRefAs(DataPath({"Mesh", "Boundaries", "Counts"}))); + const auto& countsRef = ds.getDataRefAs(DataPath({"Mesh", "Boundaries", "Counts"})); + REQUIRE(idsRef.getNumberOfTuples() == expectedIds.size()); + REQUIRE(outLabelsRef.getNumberOfTuples() == expectedLabels.size()); + REQUIRE(countsRef.getNumberOfTuples() == expectedCounts.size()); + for(usize tupleIdx = 0; tupleIdx < expectedIds.size(); ++tupleIdx) + { + CHECK(idsRef[tupleIdx] == expectedIds[tupleIdx]); + } + for(usize row = 0; row < expectedLabels.size(); ++row) + { + CHECK(outLabelsRef[2 * row] == expectedLabels[row].first); + CHECK(outLabelsRef[2 * row + 1] == expectedLabels[row].second); + CHECK(countsRef[row] == expectedCounts[row]); + } + UnitTest::CheckArraysInheritTupleDims(ds); +} + } // namespace SharedFeatureFaceFilterTest using namespace SharedFeatureFaceFilterTest; @@ -103,3 +180,31 @@ TEST_CASE("SimplnxCore::SharedFeatureFaceFilter", "[SimplnxCore][SharedFeatureFa UnitTest::CheckArraysInheritTupleDims(dataStructure); } + +TEST_CASE("SimplnxCore::SharedFeatureFace: Missing zero pair rotation race", "[SimplnxCore][SharedFeatureFaceFilter][Regression]") +{ + UnitTest::LoadPlugins(); + // The first distinct pairs create map keys 2 and 1. Key zero is absent, so the reserved row must receive + // count zero without changing the shared map. + const std::vector> pairs{{0, 2}, {0, 2}, {0, 1}}; + const std::vector expectedIds{1, 1, 2}; + const std::vector> expectedLabels{{0, 0}, {0, 2}, {0, 1}}; + const std::vector expectedCounts{0, 2, 1}; + for(usize repetition = 0; repetition < 300; ++repetition) + { + CAPTURE(repetition); + ExecuteAndCheckLabelPairs(pairs, expectedIds, expectedLabels, expectedCounts); + } +} + +TEST_CASE("SimplnxCore::SharedFeatureFace: Real zero pair counts", "[SimplnxCore][SharedFeatureFaceFilter]") +{ + UnitTest::LoadPlugins(); + // Pair (0,0) creates map key zero, so the reserved row and boundary row 1 share that key and both report + // its two triangles. + const std::vector> pairs{{0, 0}, {0, 2}, {0, 0}}; + const std::vector expectedIds{1, 2, 1}; + const std::vector> expectedLabels{{0, 0}, {0, 0}, {0, 2}}; + const std::vector expectedCounts{2, 2, 1}; + ExecuteAndCheckLabelPairs(pairs, expectedIds, expectedLabels, expectedCounts); +}