move SetNumberFormat() calls out of calc threads

As it modifies ScAttrArray, which is not thread-safe (has std::vector
per each column, so multiple threads may try resize it etc.). So if
threaded calculation is done, delay the calls to the main thread.

Change-Id: I3d87665c0dd0d40f0c2efbcf8958240ee5580233
Reviewed-on: https://gerrit.libreoffice.org/55602
Reviewed-by: Michael Meeks <michael.meeks@collabora.com>
Tested-by: Jenkins

5 files changed, 51 insertions, 8 deletions

diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index 813d388abb4d..fcd09a1214de 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -573,6 +573,7 @@ public:
         maInterpreterContext.mpFormatter = GetFormatTable();
         return maInterpreterContext;
     }
+    void MergeBackIntoNonThreadedContext( ScInterpreterContext& threadedContext );
     void SetThreadedGroupCalcInProgress( bool set ) { (void)this; ScGlobal::bThreadedGroupCalcInProgress = set; }
     bool IsThreadedGroupCalcInProgress() const { (void)this; return ScGlobal::bThreadedGroupCalcInProgress; }
 
diff --git a/sc/inc/interpretercontext.hxx b/sc/inc/interpretercontext.hxx
index d2b6814fad6e..8b920472fb44 100644
--- a/sc/inc/interpretercontext.hxx
+++ b/sc/inc/interpretercontext.hxx
@@ -12,18 +12,27 @@
 
 #include <vector>
 #include <formula/token.hxx>
+#include "types.hxx"
 
 #define TOKEN_CACHE_SIZE 8
 
 class ScDocument;
 class SvNumberFormatter;
 
