QQuickTableView: improve draining of reuse pool

It turns out that using a maxTime of 2 when draining
the pool was a bit naive. If e.g the width of the
table is greater than the height, it starts releasing
pooled items to quickly. So change the logic to be more
dynamic, and to calculate what the maxTime should be
based on the geometry of the table.

Change-Id: Ifeed62789575f98cff063f550f45eb54ef312fdb
Reviewed-by: Mitch Curtis <mitch.curtis@qt.io>

5 files changed, 152 insertions, 18 deletions

diff --git a/src/qml/types/qqmltableinstancemodel_p.h b/src/qml/types/qqmltableinstancemodel_p.h
index 71689ce6da..93ef4a697f 100644
--- a/src/qml/types/qqmltableinstancemodel_p.h
+++ b/src/qml/types/qqmltableinstancemodel_p.h
@@ -114,6 +114,7 @@ public:
     void insertIntoReusableItemsPool(QQmlDelegateModelItem *modelItem);
     QQmlDelegateModelItem *takeFromReusableItemsPool(const QQmlComponent *delegate);
     void drainReusableItemsPool(int maxPoolTime);
+    int poolSize() { return m_reusableItemsPool.size(); }
     void reuseItem(QQmlDelegateModelItem *item, int newModelIndex);
 
     QQmlIncubator::Status incubationStatus(int index) override;
diff --git a/src/quick/items/qquicktableview.cpp b/src/quick/items/qquicktableview.cpp
index 19e04b212b..0db886506e 100644
--- a/src/quick/items/qquicktableview.cpp
+++ b/src/quick/items/qquicktableview.cpp
@@ -61,16 +61,6 @@ Q_LOGGING_CATEGORY(lcTableViewDelegateLifecycle, "qt.quick.tableview.lifecycle")
 static const Qt::Edge allTableEdges[] = { Qt::LeftEdge, Qt::RightEdge, Qt::TopEdge, Qt::BottomEdge };
 static const int kBufferTimerInterval = 300;
 
-// Set the maximum life time of an item in the pool to be at least the number of
-// dimensions, which for a table is two. The reason is that the user might flick
-// both e.g the left column and the top row out before a new right column and bottom
-// row gets flicked in. This means we will end up with one column plus one row of
-// items in the pool. And flicking in a new column and a new row will typically happen
-// in separate updatePolish calls (unless you flick them both in at exactly the same
-// time). This means that we should allow flicked out items to stay in the pool for at least
-// two load cycles, to keep more items in circulation instead of deleting them prematurely.
-static const int kMaxPoolTime = 2;
-
 static QLine rectangleEdge(const QRect &rect, Qt::Edge tableEdge)
 {
     switch (tableEdge) {
@@ -868,13 +858,9 @@ void QQuickTableViewPrivate::processLoadRequest()
     updateContentHeight();
 
     loadRequest.markAsDone();
-    qCDebug(lcTableViewDelegateLifecycle()) << "request completed! Table:" << tableLayoutToString();
+    drainReusePoolAfterLoadRequest();
 
-    if (tableModel) {
-        // Whenever we're done loading a row or column, we drain the
-        // table models reuse pool of superfluous items that weren't reused.
-        tableModel->drainReusableItemsPool(kMaxPoolTime);
-    }
+    qCDebug(lcTableViewDelegateLifecycle()) << "request completed! Table:" << tableLayoutToString();
 }
 
 void QQuickTableViewPrivate::beginRebuildTable()
@@ -979,6 +965,50 @@ void QQuickTableViewPrivate::loadAndUnloadVisibleEdges()
 
 }
 
