Fix O(N^2) algorithm on the number of geometry subsets in Hydra

[marktucker]

Fix O(N^2) algorithm when displaying a mesh with a very large number of geometry subsets on it. Use a Set instead of a Vector for extraDependencies. This was resulting in extremely long update times in Hydra when asked to display a mesh with 500K geomsubsets in the Moana island scene (ignoring the wisdom of organizing the USD that way).

Description of Change(s)

Replace the use of an SdfPathVector with an SdfPathSet to avoid linear search costs.

[jilliene]

Filed as internal issue #USD-6004

[spiffmon]

Thanks, @marktucker ! Given the benefits of coherent memory access and that we don't expect the common case for Subsets to be so meaty, I wonder if it might make sense to try a TfDenseHashSet here?

[spitzak]

I think std::set is more space-efficient than hash sets?

[marktucker]

I'll give TfDenseHashSet a try, just to validate it gives similar performance characteristics. Related to this, I noticed that extraDependencies is never cleared, and when I remove this prim from the scene, UsdImagingIndexProxy::_RemoveDependencies is getting called for each geom subset, which results in the _dependenciesToRemove vector trying to fill with 500K*500K entries, of which only the first 500K are unique. Should there be a call to primInfo->extraDependencies.clear() at the end of that function?

Alternatively _dependenciesToRemove could be made into a set of some kind, but it would still be doing N^2 insertion tests without the clearing of the extraDependencies, so performance will still be awful (but at least it won't chew up all my RAM)... Any thoughts?

[marktucker]

TfDenseHashSet works great as a replacement for std::set. I'm happy to defer to your judgement on balancing performance vs. memory use.

I also tried adding the extraDependencies.clear() call and it made the removal of the 500K subsets very fast, and didn't seem to prevent it from coming back when I re-added it. The extraDependencies were also rebuilt. But I fear that you leave the extraDependencies around for some more complex code path. This comment https://github.com/PixarAnimationStudios/USD/blob/ebac0a8b6703f4fa1c27115f1f013bb9819662f4/pxr/usdImaging/usdImaging/indexProxy.cpp#L158 makes me think there is more going on here?

[spiffmon]

I think std::set is more space-efficient than hash sets?

That is true, @spitzak , but TfDenseHashSet will be a std::vector until a specified threshold of elements is met. Though there's of course a tuning issue in that threshold, we'd hope that most Mesh's have few enough subsets that they'd stay a vector.

[spiffmon]

With respect to clearing the dependencies, @marktucker, that's beyond my expertise. Maybe try it with comment and see what the Hydra team says?

TfDenseHashSet works great as a replacement for std::set. I'm happy to defer to your judgement on balancing performance vs. memory use.

I also tried adding the extraDependencies.clear() call and it made the removal of the 500K subsets very fast, and didn't seem to prevent it from coming back when I re-added it. The extraDependencies were also rebuilt. But I fear that you leave the extraDependencies around for some more complex code path. This comment

https://github.com/PixarAnimationStudios/USD/blob/ebac0a8b6703f4fa1c27115f1f013bb9819662f4/pxr/usdImaging/usdImaging/indexProxy.cpp#L158

makes me think there is more going on here?

[spitzak]

Sounds good to me

[c64kernal]

Hey @marktucker, I think that needs to be denseHashSet.h -- capital 'S'

[marktucker]

It sure does. Unless you're on Windows. Sigh. Fixed.

[c64kernal]

@marktucker with respect to the clearing -- we did run into some issues a while back where uses cases involved in our Presto delegates that handle multiple USD stages at once ran into problems, but that's been a while now and things have changed. I'm wondering if we can try it again. If you'd like to submit it in a different PR, please, I'd be happy to run it through our internal tests and validate it.