pcp: Revert specializes culling optimization
authorsunyab <sunyab@users.noreply.github.com>
Sat, 3 Feb 2024 04:04:57 +0000 (20:04 -0800)
committerpixar-oss <pixar-oss@users.noreply.github.com>
Mon, 5 Feb 2024 20:21:06 +0000 (12:21 -0800)
Change 2312006 implemented prim index culling for specializes
nodes. This noticeably reduced memory usage on several test
production shots, but increased memory usage in at least one
other shot. More crucially, it caused runtime performance
regressions in authoring workflows that added new opinions
to prims in sites that were now being culled due to this
optimization. In this case, Pcp needs to fully recompute the
entire prim index to pick up the new opinions.

We lean towards better performance over memory usage, so
I'm reverting this change for now until we can figure out
how to address the authoring issue.

(Internal change: 2314652)

pxr/usd/pcp/primIndex.cpp
pxr/usd/pcp/testenv/testPcpPrimIndex.py

index 2780af3c69bacd06f99ce4eb4728426a7fa224a0..f8edd3b877fee27d3d40a8b7e79c14fbbea1b9e9 100644 (file)
@@ -4815,44 +4815,25 @@ _NodeCanBeCulled(
     return true;
 }
 
-// Cull all nodes in the subtree rooted at the given node whose site
-// is given in culledSites.
-static bool
-_CullMatchingChildrenInSubtree(
-    PcpNodeRef node,
-    const std::unordered_set<PcpLayerStackSite, TfHash>& culledSites)
-{
-    bool allChildrenCulled = true;
-    TF_FOR_ALL(child, Pcp_GetChildrenRange(node)) {
-        allChildrenCulled &=
-            _CullMatchingChildrenInSubtree(*child, culledSites);
-    }
-
-    if (allChildrenCulled && culledSites.count(node.GetSite())) {
-        node.SetCulled(true);
-    }
-
-    return node.IsCulled();
-}
-
 // Helper that recursively culls subtrees at and under the given node.
 static void
-_CullSubtreesWithNoOpinionsHelper(
+_CullSubtreesWithNoOpinions(
     PcpNodeRef node,
     const PcpLayerStackSite& rootSite,
-    std::vector<PcpCulledDependency>* culledDeps,
-    std::unordered_set<PcpLayerStackSite, TfHash>* culledSites = nullptr)
+    std::vector<PcpCulledDependency>* culledDeps)
 {
     // Recurse and attempt to cull all children first. Order doesn't matter.
     TF_FOR_ALL(child, Pcp_GetChildrenRange(node)) {
-        // Skip culling for specializes subtrees here; these will be handled
-        // by _CullSubtreesWithNoOpinions. See comments there for more info.
+        // XXX: 
+        // We propagate and maintain duplicate node structure in the graph
+        // for specializes arcs, so when we cull we need to ensure we do so
+        // in both places consistently. For simplicity, we're going to skip
+        // this for now and not cull beneath any specializes arcs.
         if (PcpIsSpecializeArc(child->GetArcType())) {
             continue;
         }
 
-        _CullSubtreesWithNoOpinionsHelper(
-            *child, rootSite, culledDeps, culledSites);
+        _CullSubtreesWithNoOpinions(*child, rootSite, culledDeps);
     }
 
     // Now, mark this node as culled if we can. These nodes will be
@@ -4865,50 +4846,9 @@ _CullSubtreesWithNoOpinionsHelper(
         // index when Finalize() is called, so they must be saved separately
         // for later use.
         Pcp_AddCulledDependency(node, culledDeps);
-
-        if (culledSites) {
-            culledSites->insert(node.GetSite());
-        }
     }
 }
 
