[Libreoffice-commits] core.git: sc/inc sc/Library_sc.mk sc/source
Libreoffice Gerrit user
logerrit at kemper.freedesktop.org
Wed Oct 10 11:02:25 UTC 2018
sc/Library_sc.mk | 1
sc/inc/document.hxx | 17 ++--
sc/inc/interpretercontext.hxx | 10 +-
sc/inc/lookupcache.hxx | 8 ++
sc/source/core/data/documen2.cxx | 101 +++++++++++------------------
sc/source/core/data/document.cxx | 22 ++++++
sc/source/core/data/formulacell.cxx | 3
sc/source/core/tool/interpr1.cxx | 2
sc/source/core/tool/interpretercontext.cxx | 32 +++++++++
sc/source/core/tool/token.cxx | 3
10 files changed, 120 insertions(+), 79 deletions(-)
New commits:
commit 79449d73900d7a9bf061244d76f5f8eecc441198
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Mon Oct 1 14:26:57 2018 +0200
Commit: Luboš Luňák <l.lunak at collabora.com>
CommitDate: Wed Oct 10 13:01:59 2018 +0200
make VLOOKUP in Calc thread-safe
There is mutex protection needed for accessing the same SvtBroadcaster
when calling StartListeningArea(). Also some of the memory management
and caching needed fixing.
Change-Id: Ia57ed85286cf195521719cfd3b320f73a6342bb1
Reviewed-on: https://gerrit.libreoffice.org/61187
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak at collabora.com>
diff --git a/sc/Library_sc.mk b/sc/Library_sc.mk
index 5695e729ffbe..fe0e4368379b 100644
--- a/sc/Library_sc.mk
+++ b/sc/Library_sc.mk
@@ -247,6 +247,7 @@ $(eval $(call gb_Library_add_exception_objects,sc,\
sc/source/core/tool/interpr6 \
sc/source/core/tool/interpr7 \
sc/source/core/tool/interpr8 \
+ sc/source/core/tool/interpretercontext \
sc/source/core/tool/jumpmatrix \
sc/source/core/tool/listenerquery \
sc/source/core/tool/lookupcache \
diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index 06c155a07aa4..96b28d4d13f0 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -171,7 +171,7 @@ class VirtualDevice;
class ScAutoNameCache;
class ScTemporaryChartLock;
class ScLookupCache;
-struct ScLookupCacheMapImpl;
+struct ScLookupCacheMap;
class SfxUndoManager;
class ScFormulaParserPool;
struct ScClipParam;
@@ -276,11 +276,8 @@ struct ScDocumentThreadSpecific
{
ScRecursionHelper* pRecursionHelper; // information for recursive and iterative cell formulas
- ScLookupCacheMapImpl* pLookupCacheMapImpl; // cache for lookups like VLOOKUP and MATCH
-
ScDocumentThreadSpecific() :
- pRecursionHelper(nullptr),
- pLookupCacheMapImpl(nullptr)
+ pRecursionHelper(nullptr)
{
}
@@ -457,6 +454,9 @@ private:
mutable ScInterpreterContext maInterpreterContext;
+ osl::Mutex mScLookupMutex; // protection for thread-unsafe parts of handling ScLookup
+ std::vector<ScLookupCacheMap*> mThreadStoredScLookupCaches; // temporarily stored for computation threads
+
sal_uInt16 nSrcVer; // file version (load/save)
sal_uInt16 nFormulaTrackCount;
HardRecalcState eHardRecalcState; // off, temporary, eternal
@@ -582,7 +582,8 @@ public:
maInterpreterContext.mpFormatter = GetFormatTable();
return maInterpreterContext;
}
- void MergeBackIntoNonThreadedContext( ScInterpreterContext& threadedContext );
+ void SetupFromNonThreadedContext( ScInterpreterContext& threadedContext, int threadNumber );
+ void MergeBackIntoNonThreadedContext( ScInterpreterContext& threadedContext, int threadNumber );
void SetThreadedGroupCalcInProgress( bool set ) { (void)this; ScGlobal::bThreadedGroupCalcInProgress = set; }
bool IsThreadedGroupCalcInProgress() const { (void)this; return ScGlobal::bThreadedGroupCalcInProgress; }
@@ -1291,7 +1292,7 @@ public:
/** Creates a ScLookupCache cache for the range if it
doesn't already exist. */
- ScLookupCache & GetLookupCache( const ScRange & rRange );
+ ScLookupCache & GetLookupCache( const ScRange & rRange, ScInterpreterContext* pContext );
/** Only ScLookupCache dtor uses RemoveLookupCache(), do
not use elsewhere! */
void RemoveLookupCache( ScLookupCache & rCache );
@@ -2494,6 +2495,8 @@ private:
void EndListeningGroups( const std::vector<ScAddress>& rPosArray );
void SetNeedsListeningGroups( const std::vector<ScAddress>& rPosArray );
+
+ bool RemoveLookupCacheHelper( ScLookupCacheMap* cacheMap, ScLookupCache& rCache );
};
typedef std::unique_ptr<ScDocument, o3tl::default_delete<ScDocument>> ScDocumentUniquePtr;
diff --git a/sc/inc/interpretercontext.hxx b/sc/inc/interpretercontext.hxx
index 8b920472fb44..2855370e7b09 100644
--- a/sc/inc/interpretercontext.hxx
+++ b/sc/inc/interpretercontext.hxx
@@ -18,6 +18,7 @@
class ScDocument;
class SvNumberFormatter;
+struct ScLookupCacheMap;
// SetNumberFormat() is not thread-safe, so calls to it need to be delayed to the main thread.
struct DelayedSetNumberFormat
@@ -33,21 +34,18 @@ struct ScInterpreterContext
size_t mnTokenCachePos;
std::vector<formula::FormulaToken*> maTokens;
std::vector<DelayedSetNumberFormat> maDelayedSetNumberFormat;
+ ScLookupCacheMap* mScLookupCache; // cache for lookups like VLOOKUP and MATCH
ScInterpreterContext(const ScDocument& rDoc, SvNumberFormatter* pFormatter)
: mrDoc(rDoc)
, mpFormatter(pFormatter)
, mnTokenCachePos(0)
, maTokens(TOKEN_CACHE_SIZE, nullptr)
+ , mScLookupCache(nullptr)
{
}
- ~ScInterpreterContext()
- {
- for (auto p : maTokens)
- if (p)
- p->DecRef();
- }
+ ~ScInterpreterContext();
SvNumberFormatter* GetFormatTable() const { return mpFormatter; }
};
diff --git a/sc/inc/lookupcache.hxx b/sc/inc/lookupcache.hxx
index fa55c2bcbced..15ec15f086b1 100644
--- a/sc/inc/lookupcache.hxx
+++ b/sc/inc/lookupcache.hxx
@@ -23,6 +23,7 @@
#include "address.hxx"
#include <svl/listener.hxx>
+#include <memory>
#include <unordered_map>
class ScDocument;
@@ -189,6 +190,13 @@ private:
};
+// Struct because including lookupcache.hxx in document.hxx isn't wanted.
+struct ScLookupCacheMap
+{
+ std::unordered_map< ScRange, std::unique_ptr<ScLookupCache>, ScLookupCache::Hash > aCacheMap;
+};
+
+
#endif
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sc/source/core/data/documen2.cxx b/sc/source/core/data/documen2.cxx
index 653c0d81c931..d6e19993fd79 100644
--- a/sc/source/core/data/documen2.cxx
+++ b/sc/source/core/data/documen2.cxx
@@ -109,17 +109,6 @@
using namespace com::sun::star;
-// pImpl because including lookupcache.hxx in document.hxx isn't wanted, and
-// dtor plus helpers are convenient.
-struct ScLookupCacheMapImpl
-{
- std::unordered_map< ScRange, std::unique_ptr<ScLookupCache>, ScLookupCache::Hash > aCacheMap;
- void clear()
- {
- aCacheMap.clear();
- }
-};
-
ScDocument::ScDocument( ScDocumentMode eMode, SfxObjectShell* pDocShell ) :
mpCellStringPool(new svl::SharedStringPool(*ScGlobal::pCharClass)),
mpDocLinkMgr(new sc::DocumentLinkManager(pDocShell)),
@@ -357,8 +346,7 @@ ScDocument::~ScDocument()
ScAddInListener::RemoveDocument( this );
pChartListenerCollection.reset(); // before pBASM because of potential Listener!
- DELETEZ(maNonThreaded.pLookupCacheMapImpl); // before pBASM because of listeners
- DELETEZ(maThreadSpecific.pLookupCacheMapImpl);
+ ClearLookupCaches(); // before pBASM because of listeners
// destroy BroadcastAreas first to avoid un-needed Single-EndListenings of Formula-Cells
pBASM.reset(); // BroadcastAreaSlotMachine
@@ -1151,22 +1139,21 @@ ScRecursionHelper* ScDocument::CreateRecursionHelperInstance()
return new ScRecursionHelper;
}
-ScLookupCache & ScDocument::GetLookupCache( const ScRange & rRange )
+ScLookupCache & ScDocument::GetLookupCache( const ScRange & rRange, ScInterpreterContext* pContext )
{
ScLookupCache* pCache = nullptr;
- ScLookupCacheMapImpl*& rpCacheMapImpl (
- !IsThreadedGroupCalcInProgress()
- ? maNonThreaded.pLookupCacheMapImpl
- : maThreadSpecific.pLookupCacheMapImpl );
-
- if (!rpCacheMapImpl)
- rpCacheMapImpl = new ScLookupCacheMapImpl;
- auto findIt(rpCacheMapImpl->aCacheMap.find(rRange));
- if (findIt == rpCacheMapImpl->aCacheMap.end())
+ ScLookupCacheMap*& rpCacheMap = pContext->mScLookupCache;
+ if (!rpCacheMap)
+ rpCacheMap = new ScLookupCacheMap;
+ auto findIt(rpCacheMap->aCacheMap.find(rRange));
+ if (findIt == rpCacheMap->aCacheMap.end())
{
- auto insertIt = rpCacheMapImpl->aCacheMap.emplace_hint(findIt,
+ auto insertIt = rpCacheMap->aCacheMap.emplace_hint(findIt,
rRange, o3tl::make_unique<ScLookupCache>(this, rRange) );
pCache = insertIt->second.get();
+ // The StartListeningArea() call is not thread-safe, as all threads
+ // would access the same SvtBroadcaster.
+ osl::MutexGuard guard( mScLookupMutex );
StartListeningArea(rRange, false, pCache);
}
else
@@ -1177,48 +1164,42 @@ ScLookupCache & ScDocument::GetLookupCache( const ScRange & rRange )
void ScDocument::RemoveLookupCache( ScLookupCache & rCache )
{
- if (!IsThreadedGroupCalcInProgress())
- {
- auto it(maNonThreaded.pLookupCacheMapImpl->aCacheMap.find(rCache.getRange()));
- if (it == maNonThreaded.pLookupCacheMapImpl->aCacheMap.end())
- {
- OSL_FAIL( "ScDocument::RemoveLookupCache: range not found in hash map");
- }
- else
- {
- ScLookupCache* pCache = (*it).second.release();
- maNonThreaded.pLookupCacheMapImpl->aCacheMap.erase(it);
- EndListeningArea(pCache->getRange(), false, &rCache);
- }
- }
- else
+ // Data changes leading to this should never happen during calculation (they are either
+ // a result of user input or recalc). If it turns out this can be the case, locking is needed
+ // here and also in ScLookupCache::Notify().
+ assert(!IsThreadedGroupCalcInProgress());
+ if( RemoveLookupCacheHelper( GetNonThreadedContext().mScLookupCache, rCache ))
+ return;
+ // The cache may be possibly in the caches stored for other threads.
+ for( ScLookupCacheMap* cacheMap : mThreadStoredScLookupCaches )
+ if( RemoveLookupCacheHelper( cacheMap, rCache ))
+ return;
+ OSL_FAIL( "ScDocument::RemoveLookupCache: range not found in hash map");
+}
+
+bool ScDocument::RemoveLookupCacheHelper( ScLookupCacheMap* cacheMap, ScLookupCache& rCache )
+{
+ if( cacheMap == nullptr )
+ return false;
+ auto it(cacheMap->aCacheMap.find(rCache.getRange()));
+ if (it != cacheMap->aCacheMap.end())
{
- auto it( maThreadSpecific.pLookupCacheMapImpl->aCacheMap.find(rCache.getRange()));
- if (it == maThreadSpecific.pLookupCacheMapImpl->aCacheMap.end())
- {
- OSL_FAIL( "ScDocument::RemoveLookupCache: range not found in hash map");
- }
- else
- {
- ScLookupCache* pCache = (*it).second.release();
- maThreadSpecific.pLookupCacheMapImpl->aCacheMap.erase(it);
- EndListeningArea(pCache->getRange(), false, &rCache);
- }
+ ScLookupCache* pCache = (*it).second.release();
+ cacheMap->aCacheMap.erase(it);
+ assert(!IsThreadedGroupCalcInProgress()); // EndListeningArea() is not thread-safe
+ EndListeningArea(pCache->getRange(), false, &rCache);
+ return true;
}
+ return false;
}
void ScDocument::ClearLookupCaches()
{
- if (!IsThreadedGroupCalcInProgress())
- {
- if (maNonThreaded.pLookupCacheMapImpl )
- maNonThreaded.pLookupCacheMapImpl->clear();
- }
- else
- {
- if( maThreadSpecific.pLookupCacheMapImpl )
- maThreadSpecific.pLookupCacheMapImpl->clear();
- }
+ assert(!IsThreadedGroupCalcInProgress());
+ DELETEZ(GetNonThreadedContext().mScLookupCache);
+ for( ScLookupCacheMap* cacheMap : mThreadStoredScLookupCaches )
+ delete cacheMap;
+ mThreadStoredScLookupCaches.clear();
}
bool ScDocument::IsCellInChangeTrack(const ScAddress &cell,Color *pColCellBorder)
diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx
index 6b6723921fae..ee007742810f 100644
--- a/sc/source/core/data/document.cxx
+++ b/sc/source/core/data/document.cxx
@@ -6771,7 +6771,20 @@ void ScDocumentThreadSpecific::MergeBackIntoNonThreadedData(ScDocumentThreadSpec
// What about recursion helper and lookup cache?
}
-void ScDocument::MergeBackIntoNonThreadedContext(ScInterpreterContext& threadedContext)
+void ScDocument::SetupFromNonThreadedContext(ScInterpreterContext& threadedContext, int threadNumber)
+{
+ if(int(mThreadStoredScLookupCaches.size()) >= threadNumber + 1 ) // 0-indexed
+ {
+ // It is necessary to store the VLOOKUP cache between threaded formula runs, because the results
+ // are to be shared between different formula group cells (it caches the same row for different
+ // columns). Therefore also use the thread index to make sure each thread gets back its cache,
+ // as it is decided based on thread number which rows in a formula group it handles.
+ threadedContext.mScLookupCache = mThreadStoredScLookupCaches[ threadNumber ];
+ mThreadStoredScLookupCaches[ threadNumber ] = nullptr;
+ }
+}
+
+void ScDocument::MergeBackIntoNonThreadedContext(ScInterpreterContext& threadedContext, int threadNumber)
{
// Move data from a context used by a calculation thread to the main thread's context.
// Called from the main thread after the calculation thread has already finished.
@@ -6780,6 +6793,13 @@ void ScDocument::MergeBackIntoNonThreadedContext(ScInterpreterContext& threadedC
maInterpreterContext.maDelayedSetNumberFormat.end(),
std::make_move_iterator(threadedContext.maDelayedSetNumberFormat.begin()),
std::make_move_iterator(threadedContext.maDelayedSetNumberFormat.end()));
+ if( threadedContext.mScLookupCache )
+ {
+ if(int(mThreadStoredScLookupCaches.size()) < threadNumber + 1 ) // 0-indexed
+ mThreadStoredScLookupCaches.resize( threadNumber + 1 );
+ mThreadStoredScLookupCaches[ threadNumber ] = threadedContext.mScLookupCache;
+ threadedContext.mScLookupCache = nullptr;
+ }
}
/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx
index 2de9f39afac9..4066ce1adad2 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -4667,6 +4667,7 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
for (int i = 0; i < nThreadCount; ++i)
{
contexts[i] = new ScInterpreterContext(*pDocument, pNonThreadedFormatter);
+ pDocument->SetupFromNonThreadedContext(*contexts[i], i);
rThreadPool.pushTask(o3tl::make_unique<Executor>(aTag, i, nThreadCount, pDocument, contexts[i], mxGroup->mpTopCell->aPos, mxGroup->mnLength));
}
@@ -4678,7 +4679,7 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
for (int i = 0; i < nThreadCount; ++i)
{
// This is intentionally done in this main thread in order to avoid locking.
- pDocument->MergeBackIntoNonThreadedContext(*contexts[i]);
+ pDocument->MergeBackIntoNonThreadedContext(*contexts[i], i);
delete contexts[i];
}
diff --git a/sc/source/core/tool/interpr1.cxx b/sc/source/core/tool/interpr1.cxx
index 77c2c50d0dd8..06a97d9e5893 100644
--- a/sc/source/core/tool/interpr1.cxx
+++ b/sc/source/core/tool/interpr1.cxx
@@ -9666,7 +9666,7 @@ bool ScInterpreter::LookupQueryWithCache( ScAddress & o_rResultPos,
{
ScRange aLookupRange( rParam.nCol1, rParam.nRow1, rParam.nTab,
rParam.nCol2, rParam.nRow2, rParam.nTab);
- ScLookupCache& rCache = pDok->GetLookupCache( aLookupRange);
+ ScLookupCache& rCache = pDok->GetLookupCache( aLookupRange, &mrContext );
ScLookupCache::QueryCriteria aCriteria( rEntry);
ScLookupCache::Result eCacheResult = rCache.lookup( o_rResultPos,
aCriteria, aPos);
diff --git a/sc/source/core/tool/interpretercontext.cxx b/sc/source/core/tool/interpretercontext.cxx
new file mode 100644
index 000000000000..35e77694a50d
--- /dev/null
+++ b/sc/source/core/tool/interpretercontext.cxx
@@ -0,0 +1,32 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ *
+ * This file incorporates work covered by the following license notice:
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed
+ * with this work for additional information regarding copyright
+ * ownership. The ASF licenses this file to you under the Apache
+ * License, Version 2.0 (the "License"); you may not use this file
+ * except in compliance with the License. You may obtain a copy of
+ * the License at http://www.apache.org/licenses/LICENSE-2.0 .
+ */
+
+#include <interpretercontext.hxx>
+
+#include <lookupcache.hxx>
+
+ScInterpreterContext::~ScInterpreterContext()
+{
+ for (auto p : maTokens)
+ if (p)
+ p->DecRef();
+ delete mScLookupCache;
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sc/source/core/tool/token.cxx b/sc/source/core/tool/token.cxx
index 11160a4fe7eb..94009bc62eb0 100644
--- a/sc/source/core/tool/token.cxx
+++ b/sc/source/core/tool/token.cxx
@@ -1283,9 +1283,6 @@ void ScTokenArray::CheckForThreading( const FormulaToken& r )
ocMacro,
ocOffset,
ocTableOp,
- ocVLookup,
- ocHLookup,
- ocMatch,
ocCell,
ocInfo,
ocStyle,
More information about the Libreoffice-commits
mailing list