Skip to content

HYDRA-2417 - Optimize MarkPrimDirty - #479

Merged
lilike-adsk merged 7 commits into
devfrom
vlasovi/HYDRA-2417
Aug 10, 2026
Merged

HYDRA-2417 - Optimize MarkPrimDirty#479
lilike-adsk merged 7 commits into
devfrom
vlasovi/HYDRA-2417

Conversation

@vlasovi

@vlasovi vlasovi commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator
  1. Introduce batching for multiple MarkPrimDirty calls.
  2. Optimize _PrimsDirtied method on the scene indices, specifically targeted at animation updates.

lanierd-adsk
lanierd-adsk previously approved these changes Jul 30, 2026
@vlasovi vlasovi self-assigned this Jul 30, 2026
ppt-adsk
ppt-adsk previously approved these changes Jul 30, 2026

@ppt-adsk ppt-adsk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work! Just the guard object suggestion I have.

}

// Start batching DirtyPrims notifications produced per render item.
Fvp::DirtyNotifier::beginDirtyBatch(*this);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Would be (very) nice to have a guard object for this, RAII-style. Prevents coding mistakes, is exception-safe, and you could remove the addition on line 569.

if (_sceneIndex) {

if (_sBatchingSceneIndex) {
_sPendingDirtyEntries.push_back({ _primPath, _locators });

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok, so, the original flush/batch is at per render item level, that didn't give obvious performance improvement, now, this bigger flush/batch on all render items level should be better :) .

One suggestion: this static _sPendingDirtyEntries is a bit hard to understand and we're actually not flushing,
Also it's not RAII style as @ppt-adsk mentioned, here's a suggested version from AI:

Collapse both levels of flush/batch into one flush(). Then there is exactly one DirtyPrims() call site in the codebase, and one flush().

class DirtyBatch { // owns target SI + all pending entries
public:
FVP_API explicit DirtyBatch(PXR_NS::HdRetainedSceneIndex& si);
FVP_API ~DirtyBatch(); // flush()
FVP_API void flush(); // the ONLY DirtyPrims() call, anywhere
private:
friend class DirtyNotifier;
PXR_NS::TfWeakPtr<PXR_NS::HdRetainedSceneIndex> _sceneIndex;
PXR_NS::HdSceneIndexObserver::DirtiedPrimEntries _entries;
};

class DirtyNotifier { // cursor, no _locators member
public:
DirtyNotifier(HdRetainedSceneIndex& si, const SdfPath& p); // owns a private batch-of-one
DirtyNotifier(DirtyBatch& batch, const SdfPath& p); // writes into caller's batch
// dirty*() append straight into the batch row; no flush(), no dtor logic
};

dirty*() writes through to the row, so DirtyNotifier::flush() disappears — which is literally the "one flush interface" you're after.

What makes this cheap: the batched region has exactly one emission site

I checked the call graph under beginDirtyBatch. MayaHydraRenderItemAdapter::UpdateFromDelta constructs a single notifier (renderItemAdapter.cpp:239) and accumulates the whole frame's worth of locators into it; UpdateTransform (:131) emits nothing; Populate/CreateMaterial/SetMaterial don't create notifiers (the Fvp::DirtyNotifier(*this, rprimId) at mayaHydraSceneIndex.cpp:1345 is in a DG node-removal handler, not the loop). And _EmitRenderItemTopologyDirtyLocators (:74-77) already takes Fvp::DirtyNotifier& by reference, so passing the accumulator down is the established pattern in this file.

So plumbing is one signature + one call site, not thirty:

Fvp::DirtyBatch batch(*this); // RAII, before the loop
...
ria->UpdateFromDelta(data, batch); // was: UpdateFromDelta(data)
MayaHydra::DirtyNotifier notifier(this, batch); // was: DirtyNotifier notifier(this)

Everything else in the tree keeps the two-arg ctor and behaves bit-identically to today.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@lilike-adsk I'm not sure I totally understand your plan, but it looks like a lot of refactoring. I think I can easily add RAII to the batching. As for the rest, maybe we can create a separate ticket for this work. Also, it's not actually the batching itself that gives the performance optimization, but all the changes in the specific scene indices working on top of batching.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm ok with that.

@vlasovi
vlasovi dismissed stale reviews from ppt-adsk and lanierd-adsk via 04dea44 July 30, 2026 18:27
@vlasovi vlasovi assigned vlasovi and unassigned vlasovi Jul 30, 2026
@vlasovi
vlasovi requested review from lilike-adsk and ppt-adsk July 30, 2026 18:37

@lilike-adsk lilike-adsk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good

@vlasovi vlasovi assigned vlasovi and unassigned vlasovi Aug 3, 2026
@vlasovi vlasovi added the ready-for-merge Development process is finished, PR is ready for merge label Aug 10, 2026
@lilike-adsk
lilike-adsk merged commit 04be8da into dev Aug 10, 2026
11 checks passed
@lilike-adsk
lilike-adsk deleted the vlasovi/HYDRA-2417 branch August 10, 2026 13:30
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.

5 participants