Fix some perf and cmake

[NathanaelAtEGR]

I have a few changes to propose but they have been just small stuff except for the last one:

The BOOST_NO_CXX98_FUNCTION_BASE is from: PixarAnimationStudios/OpenUSD#2634

ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} is so that MSVC INSTALL has a place for the *.lib file.

It might be my USD Build is not free of Python, but when I try to build OpenUSD without Python dependencies it still adds Boost::Boost, which is then required to be present for CMake to generate the build.

The perf issues are a big one and the main reason for this pull request.
We automate some stuff and generating Notices of over 20k Changes takes about 30 to 40 seconds. The followup Broker::EndTransaction(), or more specifically ObjectsChanged::Merge(), took 6 to 7 more minutes. Most of it wasted by an erase in the middle of a vector and ca. 25% lost on converting a Vector into a Set for every Merge (MSVC Release and RelDebug Builds). With these proposed changes the Broker::EndTransaction() of the same example goes down to 1 maybe 1.5 seconds.

[davvid]

This is something we aim to improve in the cmake packaging for usd.

Longer-term, we should switch to using find_package(pxr), we should use pxrConfig.cmake as shipped by usd, the cmake targets should be moved into a pxr:: cmake namespace, and it should be charge of configuring any of this Boost stuff.

Even longer term (or perhaps during that same effort) we should make it so that usd/pxr ships a pxr.cps cmake package file instead of its cmake-specific configuration.

This is fine given the current state of the world, though. Thanks for the patch!

[buddly27]

Boost is currently being pulled from our custom USD module, as we still can’t rely on the official CMake config provided by OpenUSD internally.

unf/cmake/modules/FindUSD.cmake

Line 88 in 137dbae

list(APPEND USD_DEPENDENCIES "Boost::boost")

At one point, Boost was also required outside the Python bindings, but that’s no longer the case, the dependency just never got removed. I’ll open a new MR to clean that up, along with formatting issues.

[buddly27]

I fixed it on this PR: #33

And I added new tests to ensure it worked as expected when built without Python bindings.

[davvid]

TODO for later ~ it would be nice to refactor the code so that we can do this without a goto. Maybe the rest of this function can be split apart into an inlineable helper function in an anonymous namespace and then we can call it from two places instead of doing the goto.

[NathanaelAtEGR]

If the goto poses an issue, we could use a similar structure to the one I replaced:

        // Skip if an ancestor of the path is already in resyncedPaths.
        bool ancestorResynced = false;
        for (const auto& ancestor : primPath.GetPrefixes()) {
            if (std::find(_resyncChanges.begin(), _resyncChanges.end(), ancestor) != _resyncChanges.end()) {
                ancestorResynced = true;
                break;
            }
        }
        if (ancestorResynced) {
            continue;
        }

I personally prefer to avoid this kind of logic, but I have seen no impact on perf. Alternatively we can go with a Lambda function:

        // Skip if an ancestor of the path is already in resyncedPaths.
        const auto ancestorResyncedFn = [&]() -> bool {
            for (const auto& ancestor : primPath.GetPrefixes()) {
                const auto it = std::find(
                    _resyncChanges.begin(), _resyncChanges.end(), ancestor);
                if (it != _resyncChanges.end()) {
                    return true;
                }
            }
            return false;
        };
        if (ancestorResyncedFn()) continue;

[davvid]

Thanks for this and the other major perf improvements.