ITEM: improve SfxItemSet notification callback

When browsing cachegrind data I stumbled over the notification
callback used by Writer in SfxItemSet::Changed. That is a
virtual method that gets called in quite some places to forward
item changes, SW uses it to record these.
For that purpose always (quite some) data gets prepared without
checking if this is necessary, this uses calls to ::Get and
::GetDefaultItem to have either the old or new Item from the
parent or default (pool).
This is not needed - except for Writer. Even there this
mechanism is not always active. Thus I:

- removed SfxItemSet::Changed, replaced with a settable callback
member of type std::function<...>. Thus one less virtual function
and depenence in SfxItemSet
- added a callback functor to SwAttrSet that can be set at the
SfxItemSet it is derived from
- setting/releasing this only in used cases. It is not even used
all the time in SW.
- moved the creation/processing of needed data to a member
function in SW (SwAttrSet::changeCallback). All processing and
evtl. needed data is now created there - on demand.
- adapted all places in SfxItemSet where that mechanism is used
to only call it if set & without pre-calculating anything
- since all former calls/usages were pretty similar I could put
all of this to SwAttrSet::changeCallback

This leads to use that only when needed now. Naturally, SW will
potentially profit less than the other apps.

Here are callgrind numbers with this change using OfficeStart,
DocLoad, DocClose, OfficeShutdown. This change also has potential
avantages @runtime/UI which also did all preparations to call
SfxItemSet::Changed all the time:

Writer doc: 0,9907 ~1%
old: 93842 mio
new: 92971 mio

Draw/Impress doc: 0,9971 ~2,8%
old: 170023 mio
new: 169544 mio
::Get reduces from 1416103 to 293874 calls
::GetDefaultItem reduces from 2252336 to 1927209 calls (nearly half)

Calc doc: 0.9868 ~1,3%
old: 194708 mio
new: 192130 mio
::Get reduces from 882298 to 880087 calls
::GetDefaultItem reduces from 4611901 to 2633555 calls (nearly half)

Of course this highly depends on the used test documents, so it can
only be a excerpt result.

Also adapted SfxItemSet::MergeRange a little bit: Do nothing not only
when a single new slot is already contaioned, but check if all slots
are already present. This works well and fast using the formally added
caching mechanism.

Change-Id: I4d369d2e5b21aa7a21687177518150515e3de954
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/155559
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>

4 files changed, 176 insertions, 72 deletions

diff --git a/include/svl/itemset.hxx b/include/svl/itemset.hxx
index 468893557b5e..a3736427b002 100644
--- a/include/svl/itemset.hxx
+++ b/include/svl/itemset.hxx
@@ -42,9 +42,20 @@ class SAL_WARN_UNUSED SVL_DLLPUBLIC SfxItemSet
     const SfxItemSet* m_pParent;       ///< derivation
     sal_uInt16        m_nCount;        ///< number of items
     sal_uInt16        m_nTotalCount;   ///< number of WhichIDs, also size of m_ppItems array
+
+    // bitfield (better packaging if a bool needs to be added)
+    bool              m_bItemsFixed : 1; ///< true if this is a SfxItemSetFixed object, so does not *own* m_ppItems
+
     SfxPoolItem const** m_ppItems;     ///< pointer to array of items, we allocate and free this unless m_bItemsFixed==true
     WhichRangesContainer m_pWhichRanges;  ///< array of Which Ranges
-    bool              m_bItemsFixed; ///< true if this is a SfxItemSetFixed object
+
+    // Notification-Callback mechanism for SwAttrSet in SW, functionPtr for callback
+    std::function<void(const SfxPoolItem*, const SfxPoolItem*)> m_aCallback;
+
+protected:
+    // Notification-Callback mechanism for SwAttrSet in SW
+    void setCallback(const std::function<void(const SfxPoolItem*, const SfxPoolItem*)> &func) { m_aCallback = func; }
+    void clearCallback() { m_aCallback = nullptr; }
 
 friend class SfxItemPoolCache;
 friend class SfxAllItemSet;
