buffer update opt: Initialize UboManager class #9331

show50726 · 2025-10-16T09:43:24Z

I'm uncertain about under what situation would the FenceStatus be Error, need some suggestions about how to handle Error status in checkFenceAndUnlockSlots.

Other notes:

~~Only partial functions have been completed, beginFrame and finishBeginFrame is still WIP~~ All of them have been done
Right now there's no unit test for UboManager, will try to figure out if there's a easier way to test the logic and add the tests later.

…)" This reverts commit 49c4a5d.

filament/src/details/UboManager.cpp

# Conflicts: # filament/src/details/MaterialInstance.cpp

filament/src/details/BufferAllocator.cpp

filament/src/details/MaterialInstance.cpp

pixelflinger · 2025-10-21T20:06:56Z

filament/src/details/UboManager.cpp

+
+    // Traverse all MIs and see which of them need slot allocation.
+    bool needReallocation =
+            updateMaterialInstanceAllocations(materialInstances, /*forceAllocateAll*/ false);


instead of the boolean parameter you could use a two state enum class{} with names are more clear about what action will be taken.

So maybe @pixelflinger can elaborate. But did you mean the "parameter" (i.e. forceAllocateAll) or the return value (i.e. allocationResult).

I changed both of the parameter and the return value to be enum

pixelflinger · 2025-10-21T20:08:43Z

filament/src/details/UboManager.cpp

+    if (!needReallocation) {
+        // No need to grow the buffer, so we can just map the buffer for writing and return.
+        mMmbHandle = driver.mapBuffer(mUbHandle, 0, mUboSize,
+            MapBufferAccessFlags::WRITE_BIT | MapBufferAccessFlags::INVALIDATE_RANGE_BIT,


the INVALIDATE_RANGE_BIT will make the whole mapped range invalid, meaning you'll have to update/write the whole range. Is it your intention here?

I misunderstood the usage of this flag. I think we don't need this flag here, thanks for the catch!

A little bit worried about the performance here, not sure if I remember correctly, but I read some articles about using it without this flag would lead to significant performance issue (The driver will assume you want to read the data)

I have another idea is that we cached the entire buffer content on the CPU side, so whenever we want to update the slots, we update the cached one. Then we can use INVALIDATE_RANGE_BIT when we're mapping. After all the updates are done and before unmapping, we copy the whole cached buffer into the mapped location. wdyt?

filament/src/details/UboManager.cpp

filament/src/details/UboManager.h

filament/src/details/UboManager.cpp

poweifeng · 2025-10-22T17:42:50Z

filament/src/details/UboManager.cpp

+
+    // Traverse all MIs and see which of them need slot allocation.
+    bool needReallocation =
+            updateMaterialInstanceAllocations(materialInstances, /*forceAllocateAll*/ false);


So maybe @pixelflinger can elaborate. But did you mean the "parameter" (i.e. forceAllocateAll) or the return value (i.e. allocationResult).

poweifeng · 2025-10-22T17:43:51Z

filament/src/details/UboManager.cpp

+    materialInstances.forEach([this, &driver](const FMaterialInstance* mi) {
+        const AllocationId allocationId = mi->getAllocationId();
+        assert_invariant(BufferAllocator::isValid(allocationId));
+        updateSlot(driver, allocationId, mi->getUniformBuffer().toBufferDescriptor(driver));


Do you need to update the allocationId of each material instance here?

The allocation id of each instance is updated in updateMaterialInstanceAllocations. At this point what we need to do is copy the data to the new location in the new UBO.

poweifeng · 2025-10-22T17:50:42Z

filament/src/details/UboManager.cpp

+        allocationIds.insert(id);
+    });
+
+    mFenceAllocationList.emplace_back( driver.createFence(), std::move(allocationIds) );


so if a reallocation happens between two frames. Will the ids of the first frame still be meaningful in the context of the allocator? Because line 168 will try to release it.

Wow that's a great point. Then I think we should destroy the existing fences during reallocation. Thanks!

filament/src/details/UboManager.h

poweifeng

lgtm with minor comments

poweifeng · 2025-10-28T16:04:07Z

filament/src/details/UboManager.cpp

+    // Migrate all MI data to the new allocated slots.
+    for (const auto& materialInstance : materialInstances) {
+        materialInstance.second.forEach([this, &driver](const FMaterialInstance* mi) {
+            if (!mi->isUsingUboBatching()) return;


nit: for consistency, here and elsewhere, bracket with next line for these early returns.

if (!mi->isUsingUboBatching()) { return; }

filament/src/details/UboManager.cpp

poweifeng · 2025-10-28T16:12:55Z

filament/src/details/UboManager.cpp

+                // For MIs whose parameters have been updated, aside from the slot it is being
+                // occupied by the GPU, we need to preserve an additional slot for it.


why is this?

Consider the scenario:

There are N material instances

All instances are updated every frame

The GPU has not finished the previous frame

The number of free slots is now insufficient for the N new updates

If we reallocate and only add 1 * uboSize for each of the N instances, we will be in the exact same situation on the very next frame. The buffer will be full again, and we'll be forced to reallocate every single frame.
To prevent this, when a material instance fails to find a slot due to Ubo update, I allocate twice its needed space.

This is also why I don't use an early break in the loop (what you ask in the other comment). I need to iterate through all material instances to get a total count of how many failed by update, so I can calculate the correct new buffer size (accommodating 2x space for all of them).

I guess it is possible that for some devices the latency is fairly high and 2 * uboSize is not enough. It is worth trying to use (number of pending fences + 1) in the future.

what if you just did something simple like always allocate max(oldSize * 1.5, newEstimatedRequiredSize). Thinking about frame delays and needed space here can get really complicated fast imo.
But my comment is more a of personal preference. This section is basically the heuristics part of the work. So I'm open to anything that's more memory efficient.

Thinking about frame delays and needed space here can get really complicated fast imo.

Yeah I agree...so I would prefer we still have *2 for those updating material instances as a buffer, and see how it works on the real apps and adjust it as needed.

show50726 added 15 commits October 15, 2025 23:41

Revert "buffer update opt: Add a flag to guard the feature (google#9322…

bd07311

…)" This reverts commit 49c4a5d.

Update feature flags

9f6e6c9

feedback

5e36f29

Update naming

b2f8665

introduce two types of ubo usages

a95321d

Use feature flags

cd8257f

feedback

daecf9e

Fix

58cf03e

Merge branch 'main' into dev/mi-changes

af7026e

add uboManager class

9b6bb81

Update

2bfadc9

Add unit tests

113e2bc

Add unit tests

ccf3d75

Adjust the order

c3ecf38

Update comments

3a0c371

show50726 added the internal Issue/PR does not affect clients label Oct 16, 2025

z3moon reviewed Oct 16, 2025

View reviewed changes

filament/src/details/UboManager.cpp Outdated Show resolved Hide resolved

filament/src/details/UboManager.cpp Outdated Show resolved Hide resolved

show50726 added 3 commits October 17, 2025 06:24

feedback

83e14f0

remove dependency to config

42c9e4a

Merge branch 'main' into dev/uboManager-no-test

416cddc

# Conflicts: # filament/src/details/MaterialInstance.cpp

show50726 changed the title ~~[dependes on #9324] buffer update opt: Initialize UboManager class and implement partial methods~~ buffer update opt: Initialize UboManager class and implement partial methods Oct 17, 2025

show50726 added 2 commits October 17, 2025 10:04

update

28feacc

Finish the implementation

f59433f

show50726 changed the title ~~buffer update opt: Initialize UboManager class and implement partial methods~~ buffer update opt: Initialize UboManager class Oct 17, 2025

Fix the build

bac5149

pixelflinger requested changes Oct 21, 2025

View reviewed changes

pixelflinger requested review from bejado and poweifeng October 21, 2025 20:13

show50726 added 2 commits October 22, 2025 13:38

feedback

9623dc2

feedback

1e1832d

show50726 added 6 commits October 22, 2025 14:41

specify types

33befbb

feedback

b541c80

enum status

86b33a0

Fix incorrect param

6a93495

add unit tests

73f4750

Merge branch 'main' into dev/uboManager-no-test

c3a6d4d

poweifeng reviewed Oct 22, 2025

View reviewed changes

show50726 requested a review from pixelflinger October 24, 2025 02:44

feedback

218e85c

show50726 force-pushed the dev/uboManager-no-test branch from 85e27e7 to 218e85c Compare October 28, 2025 12:39

Merge branch 'main' into dev/uboManager-no-test

c0632db

poweifeng approved these changes Oct 28, 2025

View reviewed changes

show50726 added 2 commits October 29, 2025 11:22

feedback

21c6e5b

Merge branch 'main' into dev/uboManager-no-test

2822441

show50726 mentioned this pull request Nov 3, 2025

[depends on #9331] buffer update opt: Integrate UboManager into the engine #9397

Open

		// For MIs whose parameters have been updated, aside from the slot it is being
		// occupied by the GPU, we need to preserve an additional slot for it.

buffer update opt: Initialize UboManager class #9331

Are you sure you want to change the base?

buffer update opt: Initialize UboManager class #9331

Uh oh!

Conversation

show50726 commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

poweifeng left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

show50726 commented Oct 16, 2025 •

edited

Loading

poweifeng left a comment •

edited

Loading