-static void
-_CullSubtreesWithNoOpinions(
-    PcpPrimIndex* primIndex,
-    const PcpLayerStackSite& rootSite,
-    std::vector<PcpCulledDependency>* culledDeps)
-{
-    // We propagate and maintain duplicate node structure in the graph
-    // for specializes arcs so when we cull we need to ensure we do so
-    // in both places consistently. 
-    //
-    // The origin subtree is marked inert as part of propagation, which
-    // means culling would remove it entirely which is not what we want.
-    // Instead, we cull whatever nodes we can in the propagated subtree
-    // under the root of the prim index, then cull the corresponding
-    // nodes underneath the origin subtree.
-    //
-    // We do a first pass to handle of all these propagated specializes
-    // nodes first to ensure that nodes in the origin subtrees are marked
-    // culled before other subtrees are processed. Otherwise, subtrees
-    // containing those origin subtrees won't be culled.
-    TF_FOR_ALL(child, Pcp_GetChildrenRange(primIndex->GetRootNode())) {
-        if (_IsPropagatedSpecializesNode(*child)) {
-            std::unordered_set<PcpLayerStackSite, TfHash> culledSites;
-            _CullSubtreesWithNoOpinionsHelper(
-                *child, rootSite, culledDeps, &culledSites);
-
-            _CullMatchingChildrenInSubtree(child->GetOriginNode(), culledSites);
-        }
-    }
-
-    TF_FOR_ALL(child, Pcp_GetChildrenRange(primIndex->GetRootNode())) {
-        if (!_IsPropagatedSpecializesNode(*child)) {
-            _CullSubtreesWithNoOpinionsHelper(*child, rootSite, culledDeps);
-        }
-    }
-}    
-
 // Helper that sets any nodes that cannot have overrides on name children
 // as inert.
 struct Pcp_DisableNonInstanceableNodesVisitor
@@ -5209,7 +5149,7 @@ PcpComputePrimIndex(
     // Mark subtrees in the graph that provide no opinions as culled.
     if (inputs.cull) {
         _CullSubtreesWithNoOpinions(
-            &outputs->primIndex, site,
+            outputs->primIndex.GetRootNode(), site,
             &outputs->culledDependencies);
     }
 
index db92d2397c767b4270db0cd2bf7893b75fd39a49..f3541c371572c349defb30226b4f7a63ef235cb3 100644 (file)
@@ -24,7 +24,7 @@
 
 import unittest
 
-from pxr import Pcp, Sdf, Tf
+from pxr import Pcp, Sdf
 
 def LoadPcpCache(rootLayer):
     return Pcp.Cache(Pcp.LayerStackIdentifier(rootLayer))
@@ -48,27 +48,6 @@ class TestPcpPrimIndex(unittest.TestCase):
                 node.GetSpecContributionRestrictedDepth(), expectedDepth,
                 "Error at {} {}".format(node.arcType, node.path))
 
-    def AssertPrimIndex(self, pcpCache, path, expected):
-        pi, err = pcpCache.ComputePrimIndex(path)
-        self.assertFalse(err, "Unexpected composition errors: {}".format(
-            ",".join(str(e) for e in err)))
-
-        for node, e in Pcp._TestPrimIndex(pi, expected):
-            expectedArcType, expectedLayerStack, expectedPath = e
-            
-            self.assertEqual(
-                node.arcType, expectedArcType,
-                "Error at {} {} {}".format(node.arcType, node.layerStack, node.path))
-            self.assertEqual(
-                node.layerStack.identifier.rootLayer, expectedLayerStack,
-                "Error at {} {} {}".format(node.arcType, node.layerStack, node.path))
-            self.assertEqual(
-                node.isCulled, False,
-                "Error at {} {} {}".format(node.arcType, node.layerStack, node.path))
-            self.assertEqual(
-                node.path, expectedPath,
-                "Error at {} {}{}".format(node.arcType, node.layerStack, node.path))
-
     def test_TestPrimIndex(self):
         """Test _TestPrimIndex generator"""
         
@@ -559,377 +538,5 @@ class TestPcpPrimIndex(unittest.TestCase):
                 ]
             ])
 
