Fix resource leaks in ArrayAllocatingPolicy.

Previously we ended up with default constructed elements in the bucket,
and were constructing new ones for every allocation. As some of the types
used in here allocate resources in their ctors themselves we were leaking
those resources.

Now we only construct each element at most once, and we don't construct
any element that is never used. The resulting re-use of elements exposed
the missing cleanup method in tst_ArrayResource.

Change-Id: Ie875fea3ebc9b6f96d0239a6064474fbea798814
Reviewed-by: Sean Harmer <sean.harmer@kdab.com>

1 files changed, 43 insertions, 18 deletions

diff --git a/src/core/resources/qresourcemanager_p.h b/src/core/resources/qresourcemanager_p.h
index 47f067409..b1207d5e2 100644
--- a/src/core/resources/qresourcemanager_p.h
+++ b/src/core/resources/qresourcemanager_p.h
@@ -152,34 +152,46 @@ class ArrayAllocatingPolicy
 {
 public:
     ArrayAllocatingPolicy()
+        : m_numBuckets(0)
+        , m_numConstructed(0)
     {
         reset();
     }
 
+    ~ArrayAllocatingPolicy()
+    {
+        deallocateBuckets();
+    }
+
     T* allocateResource()
     {
         Q_ASSERT(!m_freeList.isEmpty());
-        int idx = m_freeList.last();
-        m_freeList.pop_back();
+        int idx = m_freeList.takeLast();
         int bucketIdx = idx / BucketSize;
         int localIdx = idx % BucketSize;
-        Q_ASSERT(bucketIdx <= m_buckets.size());
-        if (bucketIdx == m_buckets.size()) {
-            m_buckets.append(Bucket(BucketSize));
-            m_bucketDataPtrs[bucketIdx] = m_buckets.last().data();
+        Q_ASSERT(bucketIdx <= m_numBuckets);
+        if (bucketIdx == m_numBuckets) {
+            m_bucketDataPtrs[bucketIdx] = static_cast<T*>(malloc(sizeof(T) * BucketSize));
+            // ### memset is only needed as long as we also use this for primitive types (see FrameGraphManager)
+            // ### remove once this is fixed, add a static_assert on T instead
+            memset(m_bucketDataPtrs[bucketIdx], 0, sizeof(T) * BucketSize);
+            ++m_numBuckets;
+        }
+
+        Q_ASSERT(idx <= m_numConstructed);
+        if (idx == m_numConstructed) {
+            new (m_bucketDataPtrs[bucketIdx] + localIdx) T;
+            ++m_numConstructed;
         }
-        // Make sure we got a properly constructed object at that point
-        // otherwise we might end up passing around bogus vtable pointers
-        // (shows up when we start releasing resources)
-        return new (m_bucketDataPtrs[bucketIdx] + localIdx) T;
+
+        return m_bucketDataPtrs[bucketIdx] + localIdx;
     }
 
     void releaseResource(T *r)
     {
         // search linearly over buckets to find the index of the resource
         // and put it into the free list
-        const int numBuckets = m_buckets.size();
-        for (int bucketIdx = 0; bucketIdx < numBuckets; ++bucketIdx) {
+        for (int bucketIdx = 0; bucketIdx < m_numBuckets; ++bucketIdx) {
             const T* firstItem = m_bucketDataPtrs[bucketIdx];
             if (firstItem > r || r >= firstItem + BucketSize) {
                 // resource is not in this bucket when its pointer address
@@ -200,13 +212,14 @@ public:
 
     void reset()
     {
-        m_buckets.clear();
+        deallocateBuckets();
         m_freeList.resize(MaxSize);
         for (int i = 0; i < MaxSize; i++)
             m_freeList[i] = MaxSize - (i + 1);
     }
 
 private:
+    Q_DISABLE_COPY(ArrayAllocatingPolicy)
 
     enum {
         MaxSize = (1 << INDEXBITS),
@@ -215,13 +228,25 @@ private:
         BucketSize = (1 << (INDEXBITS < 10 ? INDEXBITS : 10))
     };
 
-    typedef QVector<T> Bucket;
-    QList<Bucket> m_buckets;
-    // optimization: quick access to bucket data pointers
-    // this improves the performance of the releaseResource benchmarks
-    // and reduces their runtime by a factor 2
+    void deallocateBuckets()
+    {
+        while (m_numConstructed > 0) {
+            --m_numConstructed;
+            int bucketIdx = m_numConstructed / BucketSize;
+            int localIdx = m_numConstructed % BucketSize;
+            (m_bucketDataPtrs[bucketIdx] + localIdx)->~T();
+        }
+
+        while (m_numBuckets > 0) {
+            --m_numBuckets;
+            free(m_bucketDataPtrs[m_numBuckets]);
+        }
+    }
+
     T* m_bucketDataPtrs[MaxSize / BucketSize];
     QVector<int> m_freeList;
+    int m_numBuckets;
+    int m_numConstructed;
 
     void performCleanup(T *r, Int2Type<true>)
     {