Fix performance issues with graph edge iteration in ShaderGraph

[nadult]

In complex materials graph and shader graph edge iteration can be extremely slow, because some edges may be visited unnecessarily multiple times. This is especially noticable in two functions: ShaderGraph::addUpstreamDependencies and ShaderGraph::optimize() .

GraphIterator and ShaderGraphEdgeIterator classes iterate over DAGs without marking nodes as visited, which may lead to exponential traversal time for some DAGs: https://stackoverflow.com/a/69326676

This patch adds two functions which efficiently generate a unique list of (shader) graph edges and uses those lists instead of graph iterators.

For one especially complex material on which I tested this fix, shader code generation time decreased from 156.13 sec to 0.08 sec (almost 2000x speedup). Number of visited edges in ShaderGraph::optimize() decreased from 42M to ~600.

I'm not sure if the best approach is to add those two functions next to current graph iterators or maybe those iterators should be replaced altogether. Another solution which I see would be to fix the graph iterators, but IMO they add a lot of overhead and it's faster to simply use a function which returns a list of all edges in a given graph in std::vector.

[jstone-lucasfilm]

This is a really interesting proposal, @nadult, and my main concern is that we'd have two subtly different approaches for traversing graphs in MaterialX, each of which would need to be maintained, validated, and extended in the future.

Perhaps it would actually be better to upgrade the GraphIterator class (as in one of your suggestions above), storing the required visitation data directly in this class to avoid duplicate edges?

This would still allow the addition of a top-level uniqueGraphEdges convenience method, which would be defined as a simple wrapper around traverseGraph, and the traversal system would remain unified for future maintenance.

[nadult]

OK, makes sense. I will try to prepare a new solution this week with your suggestions included.

[kwokcb]

For this, I think it would be useful to have the traversal API have 2 extensions:

1. Still allow for incremental traversal using the existing iterator but allow the option to avoid duplicates. Guess that could be the default on a version bump?
2. Have the ability to get all unique edges via the uniqueGraphEdges which pre-traversses and returns all edges.

Item (1) I think is useful to preserve as an integration may iterate and want to perform some action during traversal, including extracting out additional information which may be hard to get afterwards since not all information during traversal is cached on an edge.

Of course I only looked at the changes briefly, so just my initial thoughts. Thoughts on this ? Thanks.

[nadult]

I updated the solution in the following way:

• GraphIterator has an option to skip visited edges. By default it's disabled. In Element uniqueTraverseGraph is added, which works just like traverseGraph, but has skipVisitedEdges option enabled. This function is used instead of traverseGraph() in two places to improve traversal performance (other instances are unchanged).
• ShaderGraphEdgeIterator is always skipping visited edges, because that's how we want to traverse in the single use case (ShaderGraph::traverseUpstream).
• I removed uniqueGraphEdges functions, because there is no use case for them any more.

[nadult]

Here is an example of a complex material, for which this PR makes a big difference: slow_material.zip When opened in MaterialXView, originally it loaded in ~45 seconds. With this change it loads in ~1sec.

[jstone-lucasfilm]

@nadult From a first read, this looks really promising, and thanks for the example material! We would certainly benefit from test suite examples that are focused on loading/traversal performance (rather than render performance), and it seems worthwhile to consider a contribution along these lines in a future PR.

[ld-kerley]

The performance improvements here are really impressive! This looks good to me.

[kwokcb]

• Can we still add this file, or something equivalent to the test suite are as part of this PR? There are timers in the test suite.
• BTW: The GraphEditor often "hangs" even when trying to view the graph (it uses the iterator for load/building the UI graph). Curious @nadult if you've tried your changes with this editor ?
• Once this is good, it would be worthwhile to add in the Python and Javascript wrappers. Thanks.

[nadult]

@kwokcb OK, I will try to do what you suggest on Monday. I tried the editor some time ago and it was also working slowly on my materials. I will check if this PR fixes this issue as well, if not then I will modify it so that it does.

[jstone-lucasfilm]

@kwokcb I'd recommend leaving new test suite examples to a follow-up PR, as we'll want to spend some real time in designing and validating those new examples.

Similarly, let's make sure we have exactly the right API before moving forward with Python bindings, and I would recommend omitting new JavaScript bindings until they're needed in a concrete application. It's always easier to add new bindings than it is to modify or remove existing ones.

[nadult]

I tested GraphEditor on slow_material. Without my fix, on my PC it opens in 110 seconds, with the fix in 10 seconds. slow_material node graph looks like this: It was generated programmatically from a different (proprietary) format, so it's far from being optimal.

[nadult]

I profiled GraphEditor during loading of slow_material. After applying my fix, more than 70% of time is spent in Graph::layoutPosition, so it's no longer related to this PR:

[jstone-lucasfilm]

This looks like a great improvement, @nadult, and at this point I believe we're just making sure that all of the new naming conventions and flags are set up in a forward-looking way. I'll plan to take a closer look at this soon, and our goal would be to merge this change for MaterialX 1.39.2.