Replace `boost::multi_index_container` with custom container in `stageCache.cpp` #2349

nvmkuruc · 2023-03-21T21:05:43Z

Description of Change(s)

To reduce the dependency on boost, this converts the StageContainer into a class containing three synchronized unordered maps for each lookup type. For many operations that are read only, the underlying maps can simply be exposed. For insertion / erasure operations, explicit methods are provided on StageContainer to ensure the underlying containers don't violate the synchronization invariant.

Fixes Issue(s)

I have verified that all unit tests pass with the proposed changes

I have submitted a signed Contributor License Agreement

tallytalwar · 2023-03-23T22:20:33Z

Filed as internal issue #USD-8136

crevilla · 2023-07-05T21:22:08Z

pxr/usd/usd/stageCache.cpp

+        if (  it != _byStage.cend()) {
+            return std::make_pair(it->second, false);
+        }
+        const Id id = GetNextId();


Notes about all the uses of TF_AXIOM:

We definitely do not want TF_AXIOM wrapping any statements with a side effect like the emplace statements here. The docs for TF_AXIOM state that these statements are allowed to be converted to no-ops in an optimized build (even though we don't do that in any builds currently)

We prefer to use TF_VERIFY around an expression that is required to return true or even a normal if conditional that produces a TF_CODING_ERROR it the condition is not true.

Do we need axioms/verifies here at all?

emplace returning false would be a red flag that the class's invariants have been violated. I think some sort of unlikely failure assertion would be helpful. I can switch this to a TF_VERIFY.

crevilla · 2023-07-05T21:25:14Z

pxr/usd/usd/stageCache.cpp

-struct ByStage {};
+    // Return true if the stage was successfully erased
+    bool Erase(const UsdStageRefPtr& stage) {
+        const auto it = _byStage.find(stage);


It's preferred that the return false; is on a separate line and wrapped in curly braces.

crevilla · 2023-07-05T21:25:34Z

pxr/usd/usd/stageCache.cpp

-        return entry.stage->GetRootLayer();
+    // Return true if the stage was successfully erased
+    bool Erase(const Id id) {
+        const auto it = _byId.find(id);


Same comment as above.

crevilla · 2023-07-05T21:27:52Z

pxr/usd/usd/stageCache.cpp

@@ -143,15 +220,11 @@ struct DebugHelper

    bool IsEnabled() const { return _enabled; }



Is the variadic template necessary here?
It looks like all callers of AddEntry pass the same UsdStageRefPtr, Id parameters.

It's not necessary. It may have been useful in an earlier version of the code, but AddEntry should now just take the Ptr / Id parameters.

crevilla · 2023-07-05T21:32:57Z

pxr/usd/usd/stageCache.cpp

@@ -553,11 +627,10 @@ UsdStageCache::Insert(const UsdStageRefPtr &stage)
    Id ret;

    { LockGuard lock(_mutex);
-        StagesByStage &byStage = _impl->stages.get<ByStage>();
-        auto iresult = byStage.insert(Entry(stage, GetNextId()));
+        auto iresult = _impl->stages.Insert(stage);
        if (iresult.second && debug.IsEnabled())


Can we add the braces around this debug.AddEntry too

crevilla · 2023-07-05T21:34:10Z

pxr/usd/usd/stageCache.cpp

-            if (request.IsSatisfiedBy(entry.stage))
-                return std::make_pair(entry.stage, false);
+        {
+            const IdsByStage &byStage = _impl->stages.ByStage();


While we're here, can we add braces around these nested statements.

nvmkuruc · 2023-07-05T21:55:44Z

Thanks for taking a look. I've updated the PR to address the notes.

crevilla · 2023-07-10T22:50:25Z

pxr/usd/usd/stageCache.cpp

+        // function
+        for (auto it = range.first; it != range.second;) {
+            if (conditionFn(it->second)) {
+                const auto byStageIt = _byStage.find(it->second);


Sorry for nitpicking, but the only issue here is that it this verify does happen to fail, this will loop infinitely since the iterator doesn't get updated. Maybe starting at 200, the code should be

it = _byRootLayer.erase(it); ++erasedCount; continue; } } ++it;

I restructured the loop to avoid the need for a continue in the spirit of avoiding in non-trivial loops (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Res-continue).

…eCache.cpp`

nvmkuruc force-pushed the stagecachemulti branch from f045dc9 to 0c8239c Compare April 3, 2023 21:49

nvmkuruc force-pushed the stagecachemulti branch 2 times, most recently from b03cc91 to 2620000 Compare July 3, 2023 18:17

crevilla reviewed Jul 5, 2023

View reviewed changes

nvmkuruc force-pushed the stagecachemulti branch 5 times, most recently from efa27a7 to b1e2ba4 Compare July 10, 2023 22:25

crevilla reviewed Jul 10, 2023

View reviewed changes

nvmkuruc force-pushed the stagecachemulti branch from b1e2ba4 to 0bfc6cb Compare July 11, 2023 00:01

Replace boost::multi_index_container with custom container in `stag…

38a4985

…eCache.cpp`

nvmkuruc force-pushed the stagecachemulti branch from 0bfc6cb to 38a4985 Compare July 31, 2023 21:42

sunyab added the pending push label Aug 8, 2023

pixar-oss merged commit 64dbeb4 into PixarAnimationStudios:dev Aug 16, 2023

nvmkuruc deleted the stagecachemulti branch December 29, 2023 03:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `boost::multi_index_container` with custom container in `stageCache.cpp` #2349

Replace `boost::multi_index_container` with custom container in `stageCache.cpp` #2349

nvmkuruc commented Mar 21, 2023 •

edited

Loading

tallytalwar commented Mar 23, 2023

crevilla Jul 5, 2023

nvmkuruc Jul 5, 2023

crevilla Jul 5, 2023

crevilla Jul 5, 2023

crevilla Jul 5, 2023

nvmkuruc Jul 5, 2023 •

edited

Loading

crevilla Jul 5, 2023

crevilla Jul 5, 2023

nvmkuruc commented Jul 5, 2023

crevilla Jul 10, 2023 •

edited

Loading

nvmkuruc Jul 11, 2023

		@@ -143,15 +220,11 @@ struct DebugHelper

		bool IsEnabled() const { return _enabled; }

Replace boost::multi_index_container with custom container in stageCache.cpp #2349

Replace boost::multi_index_container with custom container in stageCache.cpp #2349

Conversation

nvmkuruc commented Mar 21, 2023 • edited Loading

Description of Change(s)

Fixes Issue(s)

tallytalwar commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvmkuruc Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nvmkuruc commented Jul 5, 2023

crevilla Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Replace `boost::multi_index_container` with custom container in `stageCache.cpp` #2349

Replace `boost::multi_index_container` with custom container in `stageCache.cpp` #2349

nvmkuruc commented Mar 21, 2023 •

edited

Loading

nvmkuruc Jul 5, 2023 •

edited

Loading

crevilla Jul 10, 2023 •

edited

Loading