editeng/source/items/frmitems.cxx | 10 +------ editeng/source/items/paraitem.cxx | 2 - editeng/source/items/textitem.cxx | 53 +++++++++++++++++++++++++++----------- include/editeng/boxitem.hxx | 3 -- include/svl/poolitem.hxx | 33 +++++++++++------------ svl/source/items/cenumitm.cxx | 7 ++--- svl/source/items/globalpool.cxx | 26 ++++++++---------- sw/source/core/layout/atrfrm.cxx | 30 +++++++++++++++++---- 8 files changed, 97 insertions(+), 67 deletions(-)
New commits: commit ece698a575710ac7de0c628ef7b8dc7f59e03108 Author: Armin Le Grand (allotropia) <[email protected]> AuthorDate: Wed Jul 10 15:54:40 2024 +0200 Commit: Armin Le Grand <[email protected]> CommitDate: Thu Jul 11 11:01:01 2024 +0200 ITEM: replaced typeid/hash_code with SfxItemType for the ItemInstanceManager the identification was done using typeid/hash_code, but that has problems - it is only defined to be identical for two instances of the same type, but other types can have the same code. Thanks to Noel to find that out, that is not reliable to be used. In the meantime we have SfxItemType to identify Item instances, so I changed the code to use that. The master had five additionally added Items that use the (2a) method of this mechanism (see comments in svl/source/items/globalpool.cxx for that). The gloal is to use as less as possible of this Items in that mechanism, so I checked. For four of these hashing was used, so the mem/runtime aspect is okay -> access is fast enough to prefer mem over runtime. Adding hashed Items (or any other fast mechanism) is always okay. One did not have that (SvxBoxItem) and used just the fallback ItemInstanceManager, so I removed it. We now have 18 Items for which that mechanism is initialized immediately. Noel also already moved the countdown for starting to use the mechanism in case of default handling (case (1), NUMBER_OF_UNSHARED_INSTANCES) to the unorderd_set. someting I had already planned to do, too - thanks! Change-Id: Icf6f427e5ea64672f385357aaad75bb5b7fcbf98 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/170314 Reviewed-by: Noel Grandin <[email protected]> Tested-by: Jenkins Reviewed-by: Armin Le Grand <[email protected]> diff --git a/editeng/source/items/frmitems.cxx b/editeng/source/items/frmitems.cxx index 0ecf0b6afed1..d2a788370e77 100644 --- a/editeng/source/items/frmitems.cxx +++ b/editeng/source/items/frmitems.cxx @@ -3263,12 +3263,6 @@ SvxBoxInfoItem::SvxBoxInfoItem(const sal_uInt16 nId) ResetFlags(); } -ItemInstanceManager* SvxBoxItem::getItemInstanceManager() const -{ - static DefaultItemInstanceManager aInstanceManager(typeid(SvxBoxItem).hash_code()); - return &aInstanceManager; -} - SvxBoxInfoItem::SvxBoxInfoItem( const SvxBoxInfoItem& rCopy ) : SfxPoolItem(rCopy) , mpHorizontalLine(rCopy.mpHorizontalLine ? new SvxBorderLine(*rCopy.mpHorizontalLine) : nullptr) @@ -3985,7 +3979,7 @@ void SvxLineItem::SetLine( const SvxBorderLine* pNew ) ItemInstanceManager* SvxBrushItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxBrushItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -4602,7 +4596,7 @@ void SvxBrushItem::dumpAsXml(xmlTextWriterPtr pWriter) const ItemInstanceManager* SvxFrameDirectionItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxFrameDirectionItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } diff --git a/editeng/source/items/paraitem.cxx b/editeng/source/items/paraitem.cxx index 06ab7170341d..87d892dc3409 100644 --- a/editeng/source/items/paraitem.cxx +++ b/editeng/source/items/paraitem.cxx @@ -341,7 +341,7 @@ void SvxLineSpacingItem::SetEnumValue( sal_uInt16 nVal ) ItemInstanceManager* SvxAdjustItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxAdjustItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } diff --git a/editeng/source/items/textitem.cxx b/editeng/source/items/textitem.cxx index 65b83fbff620..a44ca0bfe612 100644 --- a/editeng/source/items/textitem.cxx +++ b/editeng/source/items/textitem.cxx @@ -161,7 +161,7 @@ bool SvxFontListItem::GetPresentation namespace { - class SvxFontItemInstanceManager : public TypeSpecificItemInstanceManager<SvxFontItem> + class SvxFontItemInstanceManager : public HashedItemInstanceManager { protected: virtual size_t hashCode(const SfxPoolItem& rItem) const override @@ -176,12 +176,17 @@ namespace o3tl::hash_combine(seed, rFontItem.GetCharSet()); return seed; } + public: + SvxFontItemInstanceManager(SfxItemType aSfxItemType) + : HashedItemInstanceManager(aSfxItemType) + { + } }; } ItemInstanceManager* SvxFontItem::getItemInstanceManager() const { - static SvxFontItemInstanceManager aInstanceManager; + static SvxFontItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -411,23 +416,29 @@ void SvxFontItem::dumpAsXml(xmlTextWriterPtr pWriter) const namespace { - class SvxPostureItemInstanceManager : public TypeSpecificItemInstanceManager<SvxPostureItem> + class SvxPostureItemInstanceManager : public HashedItemInstanceManager { protected: virtual size_t hashCode(const SfxPoolItem& rItem) const override { auto const & rPostureItem = static_cast<const SvxPostureItem&>(rItem); std::size_t seed(0); + o3tl::hash_combine(seed, rItem.Which()); o3tl::hash_combine(seed, rPostureItem.Which()); o3tl::hash_combine(seed, rPostureItem. GetEnumValue()); return seed; } + public: + SvxPostureItemInstanceManager(SfxItemType aSfxItemType) + : HashedItemInstanceManager(aSfxItemType) + { + } }; } ItemInstanceManager* SvxPostureItem::getItemInstanceManager() const { - static SvxPostureItemInstanceManager aInstanceManager; + static SvxPostureItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -549,7 +560,7 @@ void SvxPostureItem::dumpAsXml(xmlTextWriterPtr pWriter) const ItemInstanceManager* SvxWeightItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxWeightItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -679,25 +690,31 @@ void SvxWeightItem::dumpAsXml(xmlTextWriterPtr pWriter) const namespace { - class SvxFontHeightItemInstanceManager : public TypeSpecificItemInstanceManager<SvxFontHeightItem> + class SvxFontHeightItemInstanceManager : public HashedItemInstanceManager { protected: virtual size_t hashCode(const SfxPoolItem& rItem) const override { auto const & rFontHeightItem = static_cast<const SvxFontHeightItem&>(rItem); std::size_t seed(0); + o3tl::hash_combine(seed, rItem.Which()); o3tl::hash_combine(seed, rFontHeightItem.Which()); o3tl::hash_combine(seed, rFontHeightItem.GetHeight()); o3tl::hash_combine(seed, rFontHeightItem.GetProp()); o3tl::hash_combine(seed, rFontHeightItem.GetPropUnit()); return seed; } + public: + SvxFontHeightItemInstanceManager(SfxItemType aSfxItemType) + : HashedItemInstanceManager(aSfxItemType) + { + } }; } ItemInstanceManager* SvxFontHeightItem::getItemInstanceManager() const { - static SvxFontHeightItemInstanceManager aInstanceManager; + static SvxFontHeightItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -1196,7 +1213,7 @@ bool SvxTextLineItem::operator==( const SfxPoolItem& rItem ) const ItemInstanceManager* SvxUnderlineItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxUnderlineItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -1243,7 +1260,7 @@ OUString SvxUnderlineItem::GetValueTextByPos( sal_uInt16 nPos ) const ItemInstanceManager* SvxOverlineItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxOverlineItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -1290,7 +1307,7 @@ OUString SvxOverlineItem::GetValueTextByPos( sal_uInt16 nPos ) const ItemInstanceManager* SvxCrossedOutItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxCrossedOutItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -2093,7 +2110,7 @@ bool SvxEscapementItem::PutValue( const uno::Any& rVal, sal_uInt8 nMemberId ) ItemInstanceManager* SvxLanguageItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxLanguageItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -2241,7 +2258,7 @@ bool SvxBlinkItem::GetPresentation ItemInstanceManager* SvxEmphasisMarkItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxEmphasisMarkItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -2682,7 +2699,7 @@ bool SvxCharScaleWidthItem::QueryValue( uno::Any& rVal, sal_uInt8 /*nMemberId*/ ItemInstanceManager* SvxCharReliefItem::getItemInstanceManager() const { - static DefaultItemInstanceManager aInstanceManager(typeid(SvxCharReliefItem).hash_code()); + static DefaultItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -2971,23 +2988,29 @@ void GetDefaultFonts( SvxFontItem& rLatin, SvxFontItem& rAsian, SvxFontItem& rCo namespace { - class SvxRsidItemInstanceManager : public TypeSpecificItemInstanceManager<SvxRsidItem> + class SvxRsidItemInstanceManager : public HashedItemInstanceManager { protected: virtual size_t hashCode(const SfxPoolItem& rItem) const override { auto const & rRsidItem = static_cast<const SvxRsidItem&>(rItem); std::size_t seed(0); + o3tl::hash_combine(seed, rItem.Which()); o3tl::hash_combine(seed, rRsidItem.Which()); o3tl::hash_combine(seed, rRsidItem.GetValue()); return seed; } + public: + SvxRsidItemInstanceManager(SfxItemType aSfxItemType) + : HashedItemInstanceManager(aSfxItemType) + { + } }; } ItemInstanceManager* SvxRsidItem::getItemInstanceManager() const { - static SvxRsidItemInstanceManager aInstanceManager; + static SvxRsidItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } diff --git a/include/editeng/boxitem.hxx b/include/editeng/boxitem.hxx index 7afb8f2bf821..6ceff99268c3 100644 --- a/include/editeng/boxitem.hxx +++ b/include/editeng/boxitem.hxx @@ -73,9 +73,6 @@ class EDITENG_DLLPUBLIC SvxBoxItem final : public SfxPoolItem void tryMigrateComplexColor(SvxBoxItemLine eLine); -protected: - virtual ItemInstanceManager* getItemInstanceManager() const override; - public: static SfxPoolItem* CreateDefault(); diff --git a/include/svl/poolitem.hxx b/include/svl/poolitem.hxx index c993039afa7b..88e7ba855318 100644 --- a/include/svl/poolitem.hxx +++ b/include/svl/poolitem.hxx @@ -712,17 +712,17 @@ class SVL_DLLPUBLIC ItemInstanceManager friend SfxPoolItem const* implCreateItemEntry(SfxItemPool&, SfxPoolItem const*, bool); friend void implCleanupItemEntry(SfxPoolItem const*); - // Define for which class to register (usually typeid().hash_code()). - std::size_t m_aClassHash; + // Define for which SfxItemType to register + SfxItemType m_aSfxItemType; public: - ItemInstanceManager(const std::size_t aClassHash) - : m_aClassHash(aClassHash) + ItemInstanceManager(SfxItemType aSfxItemType) + : m_aSfxItemType(aSfxItemType) { } virtual ~ItemInstanceManager() = default; - std::size_t getClassHash() const { return m_aClassHash; } + SfxItemType ItemType() const { return m_aSfxItemType; } private: // standard interface, accessed exclusively @@ -745,8 +745,9 @@ class SVL_DLLPUBLIC DefaultItemInstanceManager : public ItemInstanceManager std::unordered_map<sal_uInt16, std::unordered_set<const SfxPoolItem*>> maRegistered; public: - DefaultItemInstanceManager(const std::size_t aClassHash) - : ItemInstanceManager(aClassHash) + DefaultItemInstanceManager(SfxItemType aSfxItemType) + : ItemInstanceManager(aSfxItemType) + , maRegistered() { } @@ -760,12 +761,16 @@ private: Utility template to reduce boilerplate code when creating item instance managers for specific PoolItem subclasses. */ -template<class T> -class TypeSpecificItemInstanceManager : public ItemInstanceManager +class HashedItemInstanceManager : public ItemInstanceManager { + std::unordered_map<size_t, const SfxPoolItem*> maRegistered; + +protected: + virtual size_t hashCode(const SfxPoolItem&) const = 0; + public: - TypeSpecificItemInstanceManager() - : ItemInstanceManager(typeid(T).hash_code()) + HashedItemInstanceManager(SfxItemType aSfxItemType) + : ItemInstanceManager(aSfxItemType) { } @@ -786,12 +791,6 @@ public: { maRegistered.erase(hashCode(rItem)); } - -protected: - virtual size_t hashCode(const SfxPoolItem&) const = 0; - -private: - std::unordered_map<size_t, const SfxPoolItem*> maRegistered; }; diff --git a/svl/source/items/cenumitm.cxx b/svl/source/items/cenumitm.cxx index e5e53e18d0b7..dbf3b4baa75e 100644 --- a/svl/source/items/cenumitm.cxx +++ b/svl/source/items/cenumitm.cxx @@ -92,8 +92,9 @@ namespace SfxBoolItemMap maRegistered; public: - SfxBoolItemInstanceManager() - : ItemInstanceManager(typeid(SfxBoolItem).hash_code()) + SfxBoolItemInstanceManager(SfxItemType aSfxItemType) + : ItemInstanceManager(aSfxItemType) + , maRegistered() { } @@ -158,7 +159,7 @@ namespace ItemInstanceManager* SfxBoolItem::getItemInstanceManager() const { - static SfxBoolItemInstanceManager aInstanceManager; + static SfxBoolItemInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } diff --git a/svl/source/items/globalpool.cxx b/svl/source/items/globalpool.cxx index ba16d3874429..c5ecfba62c2a 100644 --- a/svl/source/items/globalpool.cxx +++ b/svl/source/items/globalpool.cxx @@ -125,7 +125,7 @@ void DefaultItemInstanceManager::remove(const SfxPoolItem& rItem) // ignored and start sharing ALL Item derivations instantly. class InstanceManagerHelper { - typedef std::unordered_map<std::size_t, std::pair<sal_uInt16, DefaultItemInstanceManager*>> + typedef std::unordered_map<SfxItemType, std::pair<sal_uInt16, DefaultItemInstanceManager*>> managerTypeMap; managerTypeMap maManagerPerType; @@ -153,15 +153,14 @@ public: // hopefully fastest) incarnations ItemInstanceManager* pManager(rItem.getItemInstanceManager()); - // Check for correct hash, there may be derivations of that class. + // Check for correct SfxItemType, there may be derivations of that class. // Note that Managers from the Items are *not* added to local list, - // they are expected to be static instances at the Items - const std::size_t aHash(typeid(rItem).hash_code()); - if (nullptr != pManager && pManager->getClassHash() == aHash) + // they are expected to be static instances at the Items for fastest access + if (nullptr != pManager && pManager->ItemType() == rItem.ItemType()) return pManager; // check local memory for existing entry - managerTypeMap::iterator aHit(maManagerPerType.find(aHash)); + managerTypeMap::iterator aHit(maManagerPerType.find(rItem.ItemType())); // no instance yet if (aHit == maManagerPerType.end()) @@ -170,14 +169,14 @@ public: if (g_bShareImmediately) { // create, insert locally and immediately start sharing - DefaultItemInstanceManager* pNew(new DefaultItemInstanceManager(aHash)); - maManagerPerType.insert({ aHash, std::make_pair(0, pNew) }); + DefaultItemInstanceManager* pNew(new DefaultItemInstanceManager(rItem.ItemType())); + maManagerPerType.insert({ rItem.ItemType(), std::make_pair(0, pNew) }); return pNew; } // start countdown from NUMBER_OF_UNSHARED_INSTANCES until zero is reached maManagerPerType.insert( - { aHash, std::make_pair(NUMBER_OF_UNSHARED_INSTANCES, nullptr) }); + { rItem.ItemType(), std::make_pair(NUMBER_OF_UNSHARED_INSTANCES, nullptr) }); return nullptr; } @@ -195,7 +194,7 @@ public: // here the countdown is zero and there is not yet a ItemInstanceManager // incarnated. Do so, register and return it assert(nullptr == aHit->second.second); - DefaultItemInstanceManager* pNew(new DefaultItemInstanceManager(aHash)); + DefaultItemInstanceManager* pNew(new DefaultItemInstanceManager(rItem.ItemType())); aHit->second.second = pNew; return pNew; @@ -216,15 +215,14 @@ public: // hopefully fastest) incarnations ItemInstanceManager* pManager(rItem.getItemInstanceManager()); - // Check for correct hash, there may be derivations of that class. + // Check for correct SfxItemType, there may be derivations of that class. // Note that Managers from the Items are *not* added to local list, // they are expected to be static instances at the Items - const std::size_t aHash(typeid(rItem).hash_code()); - if (nullptr != pManager && pManager->getClassHash() == aHash) + if (nullptr != pManager && pManager->ItemType() == rItem.ItemType()) return pManager; // check local memory for existing entry - managerTypeMap::iterator aHit(maManagerPerType.find(aHash)); + managerTypeMap::iterator aHit(maManagerPerType.find(rItem.ItemType())); if (aHit == maManagerPerType.end()) // no instance yet, return nullptr diff --git a/sw/source/core/layout/atrfrm.cxx b/sw/source/core/layout/atrfrm.cxx index 878e155ce4ce..586c12ed4510 100644 --- a/sw/source/core/layout/atrfrm.cxx +++ b/sw/source/core/layout/atrfrm.cxx @@ -218,13 +218,14 @@ static void lcl_DelHFFormat( SwClient *pToRemove, SwFrameFormat *pFormat ) namespace { - class SwFormatFrameSizeInstanceManager : public TypeSpecificItemInstanceManager<SwFormatFrameSize> + class SwFormatFrameSizeInstanceManager : public HashedItemInstanceManager { protected: virtual size_t hashCode(const SfxPoolItem& rItem) const override { auto const & rFormatItem = static_cast<const SwFormatFrameSize&>(rItem); std::size_t seed(0); + o3tl::hash_combine(seed, rItem.Which()); o3tl::hash_combine(seed, rFormatItem.GetHeightSizeType()); o3tl::hash_combine(seed, rFormatItem.GetWidthSizeType()); o3tl::hash_combine(seed, rFormatItem.GetWidthPercent()); @@ -235,12 +236,17 @@ namespace o3tl::hash_combine(seed, rFormatItem.GetSize().Height()); return seed; } + public: + SwFormatFrameSizeInstanceManager(SfxItemType aSfxItemType) + : HashedItemInstanceManager(aSfxItemType) + { + } }; } ItemInstanceManager* SwFormatFrameSize::getItemInstanceManager() const { - static SwFormatFrameSizeInstanceManager aInstanceManager; + static SwFormatFrameSizeInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -1405,24 +1411,30 @@ void SwFormatSurround::dumpAsXml(xmlTextWriterPtr pWriter) const namespace { - class SwFormatVertOrientInstanceManager : public TypeSpecificItemInstanceManager<SwFormatVertOrient> + class SwFormatVertOrientInstanceManager : public HashedItemInstanceManager { protected: virtual size_t hashCode(const SfxPoolItem& rItem) const override { auto const & rFormatItem = static_cast<const SwFormatVertOrient&>(rItem); std::size_t seed(0); + o3tl::hash_combine(seed, rItem.Which()); o3tl::hash_combine(seed, rFormatItem.GetPos()); o3tl::hash_combine(seed, rFormatItem.GetVertOrient()); o3tl::hash_combine(seed, rFormatItem.GetRelationOrient()); return seed; } + public: + SwFormatVertOrientInstanceManager(SfxItemType aSfxItemType) + : HashedItemInstanceManager(aSfxItemType) + { + } }; } ItemInstanceManager* SwFormatVertOrient::getItemInstanceManager() const { - static SwFormatVertOrientInstanceManager aInstanceManager; + static SwFormatVertOrientInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; } @@ -1520,25 +1532,31 @@ void SwFormatVertOrient::dumpAsXml(xmlTextWriterPtr pWriter) const namespace { - class SwFormatHoriOrientInstanceManager : public TypeSpecificItemInstanceManager<SwFormatHoriOrient> + class SwFormatHoriOrientInstanceManager : public HashedItemInstanceManager { protected: virtual size_t hashCode(const SfxPoolItem& rItem) const override { auto const & rFormatItem = static_cast<const SwFormatHoriOrient&>(rItem); std::size_t seed(0); + o3tl::hash_combine(seed, rItem.Which()); o3tl::hash_combine(seed, rFormatItem.GetPos()); o3tl::hash_combine(seed, rFormatItem.GetHoriOrient()); o3tl::hash_combine(seed, rFormatItem.GetRelationOrient()); o3tl::hash_combine(seed, rFormatItem.IsPosToggle()); return seed; } + public: + SwFormatHoriOrientInstanceManager(SfxItemType aSfxItemType) + : HashedItemInstanceManager(aSfxItemType) + { + } }; } ItemInstanceManager* SwFormatHoriOrient::getItemInstanceManager() const { - static SwFormatHoriOrientInstanceManager aInstanceManager; + static SwFormatHoriOrientInstanceManager aInstanceManager(ItemType()); return &aInstanceManager; }