-    @unittest.skipIf(not Tf.GetEnvSetting("PCP_CULLING"), "Culling is disabled")
-    def test_PrimIndexCulling_Specializes(self):
-        """Tests node culling optimization with specializes arcs"""
-        refLayer = Sdf.Layer.CreateAnonymous("ref")
-        refLayer.ImportFromString('''
-        #sdf 1.4.32
-
-        def "SpecRefA"
-        {
-            def "ChildA"
-            {
-            }
-        }
-
-        def "SpecRefB"
-        {
-        }
-
-        def "Spec" (
-            references = [</SpecRefA>, </SpecRefB>]
-        )
-        {
-        }
-
-        def "Ref" (
-            specializes = </Spec>
-        )
-        {
-        }
-        '''.strip())
-
-        rootLayer = Sdf.Layer.CreateAnonymous("root")
-        rootLayer.ImportFromString(f'''
-        #sdf 1.4.32
-
-        def "Root" (
-            references = @{refLayer.identifier}@</Ref>
-        )
-        {{
-            def "ChildB"
-            {{
-            }}
-        }}
-        '''.strip())
-        
-        pcp = LoadPcpCache(rootLayer)
-
-        # Compute /Root. No culling occurs here since there are opinions
-        # at all sites.
-        self.AssertPrimIndex(
-            pcp, "/Root",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root"), [
-                    # Reference from /Root -> /Ref in @root@
-                    (Pcp.ArcTypeReference, refLayer, "/Ref"), [
-                        (Pcp.ArcTypeSpecialize, refLayer, "/Spec"), [
-                            (Pcp.ArcTypeReference, refLayer, "/SpecRefA"), [],
-                            (Pcp.ArcTypeReference, refLayer, "/SpecRefB"), []
-                        ]
-                    ],
-
-                    # Implied specialize due to /Ref -> /Spec in @ref@
-                    (Pcp.ArcTypeSpecialize, rootLayer, "/Spec"), [],
-
-                    # Propagated specialize due to /Ref -> /Spec in @ref@
-                    (Pcp.ArcTypeSpecialize, refLayer, "/Spec"), [
-                        (Pcp.ArcTypeReference, refLayer, "/SpecRefA"), [],
-                        (Pcp.ArcTypeReference, refLayer, "/SpecRefB"), []
-                    ]
-                ]
-            ])
-
-        # Compute /Root/ChildA. The reference arcs pointing to /SpecRefB
-        # beneath the specializes arcs are culled since there are no opinions
-        # at /SpecRefB/ChildA.
-        self.AssertPrimIndex(
-            pcp, "/Root/ChildA",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root/ChildA"), [
-                    # Reference from /Root -> /Ref in @root@
-                    (Pcp.ArcTypeReference, refLayer, "/Ref/ChildA"), [
-                        (Pcp.ArcTypeSpecialize, refLayer, "/Spec/ChildA"), [
-                            (Pcp.ArcTypeReference, refLayer, "/SpecRefA/ChildA"), []
-                        ]
-                    ],
-
-                    # Propagated specialize due to /Ref -> /Spec in @ref@
-                    (Pcp.ArcTypeSpecialize, refLayer, "/Spec/ChildA"), [
-                        (Pcp.ArcTypeReference, refLayer, "/SpecRefA/ChildA"), []
-                    ]
-                ]
-            ])
-
-        # Compute /Root/ChildB. All nodes should be culled since there are
-        # no opinions for ChildB anywhere except at /Root/ChildB.
-        self.AssertPrimIndex(
-            pcp, "/Root/ChildB",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root/ChildB"), []
-            ])
-
-    @unittest.skipIf(not Tf.GetEnvSetting("PCP_CULLING"), "Culling is disabled")
-    def test_PrimIndexCulling_SubrootSpecializes(self):
-        """Tests node culling optimization with specializes arcs pointing
-        to subroot prims"""
-        refLayer = Sdf.Layer.CreateAnonymous("ref")
-        refLayer.ImportFromString('''
-        #sdf 1.4.32
-
-        def "SpecRefA"
-        {
-            def "ChildA"
-            {
-            }
-        }
-
-        def "SpecRefB"
-        {
-        }
-
-        def "Ref"
-        {
-            def "Instance" (
-                specializes = </Ref/Spec>
-            )
-            {
-            }
-
-            def "Spec" (
-                references = [</SpecRefA>, </SpecRefB>]
-            )
-            {
-            }
-        }
-        '''.strip())
-
-        rootLayer = Sdf.Layer.CreateAnonymous("root")
-        rootLayer.ImportFromString(f'''
-        #sdf 1.4.32
-
-        def "Root" (
-            references = @{refLayer.identifier}@</Ref>
-        )
-        {{
-            def "Spec"
-            {{
-            }}
-
-            def "Instance"
-            {{
-                def "ChildB"
-                {{
-                }}
-            }}
-        }}
-        '''.strip())
-        
-        pcp = LoadPcpCache(rootLayer)
-
-        # Compute /Root/Instance. No culling occurs here since there are
-        # opinions at all sites.
-        self.AssertPrimIndex(
-            pcp, "/Root/Instance",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root/Instance"), [
-                    # Reference from /Root -> /Ref in @root@
-                    (Pcp.ArcTypeReference, refLayer, "/Ref/Instance"), [
-                        (Pcp.ArcTypeSpecialize, refLayer, "/Ref/Spec"), [
-                            (Pcp.ArcTypeReference, refLayer, "/SpecRefA"), [],
-                            (Pcp.ArcTypeReference, refLayer, "/SpecRefB"), []
-                        ]
-                    ],
-
-                    # Implied specialize due to /Ref/Instance -> /Ref/Spec in @ref@
-                    (Pcp.ArcTypeSpecialize, rootLayer, "/Root/Spec"), [],
-
-                    # Propagated specialize due to /Ref/Instance -> /Ref/Spec in @ref@
-                    (Pcp.ArcTypeSpecialize, refLayer, "/Ref/Spec"), [
-                        (Pcp.ArcTypeReference, refLayer, "/SpecRefA"), [],
-                        (Pcp.ArcTypeReference, refLayer, "/SpecRefB"), []
-                    ]
-                ]
-            ])
-
-        # Compute /Root/Instance/ChildA. The reference arcs pointing to
-        # /SpecRefB beneath the specializes arcs are culled since there are no
-        # opinions at /SpecRefB/ChildA.
-        self.AssertPrimIndex(
-            pcp, "/Root/Instance/ChildA",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root/Instance/ChildA"), [
-                    # Reference from /Root -> /Ref in @root@
-                    (Pcp.ArcTypeReference, refLayer, "/Ref/Instance/ChildA"), [
-                        (Pcp.ArcTypeSpecialize, refLayer, "/Ref/Spec/ChildA"), [
-                            (Pcp.ArcTypeReference, refLayer, "/SpecRefA/ChildA"), []
-                        ]
-                    ],
-
-                    # Propagated specialize due to /Ref/Instance -> /Ref/Spec in @ref@
-                    (Pcp.ArcTypeSpecialize, refLayer, "/Ref/Spec/ChildA"), [
-                        (Pcp.ArcTypeReference, refLayer, "/SpecRefA/ChildA"), []
-                    ]
-                ]
-            ])
-
-        # Compute /Root/Instance/ChildB. All nodes should be culled since there
-        # are no opinions for ChildB anywhere except at /Root/Instance/ChildB.
-        self.AssertPrimIndex(
-            pcp, "/Root/Instance/ChildB",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root/Instance/ChildB"), []
-            ])
-
-    @unittest.skipIf(not Tf.GetEnvSetting("PCP_CULLING"), "Culling is disabled")
-    def test_PrimIndexCulling_SpecializesHierarchy(self):
-        """Tests node culling optimization with hierarchy of specializes arcs"""
-
-        rootLayer = Sdf.Layer.CreateAnonymous()
-        rootLayer.ImportFromString('''
-        #sdf 1.4.32
-
-        def "Ref"
-        {
-            def "Instance" (
-                specializes = </Ref/SpecA>
-            )
-            {
-            }
-
-            def "SpecA" (
-                specializes = </Ref/SpecB>
-            )
-            {
-            }
-
-            def "SpecB"
-            {
-                def "Child"
-                {
-                }
-            }
-        }
-
-        def "Root" (
-            references = </Ref>
-        )
-        {
-        }        
-        '''.strip())
-        
-        pcp = LoadPcpCache(rootLayer)
-
-        # Compute /Root/Instance to show initial prim index structure.
-        # No culling occurs here.
-        self.AssertPrimIndex(
-            pcp, "/Root/Instance",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root/Instance"), [
-                    # Reference from /Root -> /Ref
-                    (Pcp.ArcTypeReference, rootLayer, "/Ref/Instance"), [
-                        # Specializes from /Ref/Instance -> /Ref/SpecA
-                        (Pcp.ArcTypeSpecialize, rootLayer, "/Ref/SpecA"), [
-                            # Specializes from /Ref/SpecA -> /Ref/SpecB
-                            (Pcp.ArcTypeSpecialize, rootLayer, "/Ref/SpecB"), []
-                        ]
-                    ],
-
-                    # Implied specializes due to /Ref/Instance -> /Ref/SpecA
-                    (Pcp.ArcTypeSpecialize, rootLayer, "/Root/SpecA"), [
-                        (Pcp.ArcTypeSpecialize, rootLayer, "/Root/SpecB"), []
-                    ],
-
-                    # Propagated specializes due to /Ref/Instance -> /Ref/SpecA
-                    (Pcp.ArcTypeSpecialize, rootLayer, "/Ref/SpecA"), [],
-
-                    # Propagated specializes due to implied /Root/SpecB
-                    (Pcp.ArcTypeSpecialize, rootLayer, "/Root/SpecB"), [],
-
-                    # Propagated specializes due to /Ref/SpecA -> /Ref/SpecB.
-                    (Pcp.ArcTypeSpecialize, rootLayer, "/Ref/SpecB"), []
-                ]
-            ])
-
-        # Compute /Root/Instance/Child. Most of the nodes are culled and
-        # removed because they provide no opinions on the Child prim, but
-        # the origin specializes hierarchy noted below cannot be culled.
-        self.AssertPrimIndex(
-            pcp, "/Root/Instance/Child",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root/Instance/Child"), [
-                    # Reference from /Root -> /Ref
-                    (Pcp.ArcTypeReference, rootLayer, "/Ref/Instance/Child"), [
-                        # The propagated specializes subtree for 
-                        # /Ref/SpecA/Child is culled, but the propagated subtree
-                        # for /Ref/SpecB/Child is not. That prevents the origin
-                        # subtree for /Ref/SpecA/Child from being culled.
-                        (Pcp.ArcTypeSpecialize, rootLayer, "/Ref/SpecA/Child"), [
-                            (Pcp.ArcTypeSpecialize, rootLayer, "/Ref/SpecB/Child"), []
-                        ]
-                    ],
-
-                    # Propagated specializes due to /Ref/SpecA -> /Ref/SpecB.
-                    (Pcp.ArcTypeSpecialize, rootLayer, "/Ref/SpecB/Child"), []
-                ]
-            ])
-
-    @unittest.skipIf(not Tf.GetEnvSetting("PCP_CULLING"), "Culling is disabled")
-    def test_PrimIndexCulling_SpecializesAncestralCulling(self):
-        """Tests node culling optimization where the prim index for
-        the parent prim contains a node that is marked as culled
-        but has not been removed from the prim index."""
-        rootLayer = Sdf.Layer.CreateAnonymous()
-        rootLayer.ImportFromString('''
-        #sdf 1.4.32
-
-        def "RefB"
-        {
-            def "Instance" (
-                specializes = </RefB/Spec>
-            )
-            {
-                def "Child"
-                {
-                }
-            }
-
-            def "Spec"
-            {
-                def "Child"
-                {
-                }
-            }
-        }
-
-        def "Inh"
-        {
-        }
-
-        def "Ref" (
-            references = </RefB>
-            inherits = </Inh>
-        )
-        {
-        }
-
-        def "Root" (
-            references = </Ref>
-        )
-        {
-        }
-        '''.strip())
-
-        pcp = LoadPcpCache(rootLayer)
-
-        # Compute /Root/Instance/Child. The important part here is that the
-        # subtrees for the implied specializes nodes at /Ref/Spec/Child and
-        # /Root/Spec/Child have been culled and removed from the prim index.
-        self.AssertPrimIndex(
-            pcp, "/Root/Instance/Child",
-            [
-                (Pcp.ArcTypeRoot, rootLayer, "/Root/Instance/Child"), [
-                    (Pcp.ArcTypeReference, rootLayer, "/Ref/Instance/Child"), [
-                        (Pcp.ArcTypeReference, rootLayer, "/RefB/Instance/Child"), [
-                            (Pcp.ArcTypeSpecialize, rootLayer, "/RefB/Spec/Child"), [],
-                        ]
-                    ],
-
-                    (Pcp.ArcTypeSpecialize, rootLayer, "/RefB/Spec/Child")
-                ]
-            ])
-
-
 if __name__ == "__main__":
     unittest.main()