Decouple ScPatternAttr from SfxItemPool

ScPatternAttr is traditionally derived from SfxPoolItem
(or better: SfxSetItem) and held in the ScDocumentPool
as Item.

This is only because of 'using' the 'poolable'
functionality of the Item/ItemSet/ItemPool mechanism.
Lots of hacks were added to sc and Item/ItemSet/
ItemPool to make that 'work' which shows already that
this relationship is not optimal.

It uses DirectPutItemInPool/DirectRemoveItemFromPool
to do so, also with massive overhead to do that (and
with not much success). The RefCnt in the SfxPoolItem
that is used for this never worked reliably, so the
SfxItemPool was (ab)used as garbage collector (all
Items added and never removed get deleted at last
for good when the Pool goes down). For this reasons
and to be able to further get ItemSets modernized
I changed this. I did two big changes here:

(1) No longer derive ScPatternAttr from SfxItemSet/
SfxSetItem, no longer hold as SfxPoolItem

(2) Add tooling to reliably control the lifetime of
ScPatternAttr instances and ther uniqueness/
reusage for memory reasons

It is now a regular non-derived class. The SfxItemSet
formally derived from SfxSetItem is now a member. The
RefCnt is now also a member (so independent from
size/data type of SfxPoolItem). All in all it's pretty
much the same size as before.

To support handling it I created a CellAttributeHelper
that is at/owned by ScDocument and takes over tooling
to handle the ScPatternAttr. It supports to guarantee
the uniqueness of incarnated ScPatternAttr instances for
a ScDocument by providing helpers like registerAndCheck
and doUnregister. It hosts the default CellAttribute/
ScPatternAttr. That default handling was anyways not
using the standard default-handling of Items/Pools.

I adapted whole SC to use that mainly by replacing calls
to DirectPutItemInPool with registerAndCheck and
DirectRemoveItemFromPool with doUnregister, BUT: This
was not sufficient, the RefCnt kept to be broken.

For that reason I decided to also do (2) in this change:
I added a CellAttributeHolder that owns/regulates the
lifetime of a single ScPatternAttr. Originally it also
contained the CellAttributeHolder, but after some
thoughts I decided that this is not needed - if there
is no ScPatternAttr set, no CellAttributeHolder is
needed for safe cleanup at destruction of the helper.

So I moved/added the CellAttributeHolder to ScPatternAttr
where it belongs more naturally anyways. The big plus is
that CellAttributeHolder is just one ptr, so not bigger
than having a simple ScPatternAttr*. That way, e.g.
ScAttrEntry in ScAttrArray did not 'grow' at all. In
principle all places where a ScPatternAttr* is used can
now be replaced by using a CellAttributeHolder, except
for construction. It is capable to be initialized with
either ScPatternAttr instances from the heap (it creates
a copy that then gets RefCounted) or allocated (it
supports ownership change at construction time).

Note that ScAttrEntry started to get more a C++ class
in that change, it has a constructor. I did not change
the SCROW member, but that should also be done.

Also made registerAndCheck/doUnregister private in
CellAttributeHelper and exclusively used by
CellAttributeHolder. That way the RefCnt works, and a
lot of code gets much simpler (check ScItemPoolCache,
it's now straightforward) and safer and ~ScPatternAttr()
uses now a hard
assert(!isRegistered());
which shows that RefCnt works now (the 1st time?).

There can be done more (see ToDo section below) but I
myself will concentrate on getting ItemSets forward.

This decoupling makes both involved mechanisms more safe,
less complex and more stable. It also opens up
possibilities to further optimize ScPatternAttr in SC
without further hacking Item/ItemSet/ItemPool stuff.

NOTE: ScPatternAttr *should* be renamed to 'CellAttribute'
which describes what it is. The experiencd devs may know
what it does, but it is a hindrance for understanding for
attacting new devs. I already used now names like
CellAttributeHelper/CellAttributeHolder etc., but
abstained from renaming ScPatternAttr, see ToDo list below.

SfxItemSet discussion:

ScPatternAttr still contains a SfxItemSet, or better, a
SfxSetItem. For that reason it still depends on access to
an SfxItemPool (so there is acces in CellAttributeHelper).

This is in principle not needed - no Item (in the range
[ATTR_PATTERN_START .. ATTR_PATTERN_END]) needs that.
In principle ScPatternAttr could now do it's own handling
of those needed Items, however this might be done (linear
array, hash-by-WhichID, ...). The Items get translated
to and from this to the rest of the office anyways.

Note that *merging* of SfxItemSets is *still* needed what
means to have WhichID slots in SfxItemState::DONTCARE,
see note in ScPatternAttr::ScPatternAttr about that. And
there is also the Surrogates stuff which would have to be
checked.

The other extreme is to use SfxItemSet *more*, e.g. directly
derive from SfxItemSet what would make stuff easier, maybe
even get back to using the 'regular' Items like all office,
I doubt that that would be much slower, so why...?

Also possible is to remove that range of Items exclusively
used by ScPatternAttr from ScDocumentPool *completely* and
create an own Pool for them, owned by CellAttributeHelper.
That Pool might even be static global, so all SC Docs could
share all those Items - maybe even the ScPatternAttr
themselves (except the default per document). That would
remove the dependency of ScPatternAttr from a Pool
completely.

ToDo-List:
- rename ScPatternAttr to CellAttribute or similar
- use SfxItemSetFixed with range [ATTR_PATTERN_START
.. ATTR_PATTERN_END] instead of regular SfxItemSet
(if the copy-construtor works now...?)
- maybe create own/separate Pool for exclusive Items
- make ScAttrEntry more a C++ class by moving SCROW
to the private section, add get/set methods and
adapt SC

Had to add some more usages of CellAttributeHolder
to the SC Sort mechanism, there were situations where
the sorted ScPatternAttr were replaced in the Table,
but the 'sorted' ones were just ScPatternAttr*, thus
deleting the valid ones in the Table already. Using
CellAttributeHolder makes this safe, too.

Added a small, one-entry cache to CellAttributeHelper
to buffer the last found buffered ScPattrnAttr. It
has a HitRate of ca. 5-6% and brings the UnitTest
testSheetCellRangeProperties from 0m48,710s to
0m37,556s. Not too massive, but erery bit counts :-)
Also shows that after that change optimizations in
the now split functionality is possible and easy.

