[Libreoffice-commits] core.git: Branch 'libreoffice-7-1' - 2 commits - sc/inc sc/source
Luboš Luňák (via logerrit)
logerrit at kemper.freedesktop.org
Tue Dec 1 10:16:08 UTC 2020
sc/inc/document.hxx | 17 ++++-------------
sc/source/core/data/documen2.cxx | 2 +-
sc/source/core/data/documen8.cxx | 12 +++++++-----
sc/source/core/data/document.cxx | 22 +++-------------------
sc/source/core/data/formulacell.cxx | 5 ++---
5 files changed, 17 insertions(+), 41 deletions(-)
New commits:
commit e70739d24b81397ca8e216a4188f09f665030c34
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Fri Nov 27 11:43:10 2020 +0100
Commit: Luboš Luňák <l.lunak at collabora.com>
CommitDate: Tue Dec 1 11:15:50 2020 +0100
clean up ScDocumentThreadSpecific
IIRC there has always been a bit of an overlap between
ScDocumentThreadSpecific and ScInterpreterContext, but it has evolved
over time into the current state, which AFAICT is that
ScDocumentThreadSpecific is just a thread_local aggregate
of thread-specific data (including ScInterpreterContext) and
ScInterpreterContext is the actual storage used for passing data
around (including to/from the worker threads). So remove obsolete
parts of ScDocumentThreadSpecific, including the functions
for moving data to/from threads (they do not do anything, they
even can't do anything because the struct is thread_local, and
they have already been incorrectly converted to static by loplugin).
Change-Id: I81fff7c83df151413a5387b3173af60f122f374a
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/106759
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak at collabora.com>
(cherry picked from commit f98af92e2bbc2064ddc18b80ede68f4267813ddf)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/106826
diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index 5bab5a19d80c..1d6a20efd2af 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -297,16 +297,7 @@ const sal_uInt8 SC_DDE_IGNOREMODE = 255; /// For usage in FindDdeLink()
struct ScDocumentThreadSpecific
{
std::unique_ptr<ScRecursionHelper> xRecursionHelper; // information for recursive and iterative cell formulas
- ScInterpreterContext* pContext; // references the context passed around for easier access
-
- ScDocumentThreadSpecific();
- ~ScDocumentThreadSpecific();
-
- // To be called in the thread at start
- static void SetupFromNonThreadedData(const ScDocumentThreadSpecific& rNonThreadedData);
-
- // To be called in the main thread after the thread has finished
- static void MergeBackIntoNonThreadedData(ScDocumentThreadSpecific& rNonThreadedData);
+ ScInterpreterContext* pContext = nullptr; // references the context passed around for easier access
};
/// Enumeration to determine which pieces of the code should not be mutated when set.
@@ -619,8 +610,8 @@ public:
{
return IsThreadedGroupCalcInProgress() ? *maThreadSpecific.pContext : GetNonThreadedContext();
}
- static void SetupFromNonThreadedContext( ScInterpreterContext& threadedContext, int threadNumber );
- void MergeBackIntoNonThreadedContext( ScInterpreterContext& threadedContext, int threadNumber );
+ void SetupContextFromNonThreadedContext( ScInterpreterContext& threadedContext, int threadNumber );
+ void MergeContextBackIntoNonThreadedContext( ScInterpreterContext& threadedContext, int threadNumber );
void SetThreadedGroupCalcInProgress( bool set ) { (void)this; ScGlobal::bThreadedGroupCalcInProgress = set; }
bool IsThreadedGroupCalcInProgress() const { (void)this; return ScGlobal::bThreadedGroupCalcInProgress; }
@@ -2192,7 +2183,7 @@ public:
*/
void SC_DLLPUBLIC SetFormulaResults( const ScAddress& rTopPos, const double* pResults, size_t nLen );
- const ScDocumentThreadSpecific& CalculateInColumnInThread( ScInterpreterContext& rContext, const ScRange& rCalcRange, unsigned nThisThread, unsigned nThreadsTotal);
+ void CalculateInColumnInThread( ScInterpreterContext& rContext, const ScRange& rCalcRange, unsigned nThisThread, unsigned nThreadsTotal);
void HandleStuffAfterParallelCalculation( SCCOL nColStart, SCCOL nColEnd, SCROW nRow, size_t nLen, SCTAB nTab, ScInterpreter* pInterpreter );
/**
diff --git a/sc/source/core/data/documen8.cxx b/sc/source/core/data/documen8.cxx
index 267382a516eb..ee2383a6fad1 100644
--- a/sc/source/core/data/documen8.cxx
+++ b/sc/source/core/data/documen8.cxx
@@ -409,16 +409,15 @@ void ScDocument::SetFormulaResults( const ScAddress& rTopPos, const double* pRes
pTab->SetFormulaResults(rTopPos.Col(), rTopPos.Row(), pResults, nLen);
}
-const ScDocumentThreadSpecific& ScDocument::CalculateInColumnInThread( ScInterpreterContext& rContext, const ScRange& rCalcRange, unsigned nThisThread, unsigned nThreadsTotal)
+void ScDocument::CalculateInColumnInThread( ScInterpreterContext& rContext, const ScRange& rCalcRange, unsigned nThisThread, unsigned nThreadsTotal)
{
ScTable* pTab = FetchTable(rCalcRange.aStart.Tab());
if (!pTab)
- return maNonThreaded;
+ return;
assert(IsThreadedGroupCalcInProgress());
maThreadSpecific.pContext = &rContext;
- ScDocumentThreadSpecific::SetupFromNonThreadedData(maNonThreaded);
pTab->CalculateInColumnInThread(rContext, rCalcRange.aStart.Col(), rCalcRange.aEnd.Col(), rCalcRange.aStart.Row(), rCalcRange.aEnd.Row(), nThisThread, nThreadsTotal);
assert(IsThreadedGroupCalcInProgress());
@@ -427,8 +426,6 @@ const ScDocumentThreadSpecific& ScDocument::CalculateInColumnInThread( ScInterpr
// (and e.g. outlive the ScDocument), clean them up here, they cannot be cleaned up
// later from the main thread.
maThreadSpecific.xRecursionHelper->Clear();
-
- return maThreadSpecific;
}
void ScDocument::HandleStuffAfterParallelCalculation( SCCOL nColStart, SCCOL nColEnd, SCROW nRow, size_t nLen, SCTAB nTab, ScInterpreter* pInterpreter )
diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx
index 17d0bf5c9561..3adddf93a127 100644
--- a/sc/source/core/data/document.cxx
+++ b/sc/source/core/data/document.cxx
@@ -6814,29 +6814,13 @@ ScRecursionHelper& ScDocument::GetRecursionHelper()
}
}
-void ScDocumentThreadSpecific::SetupFromNonThreadedData(const ScDocumentThreadSpecific& /*rNonThreadedData*/)
-{
- // What about the recursion helper?
- // Copy the lookup cache?
-}
-
-void ScDocumentThreadSpecific::MergeBackIntoNonThreadedData(ScDocumentThreadSpecific& /*rNonThreadedData*/)
-{
- // What about recursion helper and lookup cache?
-}
-
-ScDocumentThreadSpecific::ScDocumentThreadSpecific()
- : pContext(nullptr)
-{}
-
-ScDocumentThreadSpecific::~ScDocumentThreadSpecific() {}
-
-void ScDocument::SetupFromNonThreadedContext(ScInterpreterContext& /*threadedContext*/, int /*threadNumber*/)
+void ScDocument::SetupContextFromNonThreadedContext(ScInterpreterContext& /*threadedContext*/, int /*threadNumber*/)
{
+ (void)this;
// lookup cache is now only in pooled ScInterpreterContext's
}
-void ScDocument::MergeBackIntoNonThreadedContext(ScInterpreterContext& threadedContext, int /*threadNumber*/)
+void ScDocument::MergeContextBackIntoNonThreadedContext(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.
diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx
index 0e2c211f26bf..476ef3e51560 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -4865,7 +4865,6 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
ScRange aCalcRange(mnStartCol, mrTopPos.Row() + mnStartOffset, mrTopPos.Tab(),
mnEndCol, mrTopPos.Row() + mnEndOffset, mrTopPos.Tab());
mpDocument->CalculateInColumnInThread(*mpContext, aCalcRange, mnThisThread, mnThreadsTotal);
- ScDocumentThreadSpecific::MergeBackIntoNonThreadedData(mpDocument->maNonThreaded);
}
};
@@ -4928,7 +4927,7 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
assert(!context->pInterpreter);
aInterpreters[i].reset(new ScInterpreter(this, rDocument, *context, mxGroup->mpTopCell->aPos, *pCode, true));
context->pInterpreter = aInterpreters[i].get();
- ScDocument::SetupFromNonThreadedContext(*context, i);
+ rDocument.SetupContextFromNonThreadedContext(*context, i);
rThreadPool.pushTask(std::make_unique<Executor>(aTag, i, nThreadCount, &rDocument, context, mxGroup->mpTopCell->aPos,
nColStart, nColEnd, nStartOffset, nEndOffset));
}
@@ -4944,7 +4943,7 @@ bool ScFormulaCell::InterpretFormulaGroupThreading(sc::FormulaLogger::GroupScope
{
context = aContextGetterGuard.GetInterpreterContextForThreadIdx(i);
// This is intentionally done in this main thread in order to avoid locking.
- rDocument.MergeBackIntoNonThreadedContext(*context, i);
+ rDocument.MergeContextBackIntoNonThreadedContext(*context, i);
context->pInterpreter = nullptr;
}
commit 9251d86235a80162dd638370ff9c1ebcac215057
Author: Luboš Luňák <l.lunak at collabora.com>
AuthorDate: Fri Nov 27 11:25:40 2020 +0100
Commit: Luboš Luňák <l.lunak at collabora.com>
CommitDate: Tue Dec 1 11:15:35 2020 +0100
it does not work to clear data of other thread's thread_local
The thread-specific RecursionHelper object is reused between runs,
so it should not be freed after a threaded calculation finishes.
But since threads may be left around longer by the thread pool,
the object may be destroyed at an unknown later time. Which could
possibly cause problems if it refers to data that's been destroyed
meanwhile (I think all the pointers in RecursionHelper are not
owning, so this should not be a problem in practice, but still).
So at least clear the contents, they shouldn't be reused anyway.
Change-Id: I8934441c754d20bd20d7e19a8510d9323c0db894
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/106758
Tested-by: Jenkins
Reviewed-by: Luboš Luňák <l.lunak at collabora.com>
(cherry picked from commit cb25771fedd12725a72a1eb4f532698d8221c0f2)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/106825
diff --git a/sc/source/core/data/documen2.cxx b/sc/source/core/data/documen2.cxx
index dbefe610c247..be95822d89ba 100644
--- a/sc/source/core/data/documen2.cxx
+++ b/sc/source/core/data/documen2.cxx
@@ -388,7 +388,7 @@ ScDocument::~ScDocument()
pScriptTypeData.reset();
maNonThreaded.xRecursionHelper.reset();
- maThreadSpecific.xRecursionHelper.reset();
+ assert(!maThreadSpecific.xRecursionHelper);
pPreviewFont.reset();
SAL_WARN_IF( pAutoNameCache, "sc.core", "AutoNameCache still set in dtor" );
diff --git a/sc/source/core/data/documen8.cxx b/sc/source/core/data/documen8.cxx
index 3e09e16656ab..267382a516eb 100644
--- a/sc/source/core/data/documen8.cxx
+++ b/sc/source/core/data/documen8.cxx
@@ -82,6 +82,7 @@
#include <documentlinkmgr.hxx>
#include <scopetools.hxx>
#include <tokenarray.hxx>
+#include <recursionhelper.hxx>
#include <memory>
#include <utility>
@@ -422,6 +423,10 @@ const ScDocumentThreadSpecific& ScDocument::CalculateInColumnInThread( ScInterpr
assert(IsThreadedGroupCalcInProgress());
maThreadSpecific.pContext = nullptr;
+ // If any of the thread_local data would cause problems if they stay around for too long
+ // (and e.g. outlive the ScDocument), clean them up here, they cannot be cleaned up
+ // later from the main thread.
+ maThreadSpecific.xRecursionHelper->Clear();
return maThreadSpecific;
}
More information about the Libreoffice-commits
mailing list