tdf#117066 Saving ODT document with ~1500 bookmarks is slow, part 5
Individually, these don't make much difference, but they add up
to a halving the time to save on my machine.
OStorageImpl is spending time iterating over its m_aChildrenVector to
find stuff by name, so just use a std::unordered_map.
Also return iterator from FindElement, so we can avoid searching the map
twice.
This was probably the biggest win.
Change-Id: I30776bad5377d14144fc7a47e86527e2cdb62a83
Reviewed-on: https://gerrit.libreoffice.org/70313
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
diff --git a/package/source/xstor/xstorage.cxx b/package/source/xstor/xstorage.cxx
index fe12788..c963820 100644
--- a/package/source/xstor/xstorage.cxx
+++ b/package/source/xstor/xstorage.cxx
@@ -322,8 +322,9 @@
m_pParent = nullptr;
}
std::for_each(m_aChildrenVector.begin(), m_aChildrenVector.end(), std::default_delete<SotElement_Impl>());
m_aChildrenVector.clear();
for (auto & pair : m_aChildrenMap)
delete pair.second;
m_aChildrenMap.clear();
std::for_each(m_aDeletedVector.begin(), m_aDeletedVector.end(), std::default_delete<SotElement_Impl>());
m_aDeletedVector.clear();
@@ -485,12 +486,12 @@
throw embed::InvalidStorageException( THROW_WHERE );
}
SotElementVector_Impl& OStorage_Impl::GetChildrenVector()
bool OStorage_Impl::HasChildren()
{
::osl::MutexGuard aGuard( m_xMutex->GetMutex() );
ReadContents();
return m_aChildrenVector;
return !m_aChildrenMap.empty();
}
void OStorage_Impl::GetStorageProperties()
@@ -612,7 +613,8 @@
xNewElement->m_bIsRemoved = true;
}
m_aChildrenVector.push_back(xNewElement.release());
OUString name = xNewElement->m_aName; // because we're calling release in the next line
m_aChildrenMap.emplace(name, xNewElement.release());
}
}
catch( const container::NoSuchElementException& rNoSuchElementException )
@@ -652,8 +654,9 @@
if ( !m_xPackageFolder.is() )
throw embed::InvalidStorageException( THROW_WHERE );
for ( auto& pElement : m_aChildrenVector )
for ( auto& pair : m_aChildrenMap )
{
auto & pElement = pair.second;
if ( !pElement->m_bIsRemoved )
CopyStorageElement( pElement, xDest, pElement->m_aName, bDirect );
}
@@ -1021,34 +1024,34 @@
m_aDeletedVector.clear();
// remove removed elements
SotElementVector_Impl::iterator pElementIter = m_aChildrenVector.begin();
while ( pElementIter != m_aChildrenVector.end() )
auto mapIter = m_aChildrenMap.begin();
while ( mapIter != m_aChildrenMap.end() )
{
// renamed and inserted elements must be really inserted to package later
// since thay can conflict with removed elements
if ( (*pElementIter)->m_bIsRemoved )
auto & pElement = mapIter->second;
if ( pElement->m_bIsRemoved )
{
if ( m_nStorageType == embed::StorageFormats::OFOPXML && !(*pElementIter)->m_bIsStorage )
RemoveStreamRelInfo( (*pElementIter)->m_aOriginalName );
if ( m_nStorageType == embed::StorageFormats::OFOPXML && !pElement->m_bIsStorage )
RemoveStreamRelInfo( pElement->m_aOriginalName );
// the removed elements are not in new temporary storage
if ( m_bCommited || m_bIsRoot )
xNewPackageFolder->removeByName( (*pElementIter)->m_aOriginalName );
xNewPackageFolder->removeByName( pElement->m_aOriginalName );
delete *pElementIter;
pElementIter = m_aChildrenVector.erase(pElementIter);
delete pElement;
mapIter = m_aChildrenMap.erase(mapIter);
}
else
++pElementIter;
++mapIter;
}
// there should be no more deleted elements
for ( auto& pElement : m_aChildrenVector )
for ( auto& pair : m_aChildrenMap )
{
// if it is a 'duplicate commit' inserted elements must be really inserted to package later
// since thay can conflict with renamed elements
auto & pElement = pair.second;
if ( !pElement->m_bIsInserted )
{
// for now stream is opened in direct mode that means that in case
@@ -1117,8 +1120,9 @@
}
}
for ( auto& pElement : m_aChildrenVector )
for ( auto& pair : m_aChildrenMap )
{
auto & pElement = pair.second;
// now inserted elements can be inserted to the package
if ( pElement->m_bIsInserted )
{
@@ -1212,29 +1216,35 @@
// all the children must be removed
// they will be created later on demand
SotElementVector_Impl::iterator pElementIter = m_aChildrenVector.begin();
while ( pElementIter != m_aChildrenVector.end() )
// rebuild the map - cannot do it in-place, because we're changing some of the key values
std::unordered_map<OUString, SotElement_Impl*> oldMap;
std::swap(oldMap, m_aChildrenMap);
auto pElementIter = oldMap.begin();
while ( pElementIter != oldMap.end() )
{
if ( (*pElementIter)->m_bIsInserted )
auto & pElement = pElementIter->second;
if ( pElement->m_bIsInserted )
{
delete *pElementIter;
pElementIter = m_aChildrenVector.erase(pElementIter);
delete pElement;
}
else
{
ClearElement( *pElementIter );
ClearElement( pElement );
(*pElementIter)->m_aName = (*pElementIter)->m_aOriginalName;
(*pElementIter)->m_bIsRemoved = false;
pElement->m_aName = pElement->m_aOriginalName;
pElement->m_bIsRemoved = false;
++pElementIter;
m_aChildrenMap.emplace(pElement->m_aName, pElement);
}
++pElementIter;
}
// return replaced removed elements
for ( auto& pDeleted : m_aDeletedVector )
{
m_aChildrenVector.push_back( pDeleted );
// use original name as key, because we're updating m_aName below
m_aChildrenMap.emplace( pDeleted->m_aOriginalName, pDeleted );
ClearElement( pDeleted );
@@ -1282,18 +1292,27 @@
SotElement_Impl* OStorage_Impl::FindElement( const OUString& rName )
{
::osl::MutexGuard aGuard( m_xMutex->GetMutex() );
auto it = FindElementIt(rName);
return it == m_aChildrenMap.end() ? nullptr : it->second;
}
std::unordered_map<OUString, SotElement_Impl*>::iterator OStorage_Impl::FindElementIt( const OUString& rName )
{
SAL_WARN_IF( rName.isEmpty(), "package.xstor", "Name is empty!" );
::osl::MutexGuard aGuard( m_xMutex->GetMutex() );
ReadContents();
auto pElementIter = std::find_if(m_aChildrenVector.begin(), m_aChildrenVector.end(),
[&rName](const SotElement_Impl* pElement) { return pElement->m_aName == rName && !pElement->m_bIsRemoved; });
if (pElementIter != m_aChildrenVector.end())
return *pElementIter;
auto pElementIter = m_aChildrenMap.find(rName);
if (pElementIter == m_aChildrenMap.end())
return m_aChildrenMap.end();
if (pElementIter->second->m_bIsRemoved)
return m_aChildrenMap.end();
return nullptr;
return pElementIter;
}
SotElement_Impl* OStorage_Impl::InsertStream( const OUString& aName, bool bEncr )
@@ -1321,7 +1340,7 @@
SotElement_Impl* pNewElement = InsertElement( aName, false );
pNewElement->m_xStream.reset(new OWriteStream_Impl(this, xPackageSubStream, m_xPackage, m_xContext, bEncr, m_nStorageType, true));
m_aChildrenVector.push_back( pNewElement );
m_aChildrenMap.emplace( pNewElement->m_aName, pNewElement );
m_bIsModified = true;
m_bBroadcastModified = true;
@@ -1360,7 +1379,7 @@
// the stream is inserted and must be treated as a committed one
pNewElement->m_xStream->SetToBeCommited();
m_aChildrenVector.push_back( pNewElement );
m_aChildrenMap.emplace( pNewElement->m_aName, pNewElement );
m_bIsModified = true;
m_bBroadcastModified = true;
}
@@ -1394,30 +1413,26 @@
pNewElement->m_xStorage = CreateNewStorageImpl(nStorageMode);
m_aChildrenVector.push_back( pNewElement );
m_aChildrenMap.emplace( pNewElement->m_aName, pNewElement );
return pNewElement;
}
SotElement_Impl* OStorage_Impl::InsertElement( const OUString& aName, bool bIsStorage )
{
OSL_ENSURE( FindElement( aName ) == nullptr, "Should not try to insert existing element" );
::osl::MutexGuard aGuard( m_xMutex->GetMutex() );
SotElement_Impl* pDeletedElm = nullptr;
for ( const auto& pElement : m_aChildrenVector )
auto it = m_aChildrenMap.find(aName);
if (it != m_aChildrenMap.end())
{
if ( pElement->m_aName == aName )
auto pElement = it->second;
SAL_WARN_IF( !pElement->m_bIsRemoved, "package.xstor", "Try to insert an element instead of existing one!" );
if ( pElement->m_bIsRemoved )
{
SAL_WARN_IF( !pElement->m_bIsRemoved, "package.xstor", "Try to insert an element instead of existing one!" );
if ( pElement->m_bIsRemoved )
{
SAL_WARN_IF( pElement->m_bIsInserted, "package.xstor", "Inserted elements must be deleted immediately!" );
pDeletedElm = pElement;
break;
}
SAL_WARN_IF( pElement->m_bIsInserted, "package.xstor", "Inserted elements must be deleted immediately!" );
pDeletedElm = pElement;
}
}
@@ -1428,9 +1443,7 @@
else
OpenSubStream( pDeletedElm );
m_aChildrenVector.erase(
std::remove(m_aChildrenVector.begin(), m_aChildrenVector.end(), pDeletedElm),
m_aChildrenVector.end());
m_aChildrenMap.erase(it);
m_aDeletedVector.push_back( pDeletedElm );
}
@@ -1488,12 +1501,13 @@
ReadContents();
sal_uInt32 nSize = m_aChildrenVector.size();
sal_uInt32 nSize = m_aChildrenMap.size();
uno::Sequence< OUString > aElementNames( nSize );
sal_uInt32 nInd = 0;
for ( const auto& pElement : m_aChildrenVector )
for ( const auto& pair : m_aChildrenMap )
{
auto pElement = pair.second;
if ( !pElement->m_bIsRemoved )
aElementNames[nInd++] = pElement->m_aName;
}
@@ -1502,12 +1516,9 @@
return aElementNames;
}
void OStorage_Impl::RemoveElement( SotElement_Impl* pElement )
std::unordered_map<OUString, SotElement_Impl*>::iterator OStorage_Impl::RemoveElement( std::unordered_map<OUString, SotElement_Impl*>::iterator pElementIter )
{
SAL_WARN_IF( !pElement, "package.xstor", "Element must be provided!" );
if ( !pElement )
return;
auto pElement = pElementIter->second;
if ( (pElement->m_xStorage && ( pElement->m_xStorage->m_pAntiImpl || !pElement->m_xStorage->m_aReadOnlyWrapVector.empty() ))
|| (pElement->m_xStream && ( pElement->m_xStream->m_pAntiImpl || !pElement->m_xStream->m_aInputStreamsVector.empty() )) )
@@ -1515,13 +1526,15 @@
if ( pElement->m_bIsInserted )
{
delete pElement;
m_aChildrenVector.erase(std::remove(m_aChildrenVector.begin(), m_aChildrenVector.end(), pElement), m_aChildrenVector.end());
delete pElementIter->second;
return m_aChildrenMap.erase(pElementIter);
}
else
{
pElement->m_bIsRemoved = true;
ClearElement( pElement );
++pElementIter;
return pElementIter;
}
// TODO/OFOPXML: the rel stream should be removed as well
@@ -2384,10 +2397,9 @@
if ( nStorageMode & embed::ElementModes::TRUNCATE )
{
for ( SotElement_Impl* pElementToDel : pElement->m_xStorage->m_aChildrenVector )
{
m_pImpl->RemoveElement( pElementToDel );
}
auto pElementToDelIter = pElement->m_xStorage->m_aChildrenMap.begin();
while (pElementToDelIter != pElement->m_xStorage->m_aChildrenMap.end())
pElementToDelIter = m_pImpl->RemoveElement( pElementToDelIter );
}
}
}
@@ -2793,12 +2805,11 @@
try
{
SotElement_Impl* pElement = m_pImpl->FindElement(aElementName);
if (!pElement)
auto pElementIter = m_pImpl->FindElementIt(aElementName);
if ( pElementIter == m_pImpl->m_aChildrenMap.end() )
throw container::NoSuchElementException(THROW_WHERE); //???
m_pImpl->RemoveElement(pElement);
m_pImpl->RemoveElement(pElementIter);
m_pImpl->m_bIsModified = true;
m_pImpl->m_bBroadcastModified = true;
@@ -2878,11 +2889,14 @@
if (pRefElement)
throw container::ElementExistException(THROW_WHERE); //???
SotElement_Impl* pElement = m_pImpl->FindElement(aElementName);
if (!pElement)
auto pElementIt = m_pImpl->FindElementIt( aElementName );
if ( pElementIt == m_pImpl->m_aChildrenMap.end() )
throw container::NoSuchElementException(THROW_WHERE); //???
auto pElement = pElementIt->second;
pElement->m_aName = aNewName;
m_pImpl->m_aChildrenMap.erase( pElementIt );
m_pImpl->m_aChildrenMap.emplace(pElement->m_aName, pElement);
m_pImpl->m_bIsModified = true;
m_pImpl->m_bBroadcastModified = true;
@@ -3052,17 +3066,17 @@
try
{
SotElement_Impl* pElement = m_pImpl->FindElement(aElementName);
if (!pElement)
auto pElementIter = m_pImpl->FindElementIt( aElementName );
if ( pElementIter == m_pImpl->m_aChildrenMap.end() )
throw container::NoSuchElementException(THROW_WHERE); //???
uno::Reference<XNameAccess> xNameAccess(xDest, uno::UNO_QUERY_THROW);
if (xNameAccess->hasByName(aNewName))
throw container::ElementExistException(THROW_WHERE);
m_pImpl->CopyStorageElement(pElement, xDest, aNewName, false);
m_pImpl->CopyStorageElement(pElementIter->second, xDest, aNewName, false);
m_pImpl->RemoveElement(pElement);
m_pImpl->RemoveElement(pElementIter);
m_pImpl->m_bIsModified = true;
m_pImpl->m_bBroadcastModified = true;
@@ -3612,8 +3626,9 @@
}
bool bThrow = std::any_of(
m_pImpl->m_aChildrenVector.begin(), m_pImpl->m_aChildrenVector.end(),
[](const SotElement_Impl* pElement) {
m_pImpl->m_aChildrenMap.begin(), m_pImpl->m_aChildrenMap.end(),
[](decltype(m_pImpl->m_aChildrenMap)::value_type const & pair) {
auto pElement = pair.second;
return (pElement->m_xStorage
&& (pElement->m_xStorage->m_pAntiImpl
|| !pElement->m_xStorage->m_aReadOnlyWrapVector.empty()))
@@ -3920,7 +3935,7 @@
try
{
return ( !m_pImpl->GetChildrenVector().empty() );
return m_pImpl->HasChildren();
}
catch( const uno::RuntimeException& rRuntimeException )
{
diff --git a/package/source/xstor/xstorage.hxx b/package/source/xstor/xstorage.hxx
index a365dfd..85bb2c1 100644
--- a/package/source/xstor/xstorage.hxx
+++ b/package/source/xstor/xstorage.hxx
@@ -139,7 +139,7 @@
return m_nModifiedListenerCount > 0 && m_pAntiImpl != nullptr;
}
SotElementVector_Impl m_aChildrenVector;
std::unordered_map<OUString, SotElement_Impl*> m_aChildrenMap;
SotElementVector_Impl m_aDeletedVector;
css::uno::Reference< css::container::XNameContainer > m_xPackageFolder;
@@ -205,7 +205,7 @@
void ReadContents();
void ReadRelInfoIfNecessary();
SotElementVector_Impl& GetChildrenVector();
bool HasChildren();
void GetStorageProperties();
css::uno::Sequence< css::uno::Sequence< css::beans::StringPair > > GetAllRelationshipsIfAny();
@@ -229,6 +229,7 @@
bool bDirect );
SotElement_Impl* FindElement( const OUString& rName );
std::unordered_map<OUString, SotElement_Impl*>::iterator FindElementIt( const OUString& rName );
SotElement_Impl* InsertStream( const OUString& aName, bool bEncr );
void InsertRawStream( const OUString& aName, const css::uno::Reference< css::io::XInputStream >& xInStream );
@@ -242,7 +243,7 @@
css::uno::Sequence< OUString > GetElementNames();
void RemoveElement( SotElement_Impl* pElement );
std::unordered_map<OUString, SotElement_Impl*>::iterator RemoveElement( std::unordered_map<OUString, SotElement_Impl*>::iterator pElement );
static void ClearElement( SotElement_Impl* pElement );
/// @throws css::embed::InvalidStorageException
