From 79449d73900d7a9bf061244d76f5f8eecc441198 Mon Sep 17 00:00:00 2001 From: Luboš Luňák Date: Mon, 1 Oct 2018 14:26:57 +0200 Subject: make VLOOKUP in Calc thread-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- 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(-) create mode 100644 sc/source/core/tool/interpretercontext.cxx 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 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& rPosArray ); void SetNeedsListeningGroups( const std::vector& rPosArray ); + + bool RemoveLookupCacheHelper( ScLookupCacheMap* cacheMap, ScLookupCache& rCache ); }; typedef std::unique_ptr> 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 maTokens; std::vector 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 +#include #include 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::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::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(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(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 + +#include + +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, -- cgit v1.2.3