Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve runtime of GetScenePrimPaths from O(N log(N)) to O(N) #1822

Merged

Conversation

mtavenrath
Copy link
Contributor

Description of Change(s)

Replace the std::set & std::map in GetScenePrimPaths by a std::vector for better memory utilization & linear time access.

Fixes Issue(s)

  • Improved performance of GetScenePrimPaths

  • I have submitted a signed Contributor License Agreement

@mtavenrath mtavenrath changed the base branch from release to dev April 1, 2022 07:45
@mtavenrath mtavenrath changed the title Pxr getsceneprimpaths Improve runtime of GetScenePrimPaths from O(N log(N)) to O(N) Apr 1, 2022
@mtavenrath
Copy link
Contributor Author

@tcauchois Here is the follow up to #1744.

@tcauchois
Copy link
Contributor

cc @marktucker. Thanks Markus, we'll take a look!

@jilliene
Copy link

jilliene commented Apr 4, 2022

Filed as internal issue #USD-7304

Copy link
Contributor

@sunyab sunyab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mtavenrath, left a few review notes for you to take a look at. Thanks!

pxr/usdImaging/usdImaging/instanceAdapter.cpp Show resolved Hide resolved
// instance index.
return primPaths.size() != instanceIndices.size();

result[instanceIndices[instanceIdxShifted]] = adapter->_GetPrimPathFromInstancerChain(instanceChain);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Our coding conventions use a max column width of 80 characters. Could you please line-wrap this (and other lines below) to conform to that limit?

pxr/usdImaging/usdImaging/instanceAdapter.cpp Outdated Show resolved Hide resolved
for (size_t i = 0; i < remappedIndices.size(); i++)
result.push_back(primPathsFn.primPaths[remappedIndices[i]]);
if (!instanceIndices.empty()) {
int minIdx = INT_MAX;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use either INT_MAX or std::numeric_limits::max() consistently instead of using both?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed to std::numeric_limits::max()

// Indices in the map set to the maximum int value are not being referenced.
std::vector<int> requestedIndicesMap(maxIdx - minIdx + 1, std::numeric_limits<int>::max());

// set bits for all requested indices to truefor (size_t i = 0; i < instanceIndices.size(); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a mangled comment here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next line slipped in the comment. fixed

Comment on lines 2448 to 2462
if (instanceIdxShifted < 0)
return true;

// stop enumeration once the max index has been reached
if (size_t(instanceIdxShifted) >= instanceIndices.size())
return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Our code conventions require enclosing braces even around conditionals with one line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@pixar-oss pixar-oss merged commit e025f62 into PixarAnimationStudios:dev Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants