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

MAYA106075 - Introducing color cache to improve performance #2059

Merged
merged 2 commits into from
Feb 2, 2022

Conversation

vlasovi
Copy link
Collaborator

@vlasovi vlasovi commented Feb 2, 2022

Introducing color cache to improve performance of ProxyRenderDelegate::GetSelectionHighlightColor and ProxyRenderDelegate::GetCurveDefaultColor.
Make sure the cache is accessed from inside the mutex to prevent races.

…::GetSelectionHighlightColor and ProxyRenderDelegate::GetCurveDefaultColor
williamkrick
williamkrick previously approved these changes Feb 2, 2022
Copy link
Contributor

@williamkrick williamkrick left a comment

Choose a reason for hiding this comment

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

Did you test the performance of the changes?

curveColorResult);
// Enter the mutex and check the cache
std::lock_guard<std::mutex> mutexGuard(_mayaCommandEngineMutex);
if (_dormantCurveColorCache.second == _frameCounter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we make _dormantCurveColorCache.second a std::atomic<uint64_t> then we could move acquiring the mutex to be after this check to see if we already have already read the value this frame. That would prevent lots of mutex acquires which might be faster.

In the case where the test fails we'd have to acquire the mutex and re-test to avoid reading the value two or more times, but that is okay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The std::atomic<uint64_t> approach may be faster but not necessarily. We can try it if we still need to after this change.

// Enter the mutex and check the cache
std::lock_guard<std::mutex> mutexGuard(_mayaCommandEngineMutex);
if (colorCache->second == _frameCounter) {
return colorCache->first;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use a similar lock-free pattern here.

@vlasovi
Copy link
Collaborator Author

vlasovi commented Feb 2, 2022

I haven't tested the performance on a large scene. I have none at the moment. I only tested my custom scenes and confirmed that the feature is working as expected.

@vlasovi vlasovi added the ready-for-merge Development process is finished, PR is ready for merge label Feb 2, 2022
@kxl-adsk kxl-adsk merged commit 124b037 into dev Feb 2, 2022
@kxl-adsk kxl-adsk deleted the vlasovi/MAYA106075-perf branch February 2, 2022 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants