Vulkan: Dirty VertexArray binding bit if buffer storage change
In crrev.com/c/3669603, we did optimization for black_desert_mobile that
when vertex array is unbound, we remove vertex array from buffer's
observer list to reduce overhead of observer notifications when buffer
is been modified. To compensate for the lost notification, when vertex
array is bound, we always assume every buffer that is bound to vertex
array has been dirtied, for the simplicity at that time. This CL further
the optimization of that CL. In this CL, I moved the dirty bit set into
backend and improves vulkan backend by checking buffer's serial number
and only dirty the binding if the serial has changed. Given this, now we
can also remove all the non-current vertex array from buffer's observer
list (previously it is heuristic based with a hard coded observer count
limit). This and the previous CL improves asphalt_9 by ~1%.
Bug: b/277644512
Change-Id: Ibc3f8e3df9fe70c6879e0b2bca86d8487a9dba73
Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/4481241
Reviewed-by: Shahbaz Youssefi <syoussefi@chromium.org>
Reviewed-by: Yuxin Hu <yuxinhu@google.com>
Commit-Queue: Charlie Lao <cclao@google.com>
diff --git a/src/common/bitset_utils.h b/src/common/bitset_utils.h
index 2157a9c..2fba3a9 100644
--- a/src/common/bitset_utils.h
+++ b/src/common/bitset_utils.h
@@ -513,6 +513,7 @@
using param_type = BaseBitSet::param_type;
constexpr BitSetArray();
+ constexpr explicit BitSetArray(uint64_t value);
constexpr explicit BitSetArray(std::initializer_list<param_type> init);
constexpr BitSetArray(const BitSetArray<N> &other);
@@ -716,6 +717,31 @@
}
template <std::size_t N>
+constexpr BitSetArray<N>::BitSetArray(uint64_t value)
+{
+ reset();
+
+ if (priv::kDefaultBitSetSize < 64)
+ {
+ size_t i = 0;
+ for (; i < kArraySize - 1; ++i)
+ {
+ value_type elemValue =
+ value & priv::BaseBitSetType::Mask(priv::kDefaultBitSetSize).bits();
+ mBaseBitSetArray[i] = priv::BaseBitSetType(elemValue);
+ value >>= priv::kDefaultBitSetSize;
+ }
+ value_type elemValue = value & kLastElementMask;
+ mBaseBitSetArray[i] = priv::BaseBitSetType(elemValue);
+ }
+ else
+ {
+ value_type elemValue = value & priv::BaseBitSetType::Mask(priv::kDefaultBitSetSize).bits();
+ mBaseBitSetArray[0] = priv::BaseBitSetType(elemValue);
+ }
+}
+
+template <std::size_t N>
constexpr BitSetArray<N>::BitSetArray(std::initializer_list<param_type> init)
{
reset();
diff --git a/src/common/bitset_utils_unittest.cpp b/src/common/bitset_utils_unittest.cpp
index 8c4d0c1..24a4525 100644
--- a/src/common/bitset_utils_unittest.cpp
+++ b/src/common/bitset_utils_unittest.cpp
@@ -637,6 +637,24 @@
}
}
+// Test that BitSetArray's constructor with uint64_t.
+TYPED_TEST(BitSetArrayTest, ConstructorWithUInt64)
+{
+ uint64_t value = 0x5555555555555555;
+ TypeParam testBitSet(value);
+ for (size_t i = 0; i < testBitSet.size(); ++i)
+ {
+ if (i < sizeof(uint64_t) * 8 && (value & (0x1ull << i)))
+ {
+ EXPECT_TRUE(testBitSet.test(i));
+ }
+ else
+ {
+ EXPECT_FALSE(testBitSet.test(i));
+ }
+ }
+}
+
// Test iteration over BitSetArray where there are gaps
TYPED_TEST(BitSetArrayTest, IterationWithGaps)
{
diff --git a/src/libANGLE/VertexArray.cpp b/src/libANGLE/VertexArray.cpp
index 8f01252..bd5d1e8 100644
--- a/src/libANGLE/VertexArray.cpp
+++ b/src/libANGLE/VertexArray.cpp
@@ -19,8 +19,6 @@
{
namespace
{
-constexpr size_t kMaxObserverCountToTriggerUnobserve = 20;
-
bool IsElementArrayBufferSubjectIndex(angle::SubjectIndex subjectIndex)
{
return (subjectIndex == kElementArrayBufferIndex);
@@ -137,6 +135,12 @@
{
buffer->onNonTFBindingChanged(-1);
}
+ else
+ {
+ // un-assigning to avoid assertion, since it was already removed from buffer's observer
+ // list.
+ mArrayBufferObserverBindings[bindingIndex].assignSubject(nullptr);
+ }
// Note: the non-contents observer is unbound in the ObserverBinding destructor.
buffer->removeContentsObserver(this, static_cast<uint32_t>(bindingIndex));
binding.setBuffer(context, nullptr);
@@ -153,14 +157,6 @@
}
mState.mElementArrayBuffer.bind(context, nullptr);
- // If mDirtyObserverBindingBits is set, it means we have removed it from the buffer's observer
- // list. We should unassign subject to avoid assertion.
- for (size_t bindingIndex : mDirtyObserverBindingBits)
- {
- angle::ObserverBinding *observer = &mArrayBufferObserverBindings[bindingIndex];
- observer->assignSubject(nullptr);
- }
-
mVertexArray->destroy(context);
SafeDelete(mVertexArray);
delete this;
@@ -663,15 +659,10 @@
// This becomes current vertex array on the context
void VertexArray::onBind(const Context *context)
{
- if (mDirtyObserverBindingBits.none())
- {
- return;
- }
-
// This vertex array becoming current. Some of the bindings we may have removed from buffer's
// observer list. We need to add it back to the buffer's observer list and update dirty bits
// that we may have missed while we were not observing.
- for (size_t bindingIndex : mDirtyObserverBindingBits)
+ for (size_t bindingIndex : mState.getBufferBindingMask())
{
const VertexBinding &binding = mState.getVertexBindings()[bindingIndex];
Buffer *bufferGL = binding.getBuffer().get();
@@ -680,9 +671,6 @@
bufferGL->addObserver(&mArrayBufferObserverBindings[bindingIndex]);
updateCachedMappedArrayBuffersBinding(mState.mVertexBindings[bindingIndex]);
- // Assume both data and internal storage has been dirtied.
- mDirtyBits.set(DIRTY_BIT_BINDING_0 + bindingIndex);
-
if (mBufferAccessValidationEnabled)
{
for (size_t boundAttribute :
@@ -698,26 +686,23 @@
updateCachedTransformFeedbackBindingValidation(bindingIndex, bufferGL);
}
}
- mDirtyObserverBindingBits.reset();
+ mDirtyBits.set(DIRTY_BIT_LOST_OBSERVATION);
onStateChange(angle::SubjectMessage::ContentsChanged);
}
// This becomes non-current vertex array on the context
void VertexArray::onUnbind(const Context *context)
{
- // This vertex array becoming non-current. For performance reason, if there are too many
- // observers in the buffer, we remove it from the buffers' observer list so that the cost of
- // buffer sending signal to observers will not be too expensive.
+ // This vertex array becoming non-current. For performance reason, we remove it from the
+ // buffers' observer list so that the cost of buffer sending signal to observers will not be too
+ // expensive.
for (size_t bindingIndex : mState.mBufferBindingMask)
{
const VertexBinding &binding = mState.getVertexBindings()[bindingIndex];
Buffer *bufferGL = binding.getBuffer().get();
- if (bufferGL->getObserversCount() > kMaxObserverCountToTriggerUnobserve)
- {
- bufferGL->removeObserver(&mArrayBufferObserverBindings[bindingIndex]);
- mDirtyObserverBindingBits.set(bindingIndex);
- }
+ ASSERT(bufferGL != nullptr);
+ bufferGL->removeObserver(&mArrayBufferObserverBindings[bindingIndex]);
}
}
@@ -895,13 +880,15 @@
: mVertexArray(vertexArray)
{}
-void VertexArrayBufferContentsObservers::enableForBuffer(Buffer *buffer, uint32_t bufferIndex)
+void VertexArrayBufferContentsObservers::enableForBuffer(Buffer *buffer, uint32_t attribIndex)
{
- buffer->addContentsObserver(mVertexArray, bufferIndex);
+ buffer->addContentsObserver(mVertexArray, attribIndex);
+ mBufferObserversBitMask.set(attribIndex);
}
-void VertexArrayBufferContentsObservers::disableForBuffer(Buffer *buffer, uint32_t bufferIndex)
+void VertexArrayBufferContentsObservers::disableForBuffer(Buffer *buffer, uint32_t attribIndex)
{
- buffer->removeContentsObserver(mVertexArray, bufferIndex);
+ buffer->removeContentsObserver(mVertexArray, attribIndex);
+ mBufferObserversBitMask.reset(attribIndex);
}
} // namespace gl
diff --git a/src/libANGLE/VertexArray.h b/src/libANGLE/VertexArray.h
index 91bc500..70a62b6 100644
--- a/src/libANGLE/VertexArray.h
+++ b/src/libANGLE/VertexArray.h
@@ -121,9 +121,12 @@
VertexArrayBufferContentsObservers(VertexArray *vertexArray);
void enableForBuffer(Buffer *buffer, uint32_t bufferIndex);
void disableForBuffer(Buffer *buffer, uint32_t bufferIndex);
+ bool any() const { return mBufferObserversBitMask.any(); }
private:
VertexArray *mVertexArray;
+ // Bit is set when it is observing the buffer content change
+ gl::AttributesMask mBufferObserversBitMask;
};
class VertexArray final : public angle::ObserverInterface,
@@ -143,6 +146,10 @@
// the Vulkan back-end to skip performing a pipeline change for performance.
enum DirtyBitType
{
+ // This vertex array has lost buffer observation. Check against actual buffer storage is
+ // required.
+ DIRTY_BIT_LOST_OBSERVATION,
+
DIRTY_BIT_ELEMENT_ARRAY_BUFFER,
DIRTY_BIT_ELEMENT_ARRAY_BUFFER_DATA,
@@ -168,9 +175,11 @@
// bits when it processes dirtyBits. This assertion ensures these dirty bit order matches what
// VertexArrayVk::syncState expects.
static_assert(DIRTY_BIT_BINDING_0 < DIRTY_BIT_BUFFER_DATA_0,
- "BINDING dity bits should come before DATA.");
+ "BINDING dirty bits should come before DATA.");
static_assert(DIRTY_BIT_BUFFER_DATA_0 < DIRTY_BIT_ATTRIB_0,
- "DATA dity bits should come before ATTRIB.");
+ "DATA dirty bits should come before ATTRIB.");
+ static_assert(DIRTY_BIT_LOST_OBSERVATION < DIRTY_BIT_BINDING_0,
+ "LOST_OBSERVATION dirty bits should come before BINDING.");
enum DirtyAttribBitType
{
@@ -391,9 +400,6 @@
rx::VertexArrayImpl *mVertexArray;
std::vector<angle::ObserverBinding> mArrayBufferObserverBindings;
- // Track which observer in mArrayBufferObserverBindings is not currently been removed from
- // subject's observer list.
- DirtyObserverBindingBits mDirtyObserverBindingBits;
AttributesMask mCachedTransformFeedbackConflictedBindingsMask;
diff --git a/src/libANGLE/VertexArray_unittest.cpp b/src/libANGLE/VertexArray_unittest.cpp
index a3216cc..f8c254e 100644
--- a/src/libANGLE/VertexArray_unittest.cpp
+++ b/src/libANGLE/VertexArray_unittest.cpp
@@ -19,7 +19,7 @@
TEST(VertexArrayTest, VerifyGetIndexFromDirtyBit)
{
VertexArray::DirtyBits dirtyBits;
- constexpr size_t bits[] = {1, 4, 9, 16, 25, 35};
+ constexpr size_t bits[] = {2, 4, 9, 16, 25, 35};
constexpr GLint count = sizeof(bits) / sizeof(size_t);
for (GLint i = 0; i < count; i++)
{
diff --git a/src/libANGLE/renderer/d3d/d3d11/VertexArray11.cpp b/src/libANGLE/renderer/d3d/d3d11/VertexArray11.cpp
index a5f8b6a..2621c5e 100644
--- a/src/libANGLE/renderer/d3d/d3d11/VertexArray11.cpp
+++ b/src/libANGLE/renderer/d3d/d3d11/VertexArray11.cpp
@@ -83,10 +83,23 @@
gl::AttributesMask attributesToUpdate;
// Make sure we trigger re-translation for static index or vertex data.
- for (size_t dirtyBit : dirtyBits)
+ for (auto iter = dirtyBits.begin(), endIter = dirtyBits.end(); iter != endIter; ++iter)
{
+ size_t dirtyBit = *iter;
switch (dirtyBit)
{
+ case gl::VertexArray::DIRTY_BIT_LOST_OBSERVATION:
+ {
+ // If vertex array was not observing while unbound, we need to check buffer's
+ // internal storage and take action if buffer has changed while not observing.
+ // For now we just simply assume buffer storage has changed and always dirty all
+ // binding points.
+ iter.setLaterBits(
+ gl::VertexArray::DirtyBits(mState.getBufferBindingMask().to_ulong()
+ << gl::VertexArray::DIRTY_BIT_BINDING_0));
+ break;
+ }
+
case gl::VertexArray::DIRTY_BIT_ELEMENT_ARRAY_BUFFER:
case gl::VertexArray::DIRTY_BIT_ELEMENT_ARRAY_BUFFER_DATA:
{
diff --git a/src/libANGLE/renderer/gl/VertexArrayGL.cpp b/src/libANGLE/renderer/gl/VertexArrayGL.cpp
index dc981de..c76cfa6 100644
--- a/src/libANGLE/renderer/gl/VertexArrayGL.cpp
+++ b/src/libANGLE/renderer/gl/VertexArrayGL.cpp
@@ -951,10 +951,23 @@
StateManagerGL *stateManager = GetStateManagerGL(context);
stateManager->bindVertexArray(mVertexArrayID, mNativeState);
- for (size_t dirtyBit : dirtyBits)
+ for (auto iter = dirtyBits.begin(), endIter = dirtyBits.end(); iter != endIter; ++iter)
{
+ size_t dirtyBit = *iter;
switch (dirtyBit)
{
+ case gl::VertexArray::DIRTY_BIT_LOST_OBSERVATION:
+ {
+ // If vertex array was not observing while unbound, we need to check buffer's
+ // internal storage and take action if buffer has changed while not observing.
+ // For now we just simply assume buffer storage has changed and always dirty all
+ // binding points.
+ iter.setLaterBits(
+ gl::VertexArray::DirtyBits(mState.getBufferBindingMask().to_ulong()
+ << gl::VertexArray::DIRTY_BIT_BINDING_0));
+ break;
+ }
+
case VertexArray::DIRTY_BIT_ELEMENT_ARRAY_BUFFER:
ANGLE_TRY(updateElementArrayBufferBinding(context));
break;
diff --git a/src/libANGLE/renderer/metal/VertexArrayMtl.mm b/src/libANGLE/renderer/metal/VertexArrayMtl.mm
index 30f2faa..e6b5841 100644
--- a/src/libANGLE/renderer/metal/VertexArrayMtl.mm
+++ b/src/libANGLE/renderer/metal/VertexArrayMtl.mm
@@ -266,10 +266,23 @@
const std::vector<gl::VertexAttribute> &attribs = mState.getVertexAttributes();
const std::vector<gl::VertexBinding> &bindings = mState.getVertexBindings();
- for (size_t dirtyBit : dirtyBits)
+ for (auto iter = dirtyBits.begin(), endIter = dirtyBits.end(); iter != endIter; ++iter)
{
+ size_t dirtyBit = *iter;
switch (dirtyBit)
{
+ case gl::VertexArray::DIRTY_BIT_LOST_OBSERVATION:
+ {
+ // If vertex array was not observing while unbound, we need to check buffer's
+ // internal storage and take action if buffer has changed while not observing.
+ // For now we just simply assume buffer storage has changed and always dirty all
+ // binding points.
+ iter.setLaterBits(
+ gl::VertexArray::DirtyBits(mState.getBufferBindingMask().to_ulong()
+ << gl::VertexArray::DIRTY_BIT_BINDING_0));
+ break;
+ }
+
case gl::VertexArray::DIRTY_BIT_ELEMENT_ARRAY_BUFFER:
case gl::VertexArray::DIRTY_BIT_ELEMENT_ARRAY_BUFFER_DATA:
{
diff --git a/src/libANGLE/renderer/vulkan/BufferVk.h b/src/libANGLE/renderer/vulkan/BufferVk.h
index ac14c68..1afb85e 100644
--- a/src/libANGLE/renderer/vulkan/BufferVk.h
+++ b/src/libANGLE/renderer/vulkan/BufferVk.h
@@ -119,6 +119,8 @@
return mBuffer;
}
+ vk::BufferSerial getBufferSerial() { return mBuffer.getBufferSerial(); }
+
bool isBufferValid() const { return mBuffer.valid(); }
bool isCurrentlyInUse(RendererVk *renderer) const;
diff --git a/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp b/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
index 5036d9a..08570f3 100644
--- a/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
+++ b/src/libANGLE/renderer/vulkan/VertexArrayVk.cpp
@@ -487,7 +487,8 @@
numVertices, binding.getStride(),
vertexFormat.getVertexLoadFunction(compressed)));
ANGLE_TRY(srcBuffer->unmapImpl(contextVk));
- mCurrentArrayBuffers[attribIndex] = dstBufferHelper;
+ mCurrentArrayBuffers[attribIndex] = dstBufferHelper;
+ mCurrentArrayBufferSerial[attribIndex] = dstBufferHelper->getBufferSerial();
ASSERT(conversion->dirty);
conversion->dirty = false;
@@ -522,6 +523,43 @@
size_t dirtyBit = *iter;
switch (dirtyBit)
{
+ case gl::VertexArray::DIRTY_BIT_LOST_OBSERVATION:
+ {
+ // If vertex array was not observing while unbound, we need to check buffer's
+ // internal storage and take action if buffer storage has changed while not
+ // observing.
+ if (contextVk->getRenderer()->getFeatures().compressVertexData.enabled ||
+ mContentsObservers->any())
+ {
+ // We may have lost buffer content change when it became non-current. In that
+ // case we always assume buffer has changed. If compressVertexData.enabled is
+ // true, it also depends on buffer usage which may have changed.
+ iter.setLaterBits(
+ gl::VertexArray::DirtyBits(mState.getBufferBindingMask().to_ulong()
+ << gl::VertexArray::DIRTY_BIT_BINDING_0));
+ }
+ else
+ {
+ for (size_t bindingIndex : mState.getBufferBindingMask())
+ {
+ const gl::Buffer *bufferGL = bindings[bindingIndex].getBuffer().get();
+ vk::BufferSerial bufferSerial = vk::GetImpl(bufferGL)->getBufferSerial();
+ for (size_t attribIndex : bindings[bindingIndex].getBoundAttributesMask())
+ {
+ if (attribs[attribIndex].enabled &&
+ (!bufferSerial.valid() ||
+ bufferSerial != mCurrentArrayBufferSerial[attribIndex]))
+ {
+ iter.setLaterBit(gl::VertexArray::DIRTY_BIT_BINDING_0 +
+ bindingIndex);
+ break;
+ }
+ }
+ }
+ }
+ break;
+ }
+
case gl::VertexArray::DIRTY_BIT_ELEMENT_ARRAY_BUFFER:
case gl::VertexArray::DIRTY_BIT_ELEMENT_ARRAY_BUFFER_DATA:
{
@@ -719,8 +757,9 @@
bufferOnly = false;
}
- vk::BufferHelper *bufferHelper = conversion->data.get();
- mCurrentArrayBuffers[attribIndex] = bufferHelper;
+ vk::BufferHelper *bufferHelper = conversion->data.get();
+ mCurrentArrayBuffers[attribIndex] = bufferHelper;
+ mCurrentArrayBufferSerial[attribIndex] = bufferHelper->getBufferSerial();
VkDeviceSize bufferOffset;
mCurrentArrayBufferHandles[attribIndex] =
bufferHelper
@@ -741,14 +780,16 @@
vk::BufferHelper &emptyBuffer = contextVk->getEmptyBuffer();
mCurrentArrayBuffers[attribIndex] = &emptyBuffer;
+ mCurrentArrayBufferSerial[attribIndex] = emptyBuffer.getBufferSerial();
mCurrentArrayBufferHandles[attribIndex] = emptyBuffer.getBuffer().getHandle();
mCurrentArrayBufferOffsets[attribIndex] = emptyBuffer.getOffset();
mCurrentArrayBufferStrides[attribIndex] = 0;
}
else
{
- vk::BufferHelper &bufferHelper = bufferVk->getBuffer();
- mCurrentArrayBuffers[attribIndex] = &bufferHelper;
+ vk::BufferHelper &bufferHelper = bufferVk->getBuffer();
+ mCurrentArrayBuffers[attribIndex] = &bufferHelper;
+ mCurrentArrayBufferSerial[attribIndex] = bufferHelper.getBufferSerial();
VkDeviceSize bufferOffset;
mCurrentArrayBufferHandles[attribIndex] =
bufferHelper
@@ -770,6 +811,7 @@
{
vk::BufferHelper &emptyBuffer = contextVk->getEmptyBuffer();
mCurrentArrayBuffers[attribIndex] = &emptyBuffer;
+ mCurrentArrayBufferSerial[attribIndex] = emptyBuffer.getBufferSerial();
mCurrentArrayBufferHandles[attribIndex] = emptyBuffer.getBuffer().getHandle();
mCurrentArrayBufferOffsets[attribIndex] = emptyBuffer.getOffset();
// Client side buffer will be transfered to a tightly packed buffer later
@@ -800,6 +842,7 @@
// These will be filled out by the ContextVk.
vk::BufferHelper &emptyBuffer = contextVk->getEmptyBuffer();
mCurrentArrayBuffers[attribIndex] = &emptyBuffer;
+ mCurrentArrayBufferSerial[attribIndex] = emptyBuffer.getBufferSerial();
mCurrentArrayBufferHandles[attribIndex] = emptyBuffer.getBuffer().getHandle();
mCurrentArrayBufferOffsets[attribIndex] = emptyBuffer.getOffset();
mCurrentArrayBufferStrides[attribIndex] = 0;
@@ -947,7 +990,8 @@
vertexFormat.getVertexLoadFunction(compressed)));
}
- mCurrentArrayBuffers[attribIndex] = vertexDataBuffer;
+ mCurrentArrayBuffers[attribIndex] = vertexDataBuffer;
+ mCurrentArrayBufferSerial[attribIndex] = vertexDataBuffer->getBufferSerial();
VkDeviceSize bufferOffset;
mCurrentArrayBufferHandles[attribIndex] =
vertexDataBuffer
@@ -1039,6 +1083,7 @@
.getHandle();
mCurrentArrayBufferOffsets[attribIndex] = bufferOffset;
mCurrentArrayBuffers[attribIndex] = bufferHelper;
+ mCurrentArrayBufferSerial[attribIndex] = bufferHelper->getBufferSerial();
mCurrentArrayBufferStrides[attribIndex] = 0;
ANGLE_TRY(setDefaultPackedInput(contextVk, attribIndex,
diff --git a/src/libANGLE/renderer/vulkan/VertexArrayVk.h b/src/libANGLE/renderer/vulkan/VertexArrayVk.h
index b9870d3..7438664 100644
--- a/src/libANGLE/renderer/vulkan/VertexArrayVk.h
+++ b/src/libANGLE/renderer/vulkan/VertexArrayVk.h
@@ -150,6 +150,8 @@
// The offset into the buffer to the first attrib
gl::AttribArray<GLuint> mCurrentArrayBufferRelativeOffsets;
gl::AttribArray<vk::BufferHelper *> mCurrentArrayBuffers;
+ // Tracks BufferSerial of mCurrentArrayBuffers since they are always valid to access.
+ gl::AttribArray<vk::BufferSerial> mCurrentArrayBufferSerial;
// Cache strides of attributes for a fast pipeline cache update when VAOs are changed
gl::AttribArray<angle::FormatID> mCurrentArrayBufferFormats;
gl::AttribArray<GLuint> mCurrentArrayBufferStrides;