@@ -59,9 +70,6 @@ private:
     const SfxItemSet&           operator=(const SfxItemSet &) = delete;
 
 protected:
-    // Notification-Callback
-    virtual void                Changed( const SfxPoolItem& rOld, const SfxPoolItem& rNew );
-
     void                        PutDirect(const SfxPoolItem &rItem);
 
     virtual const SfxPoolItem*  PutImpl( const SfxPoolItem&, sal_uInt16 nWhich, bool bPassingOwnership );
diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx
index 8529efbbef03..289a69adf4da 100644
--- a/svl/source/items/itemset.cxx
+++ b/svl/source/items/itemset.cxx
@@ -49,9 +49,10 @@ SfxItemSet::SfxItemSet(SfxItemPool& rPool)
     , m_pParent(nullptr)
     , m_nCount(0)
     , m_nTotalCount(svl::detail::CountRanges(rPool.GetFrozenIdRanges()))
+    , m_bItemsFixed(false)
     , m_ppItems(new SfxPoolItem const *[m_nTotalCount]{})
     , m_pWhichRanges(rPool.GetFrozenIdRanges())
-    , m_bItemsFixed(false)
+    , m_aCallback()
 {
     assert(svl::detail::validRanges2(m_pWhichRanges));
 }
@@ -61,8 +62,10 @@ SfxItemSet::SfxItemSet( SfxItemPool& rPool, SfxAllItemSetFlag )
     , m_pParent(nullptr)
     , m_nCount(0)
     , m_nTotalCount(0)
-    , m_ppItems(nullptr)
     , m_bItemsFixed(false)
+    , m_ppItems(nullptr)
+    , m_pWhichRanges()
+    , m_aCallback()
 {
 }
 
