Skip to content

Various optimizations and cleanup in the render delegate#2633

Open
sebastienblor wants to merge 1 commit into
Autodesk:masterfrom
sebastienblor:optims_110526
Open

Various optimizations and cleanup in the render delegate#2633
sebastienblor wants to merge 1 commit into
Autodesk:masterfrom
sebastienblor:optims_110526

Conversation

@sebastienblor
Copy link
Copy Markdown
Collaborator

Changes proposed in this pull request
The libs/render_delegate/ code has accumulated several correctness bugs and performance inefficiencies. This plan addresses them in priority order: safety first, then hot-path performance, then modern C++ hygiene.

1. Exception-unsafe manual locking in render_buffer.cpp

Map() calls _mutex.lock() then returns early calling _mutex.unlock() manually. Not exception-safe; also, Unmap() conditionally skips unlock() if buffer is empty, creating potential deadlock.

2. Potential null dereference in shared_arrays.h

unboxed.values[0] accessed without checking unboxed.count > 0.

3. Various string optimizations

Several optims on string allocations in render_delegate.cpp , render_settings.cpp, volume.cpp

4. O(n²) light linking in render_delegate.cpp and O(n²) duplicate detection in volume.cpp

  • std::find() on std::vector<HdLight*> per registration → O(n) per call → O(n²) total for n lights.
  • std::find_if on fields vector per field entry.

5. Use smart pointers to avoid manual new / delete where the destructor must remember to delete

  • _imagingDelegate raw pointer in the reader is changing member to std::unique_ptr<UsdArnoldProcImagingDelegate>, using make_unique at construction.
  • _volumes raw pointer vector in volume.h
    std::vector<HdArnoldShape*> managed with scattered delete calls.
  • reset(new X()) patterns in render_delegate.cpp are replaced with std::make_shared<HdResourceRegistry>()

6. Remove vestigial TF_UNUSED in openvdb_asset.

renderDelegate IS used

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR targets correctness and hot-path performance improvements in libs/render_delegate/, primarily around render buffer thread-safety, array handling safety, and reducing avoidable O(n²)/allocation-heavy patterns.

Changes:

  • Improves render buffer locking in HdArnoldRenderBuffer::Map/Unmap to avoid manual lock/unlock patterns.
  • Replaces linear duplicate checks with hash-based sets for volume field and light-linking registration.
  • Reduces transient string allocations in render settings by pre-reserving and appending.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
libs/render_delegate/volume.cpp Switches volume-field duplicate detection to per-path hash sets and minor string handling cleanup.
libs/render_delegate/shared_arrays.h Adds guards before accessing the first unboxed sample to avoid unsafe access on empty/unboxed data.
libs/render_delegate/render_settings.cpp Reduces string reallocations when building derived node names (filter/user_data/aov shader names).
libs/render_delegate/render_delegate.h Changes light-linking storage from vector to unordered_set to avoid O(n) duplicate checks.
libs/render_delegate/render_delegate.cpp Updates resource registry/render param construction and light-linking registration to match new container choices; minor string handling optimization.
libs/render_delegate/render_buffer.cpp Refactors Map/Unmap locking toward RAII and lock ownership transfer.
libs/render_delegate/openvdb_asset.cpp Removes vestigial TF_UNUSED(renderDelegate) since the delegate is stored/used.
libs/render_delegate/instancer.cpp Avoids repeated per-sample reserve() by reserving once at the maximum expected instance count.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +229 to 242
std::unique_lock<std::mutex> lock(_mutex);
if (_buffer.empty()) {
_mutex.unlock();
return nullptr;
}
lock.release(); // ownership transferred to Unmap()
return _buffer.data();
}

void HdArnoldRenderBuffer::Unmap()
{
if (!_buffer.empty()) {
_mutex.unlock();
// The mutex was acquired (and ownership released) by Map(); adopt it here so it is unlocked via RAII.
std::unique_lock<std::mutex> lock(_mutex, std::adopt_lock);
}
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.

2 participants