From 7a79302e4ea22241c02ae1c8d75a34cd5e8b9ccb Mon Sep 17 00:00:00 2001
From: Laszlo Agocs <laszlo.agocs@qt.io>
Date: Mon, 24 Apr 2023 13:21:54 +0200
Subject: [PATCH] rhi: Properly squeeze in trimOpLists
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

...and improve the comments since it is not directly obvious why
things are done the way they are.

Pick-to: 6.5
Change-Id: I6aa5f09b1b2f7f3fc18f74a4af2977d24278aae6
Reviewed-by: Christian Strømme <christian.stromme@qt.io>
---
 src/gui/rhi/qrhi.cpp | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/src/gui/rhi/qrhi.cpp b/src/gui/rhi/qrhi.cpp
index 53da22fbb3..516da50f88 100644
--- a/src/gui/rhi/qrhi.cpp
+++ b/src/gui/rhi/qrhi.cpp
@@ -6287,6 +6287,16 @@ void QRhiResourceUpdateBatch::generateMips(QRhiTexture *tex)
  */
 QRhiResourceUpdateBatch *QRhi::nextResourceUpdateBatch()
 {
+    // By default we prefer spreading out the utilization of the 64 batches as
+    // much as possible, meaning we won't pick the first one even if it's free,
+    // but prefer picking one after the last picked one. Relevant due to how
+    // QVLA and QRhiBufferData allocations behind the bufferOps are reused; in
+    // typical Qt Quick scenes this leads to a form of (eventually) seeding all
+    // the 64 resource batches with buffer operation data allocations which are
+    // then reused in subsequent frames. This comes at the expense of using
+    // more memory, but has proven good results when (CPU) profiling typical
+    // Quick/Quick3D apps.
+
     auto nextFreeBatch = [this]() -> QRhiResourceUpdateBatch * {
         auto isFree = [this](int i) -> QRhiResourceUpdateBatch * {
             const quint64 mask = 1ULL << quint64(i);
@@ -6337,7 +6347,15 @@ void QRhiResourceUpdateBatchPrivate::free()
     rhi->resUpdPoolMap &= ~mask;
     poolIndex = -1;
 
+    // textureOps is cleared, to not keep the potentially large image pixel
+    // data alive, but it is expected that the container keeps the list alloc
+    // at least. Only trimOpList() goes for the more aggressive route with squeeze.
     textureOps.clear();
+
+    // bufferOps is not touched, to allow reusing allocations (incl. in the
+    // elements' QRhiBufferData) as much as possible when this batch is used
+    // again in the future, which is important for performance, in particular
+    // with Qt Quick.
 }
 
 void QRhiResourceUpdateBatchPrivate::merge(QRhiResourceUpdateBatchPrivate *other)
@@ -6367,11 +6385,21 @@ void QRhiResourceUpdateBatchPrivate::trimOpLists()
 {
     Q_ASSERT(poolIndex == -1); // must not be in use
 
+    // Unlike free(), this is expected to aggressively deallocate all memory
+    // used by both the buffer and texture operation lists. (i.e. using
+    // squeeze() to only keep the stack prealloc of the QVLAs)
+    //
+    // This (e.g. just the destruction of bufferOps elements) may have a
+    // non-negligible performance impact e.g. with Qt Quick with scenes where
+    // there are lots of buffer operations per frame.
+
     activeBufferOpCount = 0;
     bufferOps.clear();
+    bufferOps.squeeze();
 
     activeTextureOpCount = 0;
     textureOps.clear();
+    textureOps.squeeze();
 }
 
 /*!