flatten SfxItemPool_Impl (tdf#81765 related)

Flatten the vector of SfxPoolItemArray_Impl, to reduce pointer chasing.
This struct is movable, etc, so no need to allocate it separately on the
heap.

Change-Id: I794b4356660e9cd0e63bc98b011f58162a838662
Reviewed-on: https://gerrit.libreoffice.org/70884
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

svl/qa/unit/items/test_itempool.cxx

@@ -43,17 +43,17 @@ void PoolItemTest::testPool()
     SfxItemPool *pPool = new SfxItemPool("testpool", 1, 4, aItems);
     SfxItemPool_Impl *pImpl = SfxItemPool_Impl::GetImpl(pPool);
     CPPUNIT_ASSERT(pImpl != nullptr);
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(4), pImpl->maPoolItems.size());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(4), pImpl->maPoolItemArrays.size());
 
     // Poolable
     SfxVoidItem aItemOne( 1 );
     SfxVoidItem aNotherOne( 1 );
 
     {
-        CPPUNIT_ASSERT(!pImpl->maPoolItems[0]);
+        CPPUNIT_ASSERT(pImpl->maPoolItemArrays[0].empty());
         const SfxPoolItem &rVal = pPool->Put(aItemOne);
         CPPUNIT_ASSERT(bool(rVal == aItemOne));
-        CPPUNIT_ASSERT(pImpl->maPoolItems[0] != nullptr);
+        CPPUNIT_ASSERT(!pImpl->maPoolItemArrays[0].empty());
         const SfxPoolItem &rVal2 = pPool->Put(aNotherOne);
         CPPUNIT_ASSERT(bool(rVal2 == rVal));
         CPPUNIT_ASSERT_EQUAL(&rVal, &rVal2);
@@ -69,10 +69,10 @@ void PoolItemTest::testPool()
     SfxVoidItem aItemTwo( 2 );
     SfxVoidItem aNotherTwo( 2 );
     {
-        CPPUNIT_ASSERT(!pImpl->maPoolItems[1]);
+        CPPUNIT_ASSERT(pImpl->maPoolItemArrays[1].empty());
         const SfxPoolItem &rVal = pPool->Put(aItemTwo);
         CPPUNIT_ASSERT(bool(rVal == aItemTwo));
-        CPPUNIT_ASSERT(pImpl->maPoolItems[1] != nullptr);
+        CPPUNIT_ASSERT(!pImpl->maPoolItemArrays[1].empty());
 
         const SfxPoolItem &rVal2 = pPool->Put(aNotherTwo);
         CPPUNIT_ASSERT(bool(rVal2 == rVal));
@@ -84,12 +84,12 @@ void PoolItemTest::testPool()
     SfxVoidItem aNotherFour(4);
     const SfxPoolItem &rKeyFour = pPool->Put(aRemoveFour);
     pPool->Put(aNotherFour);
-    CPPUNIT_ASSERT(pImpl->maPoolItems[3]->size() > 0);
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), pImpl->maPoolItems[3]->size());
+    CPPUNIT_ASSERT(pImpl->maPoolItemArrays[3].size() > 0);
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), pImpl->maPoolItemArrays[3].size());
     pPool->Remove(rKeyFour);
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), pImpl->maPoolItems[3]->size());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), pImpl->maPoolItemArrays[3].size());
     pPool->Put(aNotherFour);
-    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), pImpl->maPoolItems[3]->size());
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(2), pImpl->maPoolItemArrays[3].size());
 }
 
 

svl/source/inc/poolio.hxx

@@ -43,21 +43,22 @@ struct SfxPoolItemArray_Impl
 private:
     o3tl::sorted_vector<SfxPoolItem*> maPoolItemSet;
 public:
-    o3tl::sorted_vector<SfxPoolItem*>::const_iterator begin() { return maPoolItemSet.begin(); }
-    o3tl::sorted_vector<SfxPoolItem*>::const_iterator end() { return maPoolItemSet.end(); }
+    o3tl::sorted_vector<SfxPoolItem*>::const_iterator begin() const { return maPoolItemSet.begin(); }
+    o3tl::sorted_vector<SfxPoolItem*>::const_iterator end() const { return maPoolItemSet.end(); }
     /// clear array of PoolItem variants after all PoolItems are deleted
     /// or all ref counts are decreased
     void clear();
     size_t size() const {return maPoolItemSet.size();}
+    bool empty() const {return maPoolItemSet.empty();}
     void insert(SfxPoolItem* pItem) { maPoolItemSet.insert(pItem); }
-    o3tl::sorted_vector<SfxPoolItem*>::const_iterator find(SfxPoolItem* pItem) { return maPoolItemSet.find(pItem); }
+    o3tl::sorted_vector<SfxPoolItem*>::const_iterator find(SfxPoolItem* pItem) const { return maPoolItemSet.find(pItem); }
     void erase(o3tl::sorted_vector<SfxPoolItem*>::const_iterator it) { return maPoolItemSet.erase(it); }
 };
 
 struct SfxItemPool_Impl
 {
     SfxBroadcaster                  aBC;
-    std::vector<std::unique_ptr<SfxPoolItemArray_Impl>> maPoolItems;
+    std::vector<SfxPoolItemArray_Impl> maPoolItemArrays;
     std::vector<SfxItemPoolUser*>   maSfxItemPoolUsers; /// ObjectUser section
     OUString                        aName;
     std::vector<SfxPoolItem*>       maPoolDefaults;
@@ -70,7 +71,7 @@ struct SfxItemPool_Impl
     MapUnit                         eDefMetric;
 
     SfxItemPool_Impl( SfxItemPool* pMaster, const OUString& rName, sal_uInt16 nStart, sal_uInt16 nEnd )
-        : maPoolItems(nEnd - nStart + 1)
+        : maPoolItemArrays(nEnd - nStart + 1)
         , aName(rName)
         , maPoolDefaults(nEnd - nStart + 1)
         , mpStaticDefaults(nullptr)
@@ -90,7 +91,7 @@ struct SfxItemPool_Impl
 
     void DeleteItems()
     {
-        maPoolItems.clear();
+        maPoolItemArrays.clear();
         maPoolDefaults.clear();
         mpPoolRanges.reset();
     }

svl/source/items/itempool.cxx

@@ -250,7 +250,7 @@ void SfxItemPool::SetDefaults( std::vector<SfxPoolItem*>* pDefaults )
             assert(  ((*pImpl->mpStaticDefaults)[n]->Which() == n + pImpl->mnStart)
                         && "static defaults not sorted" );
             (*pImpl->mpStaticDefaults)[n]->SetKind(SfxItemKind::StaticDefault);
-            DBG_ASSERT( !(pImpl->maPoolItems[n]), "defaults with setitems with items?!" );
+            DBG_ASSERT( pImpl->maPoolItemArrays[n].empty(), "defaults with setitems with items?!" );
         }
     }
 }