+void QQuickTableViewPrivate::drainReusePoolAfterLoadRequest()
+{
+    Q_Q(QQuickTableView);
+
+    if (reusableFlag == QQmlTableInstanceModel::NotReusable || !tableModel)
+        return;
+
+    if (q->verticalOvershoot() || q->horizontalOvershoot()) {
+        // Don't drain while we're overshooting, since this will fill up the
+        // pool, but we expect to reuse them all once the content item moves back.
+        return;
+    }
+
+    // When loading edges, we don't want to drain the reuse pool too aggressively. Normally,
+    // all the items in the pool are reused rapidly as the content view is flicked around
+    // anyway. Even if the table is temporarily flicked to a section that contains fewer
+    // cells than what used to be (e.g if the flicked-in rows are taller than average), it
+    // still makes sense to keep all the items in circulation; Chances are, that soon enough,
+    // thinner rows are flicked back in again (meaning that we can fit more items into the
+    // view). But at the same time, if a delegate chooser is in use, the pool might contain
+    // items created from different delegates. And some of those delegates might be used only
+    // occasionally. So to avoid situations where an item ends up in the pool for too long, we
+    // call drain after each load request, but with a sufficiently large pool time. (If an item
+    // in the pool has a large pool time, it means that it hasn't been reused for an equal
+    // amount of load cycles, and should be released).
+    //
+    // We calculate an appropriate pool time by figuring out what the minimum time must be to
+    // not disturb frequently reused items. Since the number of items in a row might be higher
+    // than in a column (or vice versa), the minimum pool time should take into account that
+    // you might be flicking out a single row (filling up the pool), before you continue
+    // flicking in several new columns (taking them out again, but now in smaller chunks). This
+    // will increase the number of load cycles items are kept in the pool (poolTime), but still,
+    // we shouldn't release them, as they are still being reused frequently.
+    // To get a flexible maxValue (that e.g tolerates rows and columns being flicked
+    // in with varying sizes, causing some items not to be resued immediately), we multiply the
+    // value by 2. Note that we also add an extra +1 to the column count, because the number of
+    // visible columns will fluctuate between +1/-1 while flicking.
+    const int w = loadedTable.width();
+    const int h = loadedTable.height();
+    const int minTime = std::ceil(w > h ? qreal(w + 1) / h : qreal(h + 1) / w);
+    const int maxTime = minTime * 2;
+    tableModel->drainReusableItemsPool(maxTime);
+}
+
 void QQuickTableViewPrivate::loadBuffer()
 {
     // Rather than making sure to stop the timer from all locations that can
@@ -1523,7 +1553,15 @@ QQuickTableViewAttached *QQuickTableView::qmlAttachedProperties(QObject *obj)
 
 void QQuickTableView::geometryChanged(const QRectF &newGeometry, const QRectF &oldGeometry)
 {
+    Q_D(QQuickTableView);
     QQuickFlickable::geometryChanged(newGeometry, oldGeometry);
+
+    if (d->tableModel) {
+        // When the view changes size, we force the pool to
+        // shrink by releasing all pooled items.
+        d->tableModel->drainReusableItemsPool(0);
+    }
+
     polish();
 }
 
diff --git a/src/quick/items/qquicktableview_p_p.h b/src/quick/items/qquicktableview_p_p.h
index b61093e55d..4f5a1acdd3 100644
--- a/src/quick/items/qquicktableview_p_p.h
+++ b/src/quick/items/qquicktableview_p_p.h
@@ -277,6 +277,7 @@ public:
     void loadEdge(Qt::Edge edge, QQmlIncubator::IncubationMode incubationMode);
     void unloadEdge(Qt::Edge edge);
     void loadAndUnloadVisibleEdges();
+    void drainReusePoolAfterLoadRequest();
     void cancelLoadRequest();
     void processLoadRequest();
     void beginRebuildTable();
diff --git a/tests/auto/quick/qquicktableview/data/countingtableview.qml b/tests/auto/quick/qquicktableview/data/countingtableview.qml
index c736d570b8..73fe4a80da 100644
--- a/tests/auto/quick/qquicktableview/data/countingtableview.qml
+++ b/tests/auto/quick/qquicktableview/data/countingtableview.qml
@@ -55,6 +55,9 @@ Item {
     // delegatesCreatedCount is the number of items created during the lifetime of the test
     property int delegatesCreatedCount: 0
 
+    property real delegateWidth: 100
+    property real delegateHeight: 50
+
     TableView {
         id: tableView
         width: 600
@@ -69,8 +72,8 @@ Item {
         id: tableViewDelegate
         Rectangle {
             objectName: "tableViewDelegate"
-            implicitWidth: 100
-            implicitHeight: 50
+            implicitWidth: delegateWidth
+            implicitHeight: delegateHeight
             color: "lightgray"
             border.width: 1
 
diff --git a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
index b4c672e5e2..7418189ee1 100644
--- a/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
+++ b/tests/auto/quick/qquicktableview/tst_qquicktableview.cpp
@@ -117,6 +117,7 @@ private slots:
     void dataChangedSignal();
     void checkIfDelegatesAreReused_data();
     void checkIfDelegatesAreReused();
+    void checkIfDelegatesAreReusedAsymmetricTableSize();
     void checkContextProperties_data();
     void checkContextProperties();
     void checkContextPropertiesQQmlListProperyModel_data();
@@ -1226,6 +1227,96 @@ void tst_QQuickTableView::checkIfDelegatesAreReused()
     }
 }
 
+void tst_QQuickTableView::checkIfDelegatesAreReusedAsymmetricTableSize()
+{
+    // Check that we end up reusing all delegate items while flicking, also if the table contain
+    // more columns than rows. In that case, if we flick out a whole row, we'll move a lot of
+    // items into the pool. And if we then start flicking in columns, we'll only reuse a few of
+    // them for each column. Still, we don't want the pool to release the superfluous items after
+    // each load, since they are still in circulation and will be needed once we flick in a new
+    // row at the end of the test.
+    LOAD_TABLEVIEW("countingtableview.qml");
+
+    const int pageFlickCount = 3;
+    const int columnCount = 20;
+    const int rowCount = 2;
+    const qreal delegateWidth = tableView->width() / columnCount;
+    const qreal delegateHeight = (tableView->height() / rowCount) + 10;
+
+    auto model = TestModelAsVariant(100, 100);
+    tableView->setModel(model);
+
+    // Let the height of each row be much bigger than the width of each column.
+    view->rootObject()->setProperty("delegateWidth", delegateWidth);
+    view->rootObject()->setProperty("delegateHeight", delegateHeight);
+
+    WAIT_UNTIL_POLISHED;
+
+    auto initialTopLeftItem = tableViewPrivate->loadedTableItem(QPoint(0, 0))->item;
+    QVERIFY(initialTopLeftItem);
+    int pooledCount = initialTopLeftItem->property("pooledCount").toInt();
+    int reusedCount = initialTopLeftItem->property("reusedCount").toInt();
+    QCOMPARE(pooledCount, 0);
+    QCOMPARE(reusedCount, 0);
+
+    // Flick half an item left+down, to force one extra row and column to load. By doing
+    // so, we force the maximum number of rows and columns to show before we start the test.
+    // This will make things less complicated below, when checking how many
+    // times the items have been reused (all items will then report the same number).
+    tableView->setContentX(delegateWidth * 0.5);
+    tableView->setContentY(delegateHeight * 0.5);
+    tableView->polish();
+
+    WAIT_UNTIL_POLISHED;
+
+    pooledCount = initialTopLeftItem->property("pooledCount").toInt();
+    reusedCount = initialTopLeftItem->property("reusedCount").toInt();
+    QCOMPARE(pooledCount, 0);
+    QCOMPARE(reusedCount, 0);
+    QCOMPARE(tableViewPrivate->tableModel->poolSize(), 0);
+
+    // Flick one row out of view. This will move one whole row of items into the
+    // pool without reusing them, since no new row is exposed at the bottom.
+    tableView->setContentY(delegateHeight + 1);
+    tableView->polish();
+
+    WAIT_UNTIL_POLISHED;
+
+    pooledCount = initialTopLeftItem->property("pooledCount").toInt();
+    reusedCount = initialTopLeftItem->property("reusedCount").toInt();
+    QCOMPARE(pooledCount, 1);
+    QCOMPARE(reusedCount, 0);
+    QCOMPARE(tableViewPrivate->tableModel->poolSize(), columnCount + 1);
+
+    const int visibleColumnCount = tableViewPrivate->loadedTable.width();
+    const int delegateCountAfterInit = view->rootObject()->property(kDelegatesCreatedCountProp).toInt();
+
+    // Start flicking in a lot of columns, and check that the created count stays the same
+    for (int column = 1; column <= (visibleColumnCount * pageFlickCount); ++column) {
+        tableView->setContentX((delegateWidth * column) + 1);
+        tableView->polish();
+
+        WAIT_UNTIL_POLISHED;
+
+        const int delegatesCreatedCount = view->rootObject()->property(kDelegatesCreatedCountProp).toInt();
+        QCOMPARE(delegatesCreatedCount, delegateCountAfterInit);
+    }
+
+    // Finally, flick one row back into view. The pool should still contain
+    // enough items so that we don't have to create any new ones.
+    tableView->setContentY(delegateHeight * 2);
+    tableView->polish();
+
+    WAIT_UNTIL_POLISHED;
+
+    const int delegatesCreatedCount = view->rootObject()->property(kDelegatesCreatedCountProp).toInt();
+    QCOMPARE(delegatesCreatedCount, delegateCountAfterInit);
+
+    // After we flicked in a new row, we now expect all items
+    // to be visible in the view, effectively making the pool empty.
+    QCOMPARE(tableViewPrivate->tableModel->poolSize(), 0);
+}
+
 void tst_QQuickTableView::checkContextProperties_data()
 {
     QTest::addColumn<QVariant>("model");