+// SetNumberFormat() is not thread-safe, so calls to it need to be delayed to the main thread.
+struct DelayedSetNumberFormat
+{
+    SCROW mRow; // Used only with formula groups, so column and tab do not need to be stored.
+    sal_uInt32 mnNumberFormat;
+};
+
 struct ScInterpreterContext
 {
     const ScDocument& mrDoc;
     SvNumberFormatter* mpFormatter;
     size_t mnTokenCachePos;
     std::vector<formula::FormulaToken*> maTokens;
+    std::vector<DelayedSetNumberFormat> maDelayedSetNumberFormat;
 
     ScInterpreterContext(const ScDocument& rDoc, SvNumberFormatter* pFormatter)
         : mrDoc(rDoc)
diff --git a/sc/source/core/data/documen8.cxx b/sc/source/core/data/documen8.cxx
index 293eb556d676..7d5542173b96 100644
--- a/sc/source/core/data/documen8.cxx
+++ b/sc/source/core/data/documen8.cxx
@@ -445,6 +445,11 @@ const ScDocumentThreadSpecific& ScDocument::CalculateInColumnInThread( ScInterpr
 
 void ScDocument::HandleStuffAfterParallelCalculation( const ScAddress& rTopPos, size_t nLen )
 {
+    assert(!IsThreadedGroupCalcInProgress());
+    for( DelayedSetNumberFormat& data : GetNonThreadedContext().maDelayedSetNumberFormat)
+        SetNumberFormat( ScAddress( rTopPos.Col(), data.mRow, rTopPos.Tab()), data.mnNumberFormat );
+    GetNonThreadedContext().maDelayedSetNumberFormat.clear();
+
     ScTable* pTab = FetchTable(rTopPos.Tab());
     if (!pTab)
         return;
diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx
index 88d0617b038a..411e71127e38 100644
--- a/sc/source/core/data/document.cxx
+++ b/sc/source/core/data/document.cxx
@@ -3693,6 +3693,7 @@ sal_uInt32 ScDocument::GetNumberFormat( const ScInterpreterContext& rContext, co
 
 void ScDocument::SetNumberFormat( const ScAddress& rPos, sal_uInt32 nNumberFormat )
 {
+    assert(!IsThreadedGroupCalcInProgress());
     SCTAB nTab = rPos.Tab();
     if (!TableExists(nTab))
         return;
@@ -6756,4 +6757,15 @@ void ScDocumentThreadSpecific::MergeBackIntoNonThreadedData(ScDocumentThreadSpec
     // What about recursion helper and lookup cache?
 }
 
+void ScDocument::MergeBackIntoNonThreadedContext(ScInterpreterContext& threadedContext)
+{
+    // 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.
+    assert(!IsThreadedGroupCalcInProgress());
+    maInterpreterContext.maDelayedSetNumberFormat.insert(
+        maInterpreterContext.maDelayedSetNumberFormat.end(),
+        std::make_move_iterator(threadedContext.maDelayedSetNumberFormat.begin()),
+        std::make_move_iterator(threadedContext.maDelayedSetNumberFormat.end()));
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sc/source/core/data/formulacell.cxx b/sc/source/core/data/formulacell.cxx
index 4aca388c2f6e..6dda20b6a3b5 100644
--- a/sc/source/core/data/formulacell.cxx
+++ b/sc/source/core/data/formulacell.cxx
@@ -2009,7 +2009,16 @@ void ScFormulaCell::InterpretTail( ScInterpreterContext& rContext, ScInterpretTa
             if (bSetFormat && (bForceNumberFormat || ((nFormatIndex % SV_COUNTRY_LANGUAGE_OFFSET) != 0)))
             {
                 // set number format explicitly
-                pDocument->SetNumberFormat( aPos, nFormatIndex );
+                if (!pDocument->IsThreadedGroupCalcInProgress())
+                    pDocument->SetNumberFormat( aPos, nFormatIndex );
+                else
+                {
+                    // SetNumberFormat() is not thread-safe (modifies ScAttrArray), delay the work
+                    // to the main thread. Since thread calculations operate on formula groups,
+                    // it's enough to store just the row.
+                    DelayedSetNumberFormat data = { aPos.Row(), nFormatIndex };
+                    rContext.maDelayedSetNumberFormat.push_back( data );
+                }
                 bChanged = true;
             }
 
@@ -4435,7 +4444,7 @@ bool ScFormulaCell::InterpretFormulaGroup()
             const unsigned mnThisThread;
             const unsigned mnThreadsTotal;
             ScDocument* mpDocument;
-            SvNumberFormatter* mpFormatter;
+            ScInterpreterContext* mpContext;
             const ScAddress& mrTopPos;
             SCROW mnLength;
 
@@ -4444,14 +4453,14 @@ bool ScFormulaCell::InterpretFormulaGroup()
                      unsigned nThisThread,
                      unsigned nThreadsTotal,
                      ScDocument* pDocument2,
-                     SvNumberFormatter* pFormatter,
+                     ScInterpreterContext* pContext,
                      const ScAddress& rTopPos,
                      SCROW nLength) :
                 comphelper::ThreadTask(rTag),
                 mnThisThread(nThisThread),
                 mnThreadsTotal(nThreadsTotal),
                 mpDocument(pDocument2),
-                mpFormatter(pFormatter),
+                mpContext(pContext),
                 mrTopPos(rTopPos),
                 mnLength(nLength)
             {
@@ -4459,9 +4468,7 @@ bool ScFormulaCell::InterpretFormulaGroup()
 
             virtual void doWork() override
             {
-                ScInterpreterContext aContext(*mpDocument, mpFormatter);
-
-                auto aNonThreadedData = mpDocument->CalculateInColumnInThread(aContext, mrTopPos, mnLength, mnThisThread, mnThreadsTotal);
+                auto aNonThreadedData = mpDocument->CalculateInColumnInThread(*mpContext, mrTopPos, mnLength, mnThisThread, mnThreadsTotal);
                 aNonThreadedData.MergeBackIntoNonThreadedData(mpDocument->maNonThreaded);
             }
 
@@ -4485,9 +4492,11 @@ bool ScFormulaCell::InterpretFormulaGroup()
 
             // Start nThreadCount new threads
             std::shared_ptr<comphelper::ThreadTaskTag> aTag = comphelper::ThreadPool::createThreadTaskTag();
+            std::vector<ScInterpreterContext*> contexts(nThreadCount);
             for (int i = 0; i < nThreadCount; ++i)
             {
-                rThreadPool.pushTask(new Executor(aTag, i, nThreadCount, pDocument, pNonThreadedFormatter, mxGroup->mpTopCell->aPos, mxGroup->mnLength));
+                contexts[i] = new ScInterpreterContext(*pDocument, pNonThreadedFormatter);
+                rThreadPool.pushTask(new Executor(aTag, i, nThreadCount, pDocument, contexts[i], mxGroup->mpTopCell->aPos, mxGroup->mnLength));
             }
 
             SAL_INFO("sc.threaded", "Joining threads");
@@ -4495,6 +4504,13 @@ bool ScFormulaCell::InterpretFormulaGroup()
 
             pDocument->SetThreadedGroupCalcInProgress(false);
 
+            for (int i = 0; i < nThreadCount; ++i)
+            {
+                // This is intentionally done in this main thread in order to avoid locking.
+                pDocument->MergeBackIntoNonThreadedContext(*contexts[i]);
+                delete contexts[i];
+            }
+
             SAL_INFO("sc.threaded", "Done");
         }