Change-Id: I268a7b2a943ce5ddfe3c75b5e648c0f6b0cedb85
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/161244
Tested-by: Jenkins
Reviewed-by: Armin Le Grand <Armin.Le.Grand@me.com>

3 files changed, 11 insertions, 53 deletions

diff --git a/svl/source/items/itempool.cxx b/svl/source/items/itempool.cxx
index faedbd516a41..2b60edf14091 100644
--- a/svl/source/items/itempool.cxx
+++ b/svl/source/items/itempool.cxx
@@ -78,7 +78,6 @@ size_t getRemainingDirectlyPooledSfxPoolItemCount() { return nRemainingDirectlyP
 // due to ImpEditEngine::WriteRTF
 //  EE_CHAR_COLOR ok
 // due to ScDocumentPool::StyleDeleted
-//  ATTR_PATTERN ok
 // due to ScDocument::UpdateFontCharSet()
 // due to ScXMLFontAutoStylePool_Impl
 //  ATTR_FONT ok
@@ -794,10 +793,6 @@ void SfxItemPool::ResetPoolDefaultItem( sal_uInt16 nWhichId )
 
 const SfxPoolItem& SfxItemPool::DirectPutItemInPoolImpl(const SfxPoolItem& rItem, sal_uInt16 nWhich, bool bPassingOwnership)
 {
-    // CAUTION: Do not register the problematic pool default
-    if (rItem.isExceptionalSCItem() && GetMasterPool()->newItem_UseDirect(rItem))
-        return rItem;
-
 #ifdef DBG_UTIL
     nAllDirectlyPooledSfxPoolItemCount++;
     nRemainingDirectlyPooledSfxPoolItemCount++;
@@ -816,10 +811,6 @@ const SfxPoolItem& SfxItemPool::DirectPutItemInPoolImpl(const SfxPoolItem& rItem
 
 void SfxItemPool::DirectRemoveItemFromPool(const SfxPoolItem& rItem)
 {
-    // CAUTION: Do not remove the problematic pool default
-    if (rItem.isExceptionalSCItem() && GetMasterPool()->newItem_UseDirect(rItem))
-        return;
-
 #ifdef DBG_UTIL
     nRemainingDirectlyPooledSfxPoolItemCount--;
 #endif
@@ -828,19 +819,6 @@ void SfxItemPool::DirectRemoveItemFromPool(const SfxPoolItem& rItem)
     implCleanupItemEntry(*GetMasterPool(), &rItem);
 }
 
-void SfxItemPool::newItem_Callback(const SfxPoolItem& rItem) const
-{
-    if (!IsInRange(rItem.Which()) && pImpl->mpSecondary)
-        pImpl->mpSecondary->newItem_Callback(rItem);
-}
-
-bool SfxItemPool::newItem_UseDirect(const SfxPoolItem& rItem) const
-{
-    if (!IsInRange(rItem.Which()) && pImpl->mpSecondary)
-        return pImpl->mpSecondary->newItem_UseDirect(rItem);
-    return false;
-}
-
 const SfxPoolItem& SfxItemPool::GetDefaultItem( sal_uInt16 nWhich ) const
 {
     if ( !IsInRange(nWhich) )
diff --git a/svl/source/items/itemset.cxx b/svl/source/items/itemset.cxx
index 4b2442a925a7..3b8a77f8186f 100644
--- a/svl/source/items/itemset.cxx
+++ b/svl/source/items/itemset.cxx
@@ -47,7 +47,7 @@ size_t getAllocatedSfxPoolItemHolderCount() { return nAllocatedSfxPoolItemHolder
 size_t getUsedSfxPoolItemHolderCount() { return nUsedSfxPoolItemHolderCount; }
 #endif
 // NOTE: Only needed for one Item in SC (see notes below for
-// ScPatternAttr/ATTR_PATTERN). Still keep it so that when errors
+// ScPatternAttr). Still keep it so that when errors
 // come up to this change be able to quickly check using the
 // fallback flag 'ITEM_CLASSIC_MODE'
 static bool g_bItemClassicMode(getenv("ITEM_CLASSIC_MODE"));
@@ -266,11 +266,6 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
         // just use pSource which equals INVALID_POOL_ITEM
         return pSource;
 
-    if (pSource->isExceptionalSCItem() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
-        // exceptional handling for *some* items, see SC
-        // (do not copy item: use directly, it is a pool default)
-        return pSource;
-
     // CAUTION: static default items are not *that* static as it seems
     // (or: should be). If they are freed with the Pool (see
     // ::ReleaseDefaults) they will be deleted. Same is true for
@@ -373,7 +368,7 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
     //   'StaticDefault' and gets changed (..?)
 
     // only do this if classic mode or required (calls from Pool::Direct*)
-    while(g_bItemClassicMode || pSource->isExceptionalSCItem())
+    while(g_bItemClassicMode)
     {
         if (!pTargetPool->Shareable_Impl(nIndex))
             // not shareable, so no need to search for identical item
@@ -424,29 +419,20 @@ SfxPoolItem const* implCreateItemEntry(SfxItemPool& rPool, SfxPoolItem const* pS
     // increase RefCnt 0->1
     pSource->AddRef();
 
-    // Unfortunately e,g, SC does 'special' things for some new Items,
-    // so we need to give the opportunity for this. To limit this to
-    // the needed cases, use m_bExceptionalSCItem flag at item
-    if (pSource->isExceptionalSCItem())
-        pMasterPool->newItem_Callback(*pSource);
-
     // try to register @Pool (only needed if not yet registered)
     if (!pSource->isRegisteredAtPool())
     {
-        bool bRegisterAtPool(pSource->isExceptionalSCItem());
+        bool bRegisterAtPool(false);
 
-        if (!bRegisterAtPool)
+        if (g_bItemClassicMode)
         {
-            if (g_bItemClassicMode)
-            {
-                // in classic mode register only/all shareable items
-                bRegisterAtPool = pTargetPool->Shareable_Impl(nIndex);
-            }
-            else
-            {
-                // in new mode register only/all items marked as need to be registered
-                bRegisterAtPool = pTargetPool->NeedsPoolRegistration_Impl(nIndex);
-            }
+            // in classic mode register only/all shareable items
+            bRegisterAtPool = pTargetPool->Shareable_Impl(nIndex);
+        }
+        else
+        {
+            // in new mode register only/all items marked as need to be registered
+            bRegisterAtPool = pTargetPool->NeedsPoolRegistration_Impl(nIndex);
         }
 
         if (bRegisterAtPool)
@@ -466,11 +452,6 @@ void implCleanupItemEntry(SfxItemPool& rPool, SfxPoolItem const* pSource)
         // nothing to do for invalid item entries
         return;
 
-    if (pSource->isExceptionalSCItem() && rPool.GetMasterPool()->newItem_UseDirect(*pSource))
-        // exceptional handling for *some* items, see SC
-        // do not delete Item, it is a pool default
-        return;
-
     if (0 == pSource->Which())
     {
         // de-register when registered @pool
diff --git a/svl/source/items/poolitem.cxx b/svl/source/items/poolitem.cxx
index ae7c97136c48..b563abfd8b7c 100644
--- a/svl/source/items/poolitem.cxx
+++ b/svl/source/items/poolitem.cxx
@@ -498,7 +498,6 @@ SfxPoolItem::SfxPoolItem(sal_uInt16 const nWhich)
     , m_bStaticDefault(false)
     , m_bPoolDefault(false)
     , m_bRegisteredAtPool(false)
-    , m_bExceptionalSCItem(false)
     , m_bIsSetItem(false)
 #ifdef DBG_UTIL
     , m_bDeleted(false)