@@ -331,7 +331,7 @@ void SfxItemPool::ReleaseDefaults
 
 SfxItemPool::~SfxItemPool()
 {
-    if ( !pImpl->maPoolItems.empty() && !pImpl->maPoolDefaults.empty() )
+    if ( !pImpl->maPoolItemArrays.empty() && !pImpl->maPoolDefaults.empty() )
         Delete();
 
     if (pImpl->mpMaster != nullptr && pImpl->mpMaster != this)
@@ -374,8 +374,8 @@ void SfxItemPool::SetSecondaryPool( SfxItemPool *pPool )
     if ( pImpl->mpSecondary )
     {
 #ifdef DBG_UTIL
-        if (pImpl->mpStaticDefaults != nullptr && !pImpl->maPoolItems.empty()
-            && !pImpl->mpSecondary->pImpl->maPoolItems.empty())
+        if (pImpl->mpStaticDefaults != nullptr && !pImpl->maPoolItemArrays.empty()
+            && !pImpl->mpSecondary->pImpl->maPoolItemArrays.empty())
             // Delete() did not yet run?
         {
                 // Does the Master have SetItems?
@@ -385,11 +385,11 @@ void SfxItemPool::SetSecondaryPool( SfxItemPool *pPool )
 
             // Detached Pools must be empty
             bool bOK = bHasSetItems;
-            for (auto const& rSecArrayPtr : pImpl->mpSecondary->pImpl->maPoolItems)
+            for (auto const& rSecArray : pImpl->mpSecondary->pImpl->maPoolItemArrays)
             {
                 if (!bOK)
                     break;
-                if (rSecArrayPtr && rSecArrayPtr->size()>0)
+                if (rSecArray.size()>0)
                 {
                     SAL_WARN("svl.items", "old secondary pool: " << pImpl->mpSecondary->pImpl->aName
                                     << " of pool: " << pImpl->aName << " must be empty.");
@@ -463,7 +463,7 @@ SfxItemPool* SfxItemPool::Clone() const
 void SfxItemPool::Delete()
 {
     // Already deleted?
-    if (pImpl->maPoolItems.empty() || pImpl->maPoolDefaults.empty())
+    if (pImpl->maPoolItemArrays.empty() || pImpl->maPoolDefaults.empty())
         return;
 
     // Inform e.g. running Requests
@@ -480,17 +480,14 @@ void SfxItemPool::Delete()
             if (dynamic_cast<const SfxSetItem*>(pStaticDefaultItem))
             {
                 // SfxSetItem found, remove PoolItems (and defaults) with same ID
-                auto& rArrayPtr = pImpl->maPoolItems[n];
-                if (rArrayPtr)
+                auto& rArray = pImpl->maPoolItemArrays[n];
+                for (auto& rItemPtr : rArray)
                 {
-                    for (auto& rItemPtr : *rArrayPtr)
-                    {
-                        ReleaseRef(*rItemPtr, rItemPtr->GetRefCount()); // for RefCount check in dtor
-                        delete rItemPtr;
-                    }
-                    rArrayPtr->clear();
-                    // let pImpl->DeleteItems() delete item arrays in maPoolItems
+                    ReleaseRef(*rItemPtr, rItemPtr->GetRefCount()); // for RefCount check in dtor
+                    delete rItemPtr;
                 }
+                rArray.clear();
+                // let pImpl->DeleteItems() delete item arrays in maPoolItems
                 auto& rItemPtr = pImpl->maPoolDefaults[n];
                 if (rItemPtr)
                 {
@@ -505,19 +502,17 @@ void SfxItemPool::Delete()
     }
 
     // now remove remaining PoolItems (and defaults) who didn't have SetItems
-    for (auto& rArrayPtr : pImpl->maPoolItems)
+    for (auto& rArray : pImpl->maPoolItemArrays)
     {
-        if (rArrayPtr)
+        for (auto& rItemPtr : rArray)
         {
-            for (auto& rItemPtr : *rArrayPtr)
-            {
-                ReleaseRef(*rItemPtr, rItemPtr->GetRefCount()); // for RefCount check in dtor
-                delete rItemPtr;
-            }
-            rArrayPtr->clear();
-            // let pImpl->DeleteItems() delete item arrays in maPoolItems
+            ReleaseRef(*rItemPtr, rItemPtr->GetRefCount()); // for RefCount check in dtor
+            delete rItemPtr;
         }
+        rArray.clear();
+        // let pImpl->DeleteItems() delete item arrays in maPoolItems
     }
+    pImpl->maPoolItemArrays.clear();
     // default items
     for (auto rItemPtr : pImpl->maPoolDefaults)
     {
@@ -613,12 +608,7 @@ const SfxPoolItem& SfxItemPool::Put( const SfxPoolItem& rItem, sal_uInt16 nWhich
             typeid(rItem) == typeid(GetDefaultItem(nWhich)));
 
     const sal_uInt16 nIndex = GetIndex_Impl(nWhich);
-    SfxPoolItemArray_Impl* pItemArr = pImpl->maPoolItems[nIndex].get();
-    if (!pItemArr)
-    {
-        pImpl->maPoolItems[nIndex].reset(new SfxPoolItemArray_Impl);
-        pItemArr = pImpl->maPoolItems[nIndex].get();
-    }
+    SfxPoolItemArray_Impl& rItemArr = pImpl->maPoolItemArrays[nIndex];
 
     // Is this a 'poolable' item - ie. should we re-use and return
     // the same underlying item for equivalent (==) SfxPoolItems?
@@ -627,10 +617,10 @@ const SfxPoolItem& SfxItemPool::Put( const SfxPoolItem& rItem, sal_uInt16 nWhich
         // if is already in a pool, then it is worth checking if it is in this one.
         if ( IsPooledItem(&rItem) )
         {
-            auto it = pItemArr->find(const_cast<SfxPoolItem *>(&rItem));
+            auto it = rItemArr.find(const_cast<SfxPoolItem *>(&rItem));
 
             // 1. search for an identical pointer in the pool
-            if (it != pItemArr->end())
+            if (it != rItemArr.end())
             {
                 AddRef(rItem);
                 return rItem;
@@ -638,7 +628,7 @@ const SfxPoolItem& SfxItemPool::Put( const SfxPoolItem& rItem, sal_uInt16 nWhich
         }
 
         // 2. search for an item with matching attributes.
-        for (auto itr = pItemArr->begin(); itr != pItemArr->end(); ++itr)
+        for (auto itr = rItemArr.begin(); itr != rItemArr.end(); ++itr)
         {
             if (**itr == rItem)
             {
@@ -662,8 +652,8 @@ const SfxPoolItem& SfxItemPool::Put( const SfxPoolItem& rItem, sal_uInt16 nWhich
     AddRef( *pNewItem );
 
     // 4. finally insert into the pointer array
-    assert( pItemArr->find(pNewItem) == pItemArr->end() );
-    pItemArr->insert( pNewItem );
+    assert( rItemArr.find(pNewItem) == rItemArr.end() );
+    rItemArr.insert( pNewItem );
     return *pNewItem;
 }
 
@@ -704,11 +694,10 @@ void SfxItemPool::Remove( const SfxPoolItem& rItem )
         return;
 
     // Find Item in own Pool
-    SfxPoolItemArray_Impl* pItemArr = pImpl->maPoolItems[nIndex].get();
-    assert(pItemArr && "removing Item not in Pool");
+    SfxPoolItemArray_Impl& rItemArr = pImpl->maPoolItemArrays[nIndex];
 
-    auto it = pItemArr->find(const_cast<SfxPoolItem *>(&rItem));
-    if (it != pItemArr->end())
+    auto it = rItemArr.find(const_cast<SfxPoolItem *>(&rItem));
+    if (it != rItemArr.end())
     {
         if ( rItem.GetRefCount() ) //!
             ReleaseRef( rItem );
@@ -722,7 +711,7 @@ void SfxItemPool::Remove( const SfxPoolItem& rItem )
         if ( 0 == rItem.GetRefCount() && nWhich < 4000 )
         {
             delete &rItem;
-            pItemArr->erase(it);
+            rItemArr.erase(it);
         }
 
         return;
@@ -822,11 +811,8 @@ SfxItemPool::Item2Range SfxItemPool::GetItemSurrogates(sal_uInt16 nWhich) const
         return { EMPTY.end(), EMPTY.end() };
     }
 
-    SfxPoolItemArray_Impl* pItemArr = pImpl->maPoolItems[GetIndex_Impl(nWhich)].get();
-    if( pItemArr )
-        return { pItemArr->begin(), pItemArr->end() };
-
-    return { EMPTY.end(), EMPTY.end() };
+    SfxPoolItemArray_Impl& rItemArr = pImpl->maPoolItemArrays[GetIndex_Impl(nWhich)];
+    return { rItemArr.begin(), rItemArr.end() };
 }
 
 sal_uInt32 SfxItemPool::GetItemCount2(sal_uInt16 nWhich) const
@@ -839,10 +825,8 @@ sal_uInt32 SfxItemPool::GetItemCount2(sal_uInt16 nWhich) const
         return 0;
     }
 
-    SfxPoolItemArray_Impl* pItemArr = pImpl->maPoolItems[GetIndex_Impl(nWhich)].get();
-    if  ( pItemArr )
-        return pItemArr->size();
-    return 0;
+    SfxPoolItemArray_Impl& rItemArr = pImpl->maPoolItemArrays[GetIndex_Impl(nWhich)];
+    return rItemArr.size();
 }
 
 
@@ -912,10 +896,9 @@ sal_uInt16 SfxItemPool::GetTrueSlotId( sal_uInt16 nWhich ) const
 void SfxItemPool::dumpAsXml(xmlTextWriterPtr pWriter) const
 {
     xmlTextWriterStartElement(pWriter, BAD_CAST("SfxItemPool"));
-    for (auto const & rArrayPtr : pImpl->maPoolItems)
-        if (rArrayPtr)
-            for (auto const & rItem : *rArrayPtr)
-                rItem->dumpAsXml(pWriter);
+    for (auto const & rArray : pImpl->maPoolItemArrays)
+        for (auto const & rItem : rArray)
+            rItem->dumpAsXml(pWriter);
     xmlTextWriterEndElement(pWriter);
 }
 

svl/source/items/poolio.cxx

@@ -83,10 +83,9 @@ bool SfxItemPool::CheckItemInPool(const SfxPoolItem *pItem) const
     if( IsStaticDefaultItem(pItem) || IsPoolDefaultItem(pItem) )
         return true;
 
-    SfxPoolItemArray_Impl* pItemArr = pImpl->maPoolItems[GetIndex_Impl(pItem->Which())].get();
-    DBG_ASSERT(pItemArr, "ItemArr is not available");
+    SfxPoolItemArray_Impl& rItemArr = pImpl->maPoolItemArrays[GetIndex_Impl(pItem->Which())];
 
-    for ( auto p : *pItemArr )
+    for ( auto p : rItemArr )
     {
         if ( p == pItem )
             return true;