discard cached cell values if the cell changes

FormulaGroupContext is actually a cache of cell results for OpenCL,
but the cached values are not always properly discarded. Happens e.g.
with testFormulaDepTracking in sc_ucalc fails if OpenCL is forced for
it (i.e. with mnOpenCLMinimumFormulaGroupSize disabled), because
a SetString() call for a cell doesn't invalidate the cache.

This obviously reduces the cache hit rate a bit, but according to my
tests it's not that bad (in fact the cache doesn't seem to get used
that often, so I even wonder if it's worth it).

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

5 files changed, 37 insertions, 4 deletions

diff --git a/sc/inc/document.hxx b/sc/inc/document.hxx
index fcd09a1214de..ad371bd73cd8 100644
--- a/sc/inc/document.hxx
+++ b/sc/inc/document.hxx
@@ -353,9 +353,11 @@ private:
     rtl::Reference<ScPoolHelper> mxPoolHelper;
 
     std::shared_ptr<svl::SharedStringPool> mpCellStringPool;
-    std::shared_ptr<sc::FormulaGroupContext> mpFormulaGroupCxt;
     std::unique_ptr<sc::DocumentLinkManager> mpDocLinkMgr;
 
+    std::shared_ptr<sc::FormulaGroupContext> mpFormulaGroupCxt;
+    bool                mbFormulaGroupCxtBlockDiscard;
+
     ScCalcConfig        maCalcConfig;
 
     SfxUndoManager*     mpUndoManager;
@@ -1118,6 +1120,9 @@ public:
     svl::SharedString                         GetSharedString( const ScAddress& rPos ) const;
 
     std::shared_ptr<sc::FormulaGroupContext>& GetFormulaGroupContext();
+    void                                      DiscardFormulaGroupContext();
+    void                                      BlockFormulaGroupContextDiscard( bool block )
+                                                  { mbFormulaGroupCxtBlockDiscard = block; }
 
     SC_DLLPUBLIC void                         GetInputString( SCCOL nCol, SCROW nRow, SCTAB nTab, OUString& rString );
     FormulaError                              GetStringForFormula( const ScAddress& rPos, OUString& rString );
diff --git a/sc/inc/formulagroup.hxx b/sc/inc/formulagroup.hxx
index fa6bb4f61877..f0dcb4888a72 100644
--- a/sc/inc/formulagroup.hxx
+++ b/sc/inc/formulagroup.hxx
@@ -48,6 +48,8 @@ struct FormulaGroupEntry
     FormulaGroupEntry( ScFormulaCell* pCell, size_t nRow );
 };
 
+// Despite the name, this is actually a cache of cell values, used by OpenCL
+// code ... I think. And obviously it's not really a struct either.
 struct FormulaGroupContext
 {
     typedef AlignedAllocator<double,256> DoubleAllocType;
diff --git a/sc/source/core/data/column2.cxx b/sc/source/core/data/column2.cxx
index 6eb7697d074f..912428f688e1 100644
--- a/sc/source/core/data/column2.cxx
+++ b/sc/source/core/data/column2.cxx
@@ -1506,6 +1506,11 @@ SCROW ScColumn::FindNextVisibleRowWithContent(
 
 void ScColumn::CellStorageModified()
 {
+    // Remove cached values. Given how often this function is called and how (not that) often
+    // the cached values are used, it should be more efficient to just discard everything
+    // instead of trying to figure out each time exactly what to discard.
+    GetDoc()->DiscardFormulaGroupContext();
+
     // TODO: Update column's "last updated" timestamp here.
 
 #if DEBUG_COLUMN_STORAGE
@@ -1563,8 +1568,6 @@ void ScColumn::CellStorageModified()
         while (itAttr != maCellTextAttrs.end() && itAttr->type != sc::element_type_empty)
             ++itAttr;
     }
-#else
-    (void) this; // Avoid "this member function can be declared static [loplugin:staticmethods]"
 #endif
 }
 
@@ -2635,6 +2638,15 @@ bool hasNonEmpty( const sc::FormulaGroupContext::StrArrayType& rArray, SCROW nRo
     return std::any_of(it, itEnd, NonNullStringFinder());
 }
 
+struct ProtectFormulaGroupContext
+{
+    ProtectFormulaGroupContext( ScDocument* d )
+        : doc( d ) { doc->BlockFormulaGroupContextDiscard( true ); }
+    ~ProtectFormulaGroupContext()
+        { doc->BlockFormulaGroupContextDiscard( false ); }
+    ScDocument* doc;
+};
+
 }
 
 formula::VectorRefArray ScColumn::FetchVectorRefArray( SCROW nRow1, SCROW nRow2 )
@@ -2659,6 +2671,12 @@ formula::VectorRefArray ScColumn::FetchVectorRefArray( SCROW nRow1, SCROW nRow2
         return formula::VectorRefArray(pNum, pStr);
     }
 
+    // ScColumn::CellStorageModified() simply discards the entire cache (FormulaGroupContext)
+    // on any modification. However getting cell values may cause this to be called
+    // if interpreting a cell results in a change to it (not just its result though).
+    // So temporarily block the discarding.
+    ProtectFormulaGroupContext protectContext( GetDoc());
+
     double fNan;
     rtl::math::setNan(&fNan);
 
diff --git a/sc/source/core/data/documen2.cxx b/sc/source/core/data/documen2.cxx
index dd807c3c24fd..88c9b972c912 100644
--- a/sc/source/core/data/documen2.cxx
+++ b/sc/source/core/data/documen2.cxx
@@ -131,8 +131,9 @@ private:
 
 ScDocument::ScDocument( ScDocumentMode eMode, SfxObjectShell* pDocShell ) :
         mpCellStringPool(new svl::SharedStringPool(ScGlobal::pCharClass)),
-        mpFormulaGroupCxt(nullptr),
         mpDocLinkMgr(new sc::DocumentLinkManager(pDocShell)),
+        mpFormulaGroupCxt(nullptr),
+        mbFormulaGroupCxtBlockDiscard(false),
         maCalcConfig( ScInterpreter::GetGlobalConfig()),
         mpUndoManager( nullptr ),
         mpEditEngine( nullptr ),
diff --git a/sc/source/core/data/document.cxx b/sc/source/core/data/document.cxx
index b6884b9088f9..f970fd44bba7 100644
--- a/sc/source/core/data/document.cxx
+++ b/sc/source/core/data/document.cxx
@@ -3544,6 +3544,13 @@ std::shared_ptr<sc::FormulaGroupContext>& ScDocument::GetFormulaGroupContext()
     return mpFormulaGroupCxt;
 }
 
+void ScDocument::DiscardFormulaGroupContext()
+{
+    assert(!IsThreadedGroupCalcInProgress());
+    if( !mbFormulaGroupCxtBlockDiscard )
+        mpFormulaGroupCxt.reset();
+}
+
 void ScDocument::GetInputString( SCCOL nCol, SCROW nRow, SCTAB nTab, OUString& rString )
 {
     if ( ValidTab(nTab) && nTab < static_cast<SCTAB>(maTabs.size()) && maTabs[nTab] )