Improving NodeGraph::getNodeDef performance

[seang-ilm]

Fuzz testing was consistently findings some very slow inputs. Looking at this inputs through perf.

I've added a performance regression case that builds example trees that have this issue.

test -R MaterialXTest_NodeGraph_getNodeDef_performance -VV
...
27: -------------------------------------------------------------------------------
27: NodeGraph getNodeDef performance
27: -------------------------------------------------------------------------------
27: /home/sean/MaterialX_Clean/source/MaterialXTest/MaterialXCore/Performance.cpp:53
27: ...............................................................................
27:
27: benchmark name                       samples       iterations    estimated
27:                                      mean          low mean      high mean
27:                                      std dev       low std dev   high std dev
27: -------------------------------------------------------------------------------
27: validate, stubs=200 refs=100                   100             1     2.91805 m
27:                                         4.20239 ms    4.00369 ms    4.54723 ms
27:                                         1.30152 ms    875.207 us    2.10073 ms
27:
27: validate, stubs=1000 refs=100                  100             1      1.5165 s
27:                                         13.4589 ms    12.2686 ms    15.7144 ms
27:                                          8.1092 ms    5.15263 ms      13.99 ms
27:
27: validate, stubs=10000 refs=100                 100             1     23.5368 s
27:                                         154.384 ms    141.568 ms    192.638 ms
27:                                         103.788 ms    40.5831 ms    224.837 ms
27:
27:
27: 29.193 s: NodeGraph getNodeDef performance
27: ===============================================================================
27: test cases: 1 | 1 passed
27: assertions: - none -
27:
1/1 Test #27: MaterialXTest_NodeGraph_getNodeDef_performance ...   Passed   30.73 sec

I think I've found a couple of issues that come together to cause the processing time for loading .mtlx files grow linearly.

• Syntax::makeValidName uses std::find on _reservedWords. This is a std::set<string> and supports a faster O(log N) .find(). Give's a minor improvement (5-6%)

MaterialX/source/MaterialXGenShader/Syntax.cpp

Lines 152 to 162 in 9ebe5f1

	void Syntax::makeValidName(string& name) const
	{
	std::replace_if(name.begin(), name.end(), isInvalidChar, '_');
	name = replaceSubstrings(name, _invalidTokens);
	if (std::find(_reservedWords.begin(), _reservedWords.end(), name) != _reservedWords.end())
	{
	// We append "1" here because thats the prior behavior from makeIdentifier() below when
	// the reservedWords were added to the identifiers list.
	name = name+"1";
	}
	}

• NodeGraph::getNodeDef calls Document::getImplementations() on every invocation. This rebuilds a fresh vector by scanning every document-level child with a dynamic_pointer_cast on each, with no result reuse. Document::Cache already maintains hash-map lookups for ports, nodedefs, and nodedDefImplementation, but the reverse direction (nodegraph-name -> implementation) was missing. I've added a _nodeGraphImplMap to the cache, populated during rebuild() alongside the existing maps, and replaced the linear scan with an O(1) lookup.

MaterialX/source/MaterialXCore/Node.cpp

Lines 742 to 756 in b661a4b

	NodeDefPtr NodeGraph::getNodeDef() const
	{
	NodeDefPtr nodedef = resolveNameReference<NodeDef>(getNodeDefString());
	// If not directly defined look for an implementation which has a nodedef association
	if (!nodedef)
	{
	for (auto impl : getDocument()->getImplementations())
	{
	if (impl->getNodeGraph() == getQualifiedName(getName()))
	{
	nodedef = impl->getNodeDef();
	}
	}
	}
	return nodedef;

If we look at benchmark time I get a significant improvement

Baseline	Time(s)
Unmodified	44.8
Syntax::makeValidName, fix _reservedWords check	41.94
Use Document::Cache lookups	12.34

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: seang-ilm / name: Sean Gilligan (2dbf32f, 7c5f879, ad3b326)

[ld-kerley]

I think its possible for more than one Implementation element to use the same NodeGraph - so this might need to be

std::unordered_map<string, std::vector>

For standard surface (here) we have two different Implementation elements that have the same NodeGraph

[kwokcb]

Curious if there are any metrics for the additional update cost especially if the cache updates frequently ? At most currently 1 or a few items will ever be found and it's mostly static during the lifetime of a document.

[ld-kerley]

This seems like a nice improvement :) good catch

[kwokcb]

Would it make sense that instead of caching the implementation reference and then getting the nodedef here, just store the nodedef reference as the lookup ?

[kwokcb]

Curious if there are any metrics for the additional update cost especially if the cache updates frequently ? At most currently 1 or a few items will ever be found and it's mostly static during the lifetime of a document.