From 88cfd144afda44aa6d34cdc4de1afd5a2ae0a131 Mon Sep 17 00:00:00 2001 From: Jonathan Stone Date: Fri, 26 Dec 2025 15:59:20 -0800 Subject: [PATCH 1/4] Improvements to thread safety - Fix a race condition in the Document cache, where the `valid` flag could be simultaneously written and read from different threads. - Add an initial Thread Safety section to the Developer Guidelines. --- CONTRIBUTING.md | 8 ++ source/MaterialXCore/Document.cpp | 188 ++++++++++++++++-------------- 2 files changed, 111 insertions(+), 85 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 72e8935923..b8c30d97db 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -242,6 +242,14 @@ be cheap to copy. When returning shared pointers, they should be returned by value rather than by reference. Methods should be marked as `const` whenever they do not modify the object's state. +#### Thread Safety + +MaterialX classes support multiple concurrent readers, but not concurrent +reads and writes, following the pattern of standard C++ containers. This +design enables efficient parallel processing in read-heavy workloads such +as shader generation and scene traversal, while keeping the implementation +simple and avoiding the overhead of fine-grained locking. + #### Exception Handling Exceptions should be used for exceptional conditions rather than for normal diff --git a/source/MaterialXCore/Document.cpp b/source/MaterialXCore/Document.cpp index 3bb8a5b106..76115687e6 100644 --- a/source/MaterialXCore/Document.cpp +++ b/source/MaterialXCore/Document.cpp @@ -5,6 +5,7 @@ #include +#include #include MATERIALX_NAMESPACE_BEGIN @@ -29,82 +30,120 @@ class Document::Cache { public: Cache() : - valid(false) + _valid(false) { } ~Cache() = default; + void setDocument(weak_ptr document) + { + _doc = document; + invalidate(); + } + + void invalidate() + { + _valid.store(false, std::memory_order_relaxed); + } + + vector getMatchingPorts(const string& nodeName) + { + refresh(); + auto it = _portElementMap.find(nodeName); + return (it != _portElementMap.end()) ? it->second : vector(); + } + + vector getMatchingNodeDefs(const string& nodeName) + { + refresh(); + auto it = _nodeDefMap.find(nodeName); + return (it != _nodeDefMap.end()) ? it->second : vector(); + } + + vector getMatchingImplementations(const string& nodeDef) + { + refresh(); + auto it = _implementationMap.find(nodeDef); + return (it != _implementationMap.end()) ? it->second : vector(); + } + + private: void refresh() { - // Thread synchronization for multiple concurrent readers of a single document. - std::lock_guard guard(mutex); + // Perform a lock-free read and return if the cache is valid. + if (_valid.load(std::memory_order_acquire)) + { + return; + } - if (!valid) + // Acquire a lock and double-check the valid flag for robustness. + std::lock_guard lock(_mutex); + if (_valid.load(std::memory_order_relaxed)) { - // Clear the existing cache. - portElementMap.clear(); - nodeDefMap.clear(); - implementationMap.clear(); + return; + } - // Traverse the document to build a new cache. - for (ElementPtr elem : doc.lock()->traverseTree()) - { - const string& nodeName = elem->getAttribute(PortElement::NODE_NAME_ATTRIBUTE); - const string& nodeGraphName = elem->getAttribute(PortElement::NODE_GRAPH_ATTRIBUTE); - const string& nodeString = elem->getAttribute(NodeDef::NODE_ATTRIBUTE); - const string& nodeDefString = elem->getAttribute(InterfaceElement::NODE_DEF_ATTRIBUTE); + // Verify that the document is still valid. + auto doc = _doc.lock(); + if (!doc) + { + return; + } - if (!nodeName.empty()) - { - PortElementPtr portElem = elem->asA(); - if (portElem) - { - portElementMap[portElem->getQualifiedName(nodeName)].push_back(portElem); - } - } - else + // Clear the existing cache. + _portElementMap.clear(); + _nodeDefMap.clear(); + _implementationMap.clear(); + + // Traverse the document to build a new cache. + for (ElementPtr elem : doc->traverseTree()) + { + const string& nodeName = elem->getAttribute(PortElement::NODE_NAME_ATTRIBUTE); + const string& nodeGraphName = elem->getAttribute(PortElement::NODE_GRAPH_ATTRIBUTE); + const string& nodeString = elem->getAttribute(NodeDef::NODE_ATTRIBUTE); + const string& nodeDefString = elem->getAttribute(InterfaceElement::NODE_DEF_ATTRIBUTE); + + const string& portKey = !nodeName.empty() ? nodeName : nodeGraphName; + if (!portKey.empty()) + { + PortElementPtr portElem = elem->asA(); + if (portElem) { - if (!nodeGraphName.empty()) - { - PortElementPtr portElem = elem->asA(); - if (portElem) - { - portElementMap[portElem->getQualifiedName(nodeGraphName)].push_back(portElem); - } - } + _portElementMap[portElem->getQualifiedName(portKey)].push_back(portElem); } - if (!nodeString.empty()) + } + if (!nodeString.empty()) + { + NodeDefPtr nodeDef = elem->asA(); + if (nodeDef) { - NodeDefPtr nodeDef = elem->asA(); - if (nodeDef) - { - nodeDefMap[nodeDef->getQualifiedName(nodeString)].push_back(nodeDef); - } + _nodeDefMap[nodeDef->getQualifiedName(nodeString)].push_back(nodeDef); } - if (!nodeDefString.empty()) + } + if (!nodeDefString.empty()) + { + InterfaceElementPtr interface = elem->asA(); + if (interface) { - InterfaceElementPtr interface = elem->asA(); - if (interface) + if (interface->isA() || interface->isA()) { - if (interface->isA() || interface->isA()) - { - implementationMap[interface->getQualifiedName(nodeDefString)].push_back(interface); - } + _implementationMap[interface->getQualifiedName(nodeDefString)].push_back(interface); } } } - - valid = true; } + + // Release semantics ensure all map writes are visible before valid becomes true. + _valid.store(true, std::memory_order_release); } - public: - weak_ptr doc; - std::mutex mutex; - bool valid; - std::unordered_map> portElementMap; - std::unordered_map> nodeDefMap; - std::unordered_map> implementationMap; + private: + weak_ptr _doc; + std::mutex _mutex; + std::atomic _valid; + std::unordered_map> _portElementMap; + std::unordered_map> _nodeDefMap; + std::unordered_map> _implementationMap; }; // @@ -124,7 +163,7 @@ Document::~Document() void Document::initialize() { _root = getSelf(); - _cache->doc = getDocument(); + _cache->setDocument(getDocument()); clearContent(); setVersionIntegers(MATERIALX_MAJOR_VERSION, MATERIALX_MINOR_VERSION); @@ -284,18 +323,7 @@ std::pair Document::getVersionIntegers() const vector Document::getMatchingPorts(const string& nodeName) const { - // Refresh the cache. - _cache->refresh(); - - // Return all port elements matching the given node name. - if (_cache->portElementMap.count(nodeName)) - { - return _cache->portElementMap.at(nodeName); - } - else - { - return vector(); - } + return _cache->getMatchingPorts(nodeName); } ValuePtr Document::getGeomPropValue(const string& geomPropName, const string& geom) const @@ -342,19 +370,14 @@ vector Document::getMaterialOutputs() const vector Document::getMatchingNodeDefs(const string& nodeName) const { // Recurse to data library if present. - vector matchingNodeDefs = hasDataLibrary() ? + vector matchingNodeDefs = hasDataLibrary() ? getDataLibrary()->getMatchingNodeDefs(nodeName) : vector(); - // Refresh the cache. - _cache->refresh(); + // Append all nodedefs matching the given node name. + vector localNodeDefs = _cache->getMatchingNodeDefs(nodeName); + matchingNodeDefs.insert(matchingNodeDefs.end(), localNodeDefs.begin(), localNodeDefs.end()); - // Return all nodedefs matching the given node name. - if (_cache->nodeDefMap.count(nodeName)) - { - matchingNodeDefs.insert(matchingNodeDefs.end(), _cache->nodeDefMap.at(nodeName).begin(), _cache->nodeDefMap.at(nodeName).end()); - } - return matchingNodeDefs; } @@ -364,15 +387,10 @@ vector Document::getMatchingImplementations(const string& n vector matchingImplementations = hasDataLibrary() ? getDataLibrary()->getMatchingImplementations(nodeDef) : vector(); - - // Refresh the cache. - _cache->refresh(); - // Return all implementations matching the given nodedef string. - if (_cache->implementationMap.count(nodeDef)) - { - matchingImplementations.insert(matchingImplementations.end(), _cache->implementationMap.at(nodeDef).begin(), _cache->implementationMap.at(nodeDef).end()); - } + // Append all implementations matching the given nodedef string. + vector localImpls = _cache->getMatchingImplementations(nodeDef); + matchingImplementations.insert(matchingImplementations.end(), localImpls.begin(), localImpls.end()); return matchingImplementations; } @@ -388,7 +406,7 @@ bool Document::validate(string* message) const void Document::invalidateCache() { - _cache->valid = false; + _cache->invalidate(); } // From 2a288e3d231c409b1872b1594d53ce834b14f572 Mon Sep 17 00:00:00 2001 From: Jonathan Stone Date: Mon, 5 Jan 2026 10:15:06 -0800 Subject: [PATCH 2/4] Switch to std::shared_mutex Following up on review notes from Jerry Gamache, this change switches over to a std::shared_mutex pattern, which should be simpler and more robust. --- source/MaterialXCore/Document.cpp | 59 +++++++++++++++++-------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/source/MaterialXCore/Document.cpp b/source/MaterialXCore/Document.cpp index 76115687e6..6ee1c1d85e 100644 --- a/source/MaterialXCore/Document.cpp +++ b/source/MaterialXCore/Document.cpp @@ -5,8 +5,7 @@ #include -#include -#include +#include MATERIALX_NAMESPACE_BEGIN @@ -37,65 +36,72 @@ class Document::Cache void setDocument(weak_ptr document) { + std::unique_lock lock(_mutex); _doc = document; - invalidate(); + _valid = false; } void invalidate() { - _valid.store(false, std::memory_order_relaxed); + std::unique_lock lock(_mutex); + _valid = false; } vector getMatchingPorts(const string& nodeName) { - refresh(); + auto lock = refreshWithLock(); auto it = _portElementMap.find(nodeName); return (it != _portElementMap.end()) ? it->second : vector(); } vector getMatchingNodeDefs(const string& nodeName) { - refresh(); + auto lock = refreshWithLock(); auto it = _nodeDefMap.find(nodeName); return (it != _nodeDefMap.end()) ? it->second : vector(); } vector getMatchingImplementations(const string& nodeDef) { - refresh(); + auto lock = refreshWithLock(); auto it = _implementationMap.find(nodeDef); return (it != _implementationMap.end()) ? it->second : vector(); } private: - void refresh() + std::shared_lock refreshWithLock() { - // Perform a lock-free read and return if the cache is valid. - if (_valid.load(std::memory_order_acquire)) - { - return; - } + std::shared_lock lock(_mutex); - // Acquire a lock and double-check the valid flag for robustness. - std::lock_guard lock(_mutex); - if (_valid.load(std::memory_order_relaxed)) + if (_valid) { - return; + return lock; } - // Verify that the document is still valid. - auto doc = _doc.lock(); - if (!doc) + lock.unlock(); + { - return; + std::unique_lock writeLock(_mutex); + if (!_valid) + { + auto doc = _doc.lock(); + if (doc) + { + rebuild(doc); + } + } } - // Clear the existing cache. + lock.lock(); + return lock; + } + + void rebuild(DocumentPtr doc) + { _portElementMap.clear(); _nodeDefMap.clear(); _implementationMap.clear(); - // Traverse the document to build a new cache. for (ElementPtr elem : doc->traverseTree()) { const string& nodeName = elem->getAttribute(PortElement::NODE_NAME_ATTRIBUTE); @@ -133,14 +139,13 @@ class Document::Cache } } - // Release semantics ensure all map writes are visible before valid becomes true. - _valid.store(true, std::memory_order_release); + _valid = true; } private: weak_ptr _doc; - std::mutex _mutex; - std::atomic _valid; + mutable std::shared_mutex _mutex; + bool _valid; std::unordered_map> _portElementMap; std::unordered_map> _nodeDefMap; std::unordered_map> _implementationMap; From 4ca58aa5c98ce0a783885da376290f1dc9e04ec4 Mon Sep 17 00:00:00 2001 From: Jonathan Stone Date: Mon, 5 Jan 2026 10:20:02 -0800 Subject: [PATCH 3/4] Retore original comments --- source/MaterialXCore/Document.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source/MaterialXCore/Document.cpp b/source/MaterialXCore/Document.cpp index 6ee1c1d85e..38d156320e 100644 --- a/source/MaterialXCore/Document.cpp +++ b/source/MaterialXCore/Document.cpp @@ -98,10 +98,12 @@ class Document::Cache void rebuild(DocumentPtr doc) { + // Clear the existing cache. _portElementMap.clear(); _nodeDefMap.clear(); _implementationMap.clear(); + // Traverse the document to build a new cache. for (ElementPtr elem : doc->traverseTree()) { const string& nodeName = elem->getAttribute(PortElement::NODE_NAME_ATTRIBUTE); From a7765b6d28fa7537f4eda89ccf1ee2d606f10874 Mon Sep 17 00:00:00 2001 From: Jonathan Stone Date: Mon, 5 Jan 2026 10:22:01 -0800 Subject: [PATCH 4/4] Include for GCC and Clang --- source/MaterialXCore/Document.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/source/MaterialXCore/Document.cpp b/source/MaterialXCore/Document.cpp index 38d156320e..ec8bb54e84 100644 --- a/source/MaterialXCore/Document.cpp +++ b/source/MaterialXCore/Document.cpp @@ -5,6 +5,7 @@ #include +#include #include MATERIALX_NAMESPACE_BEGIN