@@ -72,9 +75,10 @@ SfxItemSet::SfxItemSet( SfxItemPool& rPool, WhichRangesContainer&& ranges, SfxPo
     , m_pParent(nullptr)
     , m_nCount(0)
     , m_nTotalCount(nTotalCount)
+    , m_bItemsFixed(true)
     , m_ppItems(ppItems)
     , m_pWhichRanges(std::move(ranges))
-    , m_bItemsFixed(true)
+    , m_aCallback()
 {
     assert(ppItems);
     assert(m_pWhichRanges.size() > 0);
@@ -86,9 +90,10 @@ SfxItemSet::SfxItemSet(SfxItemPool& pool, WhichRangesContainer wids)
     , m_pParent(nullptr)
     , m_nCount(0)
     , m_nTotalCount(svl::detail::CountRanges(wids))
+    , m_bItemsFixed(false)
     , m_ppItems(new SfxPoolItem const *[m_nTotalCount]{})
     , m_pWhichRanges(std::move(wids))
-    , m_bItemsFixed(false)
+    , m_aCallback()
 {
     assert(svl::detail::CountRanges(m_pWhichRanges) != 0);
     assert(svl::detail::validRanges2(m_pWhichRanges));
@@ -99,9 +104,10 @@ SfxItemSet::SfxItemSet( const SfxItemSet& rASet )
     , m_pParent( rASet.m_pParent )
     , m_nCount( rASet.m_nCount )
     , m_nTotalCount( rASet.m_nTotalCount )
+    , m_bItemsFixed(false)
     , m_ppItems(nullptr)
     , m_pWhichRanges( rASet.m_pWhichRanges )
-    , m_bItemsFixed(false)
+    , m_aCallback(rASet.m_aCallback)
 {
     if (rASet.m_pWhichRanges.empty())
     {
@@ -139,9 +145,10 @@ SfxItemSet::SfxItemSet(SfxItemSet&& rASet) noexcept
     , m_pParent( rASet.m_pParent )
     , m_nCount( rASet.m_nCount )
     , m_nTotalCount( rASet.TotalCount() )
+    , m_bItemsFixed(false)
     , m_ppItems( rASet.m_ppItems )
     , m_pWhichRanges( std::move(rASet.m_pWhichRanges) )
-    , m_bItemsFixed(false)
+    , m_aCallback(rASet.m_aCallback)
 {
     if (rASet.m_bItemsFixed)
     {
@@ -234,16 +241,12 @@ sal_uInt16 SfxItemSet::ClearSingleItem_ForOffset( sal_uInt16 nOffset )
 
         if ( !IsInvalidItem(pItemToClear) )
         {
-            const sal_uInt16 nWhich(pItemToClear->Which());
-
-            if (SfxItemPool::IsWhich(nWhich))
+            // Notification-Callback
+            if(m_aCallback)
             {
-                const SfxPoolItem& rNew = m_pParent
-                        ? m_pParent->Get( nWhich )
-                        : m_pPool->GetDefaultItem( nWhich );
-
-                Changed( *pItemToClear, rNew );
+                m_aCallback(pItemToClear, nullptr);
             }
+
             if ( pItemToClear->Which() )
                 m_pPool->Remove( *pItemToClear );
         }
@@ -273,13 +276,10 @@ sal_uInt16 SfxItemSet::ClearAllItemsImpl()
             if ( IsInvalidItem(pItemToClear) )
                 continue;
 
-            if (SfxItemPool::IsWhich(nWhich))
+            // Notification-Callback
+            if(m_aCallback)
             {
-                const SfxPoolItem& rNew = m_pParent
-                        ? m_pParent->Get( nWhich )
-                        : m_pPool->GetDefaultItem( nWhich );
-
-                Changed( *pItemToClear, rNew );
+                m_aCallback(pItemToClear, nullptr);
             }
 
             // #i32448#
@@ -443,8 +443,13 @@ const SfxPoolItem* SfxItemSet::PutImpl( const SfxPoolItem& rItem, sal_uInt16 nWh
                 const SfxPoolItem& rNew = m_pPool->PutImpl( rItem, nWhich, bPassingOwnership );
                 const SfxPoolItem* pOld = *ppFnd;
                 *ppFnd = &rNew;
-                if (SfxItemPool::IsWhich(nWhich))
-                    Changed( *pOld, rNew );
+
+                // Notification-Callback
+                if(m_aCallback)
+                {
+                    m_aCallback(pOld, &rNew);
+                }
+
                 m_pPool->Remove( *pOld );
             }
         }
@@ -461,12 +466,11 @@ const SfxPoolItem* SfxItemSet::PutImpl( const SfxPoolItem& rItem, sal_uInt16 nWh
             {
                 const SfxPoolItem& rNew = m_pPool->PutImpl( rItem, nWhich, bPassingOwnership );
                 *ppFnd = &rNew;
-                if (SfxItemPool::IsWhich(nWhich))
+
+                // Notification-Callback
+                if(m_aCallback)
                 {
-                    const SfxPoolItem& rOld = m_pParent
-                        ? m_pParent->Get( nWhich )
-                        : m_pPool->GetDefaultItem( nWhich );
-                    Changed( rOld, rNew );
+                    m_aCallback(nullptr, &rNew);
                 }
             }
         }
@@ -594,12 +598,22 @@ void SfxItemSet::PutExtended
  */
 void SfxItemSet::MergeRange( sal_uInt16 nFrom, sal_uInt16 nTo )
 {
-    // special case: exactly one sal_uInt16 which is already included?
-    if (nFrom == nTo)
-        if (SfxItemState eItemState = GetItemState(nFrom, false);
-            eItemState == SfxItemState::DEFAULT || eItemState == SfxItemState::SET)
-            return;
+    // check if all from new range are already included. This will
+    // use the cache in WhichRangesContainer since we check linearly.
+    // Start with assuming all are included, but only if not empty.
+    // If empty all included is wrong (and m_pWhichRanges.MergeRange
+    // will do the right thing/shortcut)
+    bool bAllIncluded(!m_pWhichRanges.empty());
+
+    for (sal_uInt16 a(nFrom); bAllIncluded && a <= nTo; a++)
+        if (INVALID_WHICHPAIR_OFFSET == m_pWhichRanges.getOffsetFromWhich(a))
+            bAllIncluded = false;
+
+    // if yes, we are done
+    if (bAllIncluded)
+        return;
 
+    // need to create new WhichRanges
     auto pNewRanges = m_pWhichRanges.MergeRange(nFrom, nTo);
     RecreateRanges_Impl(pNewRanges);
     m_pWhichRanges = std::move(pNewRanges);
@@ -815,13 +829,6 @@ const SfxPoolItem& SfxItemSet::Get( sal_uInt16 nWhich, bool bSrchInParent) const
 }
 
 /**
- * Notification callback
- */
-void SfxItemSet::Changed( const SfxPoolItem&, const SfxPoolItem& )
-{
-}
-
-/**
  * Only retain the Items that are also present in rSet
  * (nevermind their value).
  */
@@ -851,15 +858,12 @@ void SfxItemSet::Intersect( const SfxItemSet& rSet )
                 // Delete from Pool
                 if( !IsInvalidItem( *ppFnd1 ) )
                 {
-                    sal_uInt16 nWhich = (*ppFnd1)->Which();
-                    if (SfxItemPool::IsWhich(nWhich))
+                    // Notification-Callback
+                    if(m_aCallback)
                     {
-                        const SfxPoolItem& rNew = m_pParent
-                            ? m_pParent->Get( nWhich )
-                            : m_pPool->GetDefaultItem( nWhich );
-
-                        Changed( **ppFnd1, rNew );
+                        m_aCallback(*ppFnd1, nullptr);
                     }
+
                     m_pPool->Remove( **ppFnd1 );
                 }
                 *ppFnd1 = nullptr;
@@ -897,15 +901,12 @@ void SfxItemSet::Differentiate( const SfxItemSet& rSet )
                 // Delete from Pool
                 if( !IsInvalidItem( *ppFnd1 ) )
                 {
-                    sal_uInt16 nWhich = (*ppFnd1)->Which();
-                    if (SfxItemPool::IsWhich(nWhich))
+                    // Notification-Callback
+                    if(m_aCallback)
                     {
-                        const SfxPoolItem& rNew = m_pParent
-                            ? m_pParent->Get( nWhich )
-                            : m_pPool->GetDefaultItem( nWhich );
-
-                        Changed( **ppFnd1, rNew );
+                        m_aCallback(*ppFnd1, nullptr);
                     }
+
                     m_pPool->Remove( **ppFnd1 );
                 }
                 *ppFnd1 = nullptr;
@@ -1627,6 +1628,7 @@ WhichRangesContainer WhichRangesContainer::MergeRange(sal_uInt16 nFrom,
     if (empty())
         return WhichRangesContainer(nFrom, nTo);
 
+    // reset buffer
     m_aLastWhichPairOffset = INVALID_WHICHPAIR_OFFSET;
 
     // create vector of ranges (sal_uInt16 pairs of lower and upper bound)
diff --git a/sw/inc/swatrset.hxx b/sw/inc/swatrset.hxx
index 96a7cc833e55..b469b77fc9e9 100644
--- a/sw/inc/swatrset.hxx
+++ b/sw/inc/swatrset.hxx
@@ -161,10 +161,22 @@ public:
 class SW_DLLPUBLIC SwAttrSet final : public SfxItemSet
 {
     // Pointer for Modify-System
-    SwAttrSet *m_pOldSet, *m_pNewSet;
+    SwAttrSet *m_pOldSet;
+    SwAttrSet *m_pNewSet;
 
-    // Notification-Callback
-    virtual void Changed( const SfxPoolItem& rOld, const SfxPoolItem& rNew ) override;
+    // helper class for change callback & local instance
+    // needed to forward the processing call to the correct instance
+    class callbackHolder final
+    {
+    private:
+        SwAttrSet* m_Set;
+    public:
+        callbackHolder(SwAttrSet* pSet) : m_Set(pSet) {}
+        void operator () (const SfxPoolItem* pOld, const SfxPoolItem* pNew) { m_Set->changeCallback(pOld, pNew); }
+    } m_aCallbackHolder;
+
+    // processor for change callback
+    void changeCallback(const SfxPoolItem* pOld, const SfxPoolItem* pNew) const;
 
     void PutChgd( const SfxPoolItem& rI ) { SfxItemSet::PutDirect( rI ); }
 public:
diff --git a/sw/source/core/attr/swatrset.cxx b/sw/source/core/attr/swatrset.cxx
index e35dfc492406..f2761d7dec41 100644
--- a/sw/source/core/attr/swatrset.cxx
+++ b/sw/source/core/attr/swatrset.cxx
@@ -92,19 +92,100 @@ SwAttrPool::~SwAttrPool()
     SetSecondaryPool(nullptr);
 }
 
+/// Notification callback
+void SwAttrSet::changeCallback(const SfxPoolItem* pOld, const SfxPoolItem* pNew) const
+{
+    // at least one SfxPoolItem has to be provided, else this is an error
+    assert(nullptr != pOld || nullptr != pNew);
+    sal_uInt16 nWhich(0);
+
+    if (nullptr != pOld)
+    {
+        // do not handle if an invalid item is involved
+        if (IsInvalidItem(pOld))
+            return;
+
+        // get WhichID from pOld
+        nWhich = pOld->Which();
+    }
+
+    if (nullptr != pNew)
+    {
+        // do not handle if an invalid item is involved
+        if (IsInvalidItem(pNew))
+            return;
+
+        if (0 == nWhich)
+        {
+            // get WhichID from pNew
+            nWhich = pNew->Which();
+        }
+    }
+
+    // all given itmes are valid. If we got no WhichID != 0 then
+    // pOld == pNew == nullptr or SfxVoidItem(0) and we have no
+    // valid input. Also not needed if !IsWhich (aka > SFX_WHICH_MAX)
+    if (0 == nWhich || !SfxItemPool::IsWhich(nWhich))
+        return;
+
+    if(m_pOldSet)
+    {
+        // old state shall be saved
+        if (nullptr == pOld)
+        {
+            // no old value given, generate default from WhichID
+            const SfxItemSet* pParent(GetParent());
+            m_pOldSet->PutChgd(nullptr != pParent
+                ? pParent->Get(nWhich)
+                : GetPool()->GetDefaultItem(nWhich));
+        }
+        else if (!IsInvalidItem(pOld))
+        {
+            // set/remember old value
+            m_pOldSet->PutChgd(*pOld);
+        }
+    }
+
+    if(m_pNewSet)
+    {
+        // old state shall be saved
+        if (nullptr == pNew)
+        {
+            // no new value given, generate default from WhichID
+            const SfxItemSet* pParent(GetParent());
+            m_pNewSet->PutChgd(nullptr != pParent
+                ? pParent->Get(nWhich)
+                : GetPool()->GetDefaultItem(nWhich));
+        }
+        else if (!IsInvalidItem(pNew))
+        {
+            // set/remember new value
+            m_pNewSet->PutChgd(*pNew);
+        }
+    }
+}
+
 SwAttrSet::SwAttrSet( SwAttrPool& rPool, sal_uInt16 nWh1, sal_uInt16 nWh2 )
-    : SfxItemSet( rPool, nWh1, nWh2 ), m_pOldSet( nullptr ), m_pNewSet( nullptr )
+    : SfxItemSet( rPool, nWh1, nWh2 )
+    , m_pOldSet( nullptr )
+    , m_pNewSet( nullptr )
+    , m_aCallbackHolder(this)
 {
 }
 
 SwAttrSet::SwAttrSet( SwAttrPool& rPool, const WhichRangesContainer& nWhichPairTable )
     : SfxItemSet( rPool, nWhichPairTable )
-    , m_pOldSet( nullptr ), m_pNewSet( nullptr )
+    , m_pOldSet( nullptr )
+    , m_pNewSet( nullptr )
+    , m_aCallbackHolder(this)
 {
 }
 
 SwAttrSet::SwAttrSet( const SwAttrSet& rSet )
-    : SfxItemSet( rSet ), m_pOldSet( nullptr ), m_pNewSet( nullptr )
+    : SfxItemSet( rSet )
+    , m_pOldSet( nullptr )
+    , m_pNewSet( nullptr )
+    , m_aCallbackHolder(this)
 {
 }
 
@@ -146,7 +227,9 @@ bool SwAttrSet::Put_BC( const SfxPoolItem& rAttr,
 {
     m_pNewSet = pNew;
     m_pOldSet = pOld;
+    setCallback(m_aCallbackHolder);
     bool bRet = nullptr != SfxItemSet::Put( rAttr );
+    clearCallback();
     m_pOldSet = m_pNewSet = nullptr;
     return bRet;
 }
@@ -156,7 +239,9 @@ bool SwAttrSet::Put_BC( const SfxItemSet& rSet,
 {
     m_pNewSet = pNew;
     m_pOldSet = pOld;
+    setCallback(m_aCallbackHolder);
     bool bRet = SfxItemSet::Put( rSet );
+    clearCallback();
     m_pOldSet = m_pNewSet = nullptr;
     return bRet;
 }
@@ -166,7 +251,9 @@ sal_uInt16 SwAttrSet::ClearItem_BC( sal_uInt16 nWhich,
 {
     m_pNewSet = pNew;
     m_pOldSet = pOld;
+    setCallback(m_aCallbackHolder);
     sal_uInt16 nRet = SfxItemSet::ClearItem( nWhich );
+    clearCallback();
     m_pOldSet = m_pNewSet = nullptr;
     return nRet;
 }
@@ -177,9 +264,11 @@ sal_uInt16 SwAttrSet::ClearItem_BC( sal_uInt16 nWhich1, sal_uInt16 nWhich2,
     OSL_ENSURE( nWhich1 <= nWhich2, "no valid range" );
     m_pNewSet = pNew;
     m_pOldSet = pOld;
+    setCallback(m_aCallbackHolder);
     sal_uInt16 nRet = 0;
     for( ; nWhich1 <= nWhich2; ++nWhich1 )
         nRet = nRet + SfxItemSet::ClearItem( nWhich1 );
+    clearCallback();
     m_pOldSet = m_pNewSet = nullptr;
     return nRet;
 }
@@ -189,20 +278,13 @@ int SwAttrSet::Intersect_BC( const SfxItemSet& rSet,
 {
     m_pNewSet = pNew;
     m_pOldSet = pOld;
+    setCallback(m_aCallbackHolder);
     SfxItemSet::Intersect( rSet );
+    clearCallback();
     m_pOldSet = m_pNewSet = nullptr;
     return pNew ? pNew->Count() : ( pOld ? pOld->Count() : 0 );
 }
 
-/// Notification callback
-void  SwAttrSet::Changed( const SfxPoolItem& rOld, const SfxPoolItem& rNew )
-{
-    if( m_pOldSet )
-        m_pOldSet->PutChgd( rOld );
-    if( m_pNewSet )
-        m_pNewSet->PutChgd( rNew );
-}
-
 /** special treatment for some attributes
 
     Set the Modify pointer (old pDefinedIn) for the following attributes: