Improve caching of equivalent variables in analyser model.

src/analyser.cpp

@@ -25,12 +25,13 @@ limitations under the License.
 
 #include "libcellml/analyserequation.h"
 #include "libcellml/analyserexternalvariable.h"
-#include "libcellml/generatorprofile.h"
+#include "libcellml/analysermodel.h"
 #include "libcellml/validator.h"
 
 #include "analyser_p.h"
 #include "analyserequation_p.h"
 #include "analyserequationast_p.h"
+#include "analysermodel_p.h"
 #include "analyservariable_p.h"
 #include "commonutils.h"
 #include "generator_p.h"

src/analyser_p.h

@@ -14,6 +14,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
+#include "libcellml/analyser.h"
 #include "libcellml/generatorprofile.h"
 #include "libcellml/issue.h"
 

src/analysermodel.cpp

@@ -16,16 +16,21 @@ limitations under the License.
 
 #include "libcellml/analysermodel.h"
 
-#include "libcellml/analyservariable.h"
-
 #include "analysermodel_p.h"
+#include "analyservariable_p.h"
 #include "utilities.h"
 
 namespace libcellml {
 
 AnalyserModelPtr AnalyserModel::AnalyserModelImpl::create(const ModelPtr &model)
 {
-    return std::shared_ptr<AnalyserModel> {new AnalyserModel(model)};
+    auto res = std::shared_ptr<AnalyserModel> {new AnalyserModel(model)};
+
+    if (model) {
+        res->mPimpl->buildEquivalentVariablesCache();
+    }
+
+    return res;
 }
 
 AnalyserModel::AnalyserModelImpl::AnalyserModelImpl(const ModelPtr &model)
@@ -43,6 +48,38 @@ AnalyserModel::~AnalyserModel()
     delete mPimpl;
 }
 
+void AnalyserModel::AnalyserModelImpl::buildEquivalentVariablesCache(const ComponentPtr &component)
+{
+    for (size_t i = 0; i < component->variableCount(); ++i) {
+        auto variable = component->variable(i);
+
+        for (size_t j = 0; j < variable->equivalentVariableCount(); ++j) {
+            auto equivalentVariable = variable->equivalentVariable(j);
+            auto v1 = reinterpret_cast<uintptr_t>(variable.get());
+            auto v2 = reinterpret_cast<uintptr_t>(equivalentVariable.get());
+
+            if (v2 < v1) {
+                std::swap(v1, v2);
+            }
+
+            uniteEquivalentVariableAddresses(v1, v2);
+        }
+    }
+
+    for (size_t i = 0; i < component->componentCount(); ++i) {
+        buildEquivalentVariablesCache(component->component(i));
+    }
+}
+
+void AnalyserModel::AnalyserModelImpl::buildEquivalentVariablesCache()
+{
+    mEquivalentVariableCache.clear();
+
+    for (size_t i = 0; i < mModel->componentCount(); ++i) {
+        buildEquivalentVariablesCache(mModel->component(i));
+    }
+}
+
 bool AnalyserModel::isValid() const
 {
     switch (mPimpl->mType) {
@@ -498,25 +535,18 @@ bool AnalyserModel::areEquivalentVariables(const VariablePtr &variable1,
     // turn, this means that we can speed up any feature (e.g., code generation)
     // that also relies on that utility.
 
-    auto v1 = reinterpret_cast<uintptr_t>(variable1.get());
-    auto v2 = reinterpret_cast<uintptr_t>(variable2.get());
-
-    if (v1 > v2) {
-        std::swap(v1, v2);
+    if ((variable1 == nullptr) || (variable2 == nullptr)) {
+        return false;
     }
 
-    auto key = AnalyserModel::AnalyserModelImpl::VariableKeyPair {v1, v2};
-    auto it = mPimpl->mCachedEquivalentVariables.find(key);
-
-    if (it != mPimpl->mCachedEquivalentVariables.end()) {
-        return it->second;
+    if (variable1 == variable2) {
+        return true;
     }
 
-    auto res = libcellml::areEquivalentVariables(variable1, variable2);
-
-    mPimpl->mCachedEquivalentVariables.emplace(key, res);
+    const auto v1 = reinterpret_cast<uintptr_t>(variable1.get());
+    const auto v2 = reinterpret_cast<uintptr_t>(variable2.get());
 
-    return res;
+    return mPimpl->findVariableAddress(v1) == mPimpl->findVariableAddress(v2);
 }
 
 } // namespace libcellml

src/analysermodel_p.h

@@ -46,32 +46,34 @@ struct AnalyserModel::AnalyserModelImpl
 
     std::vector<AnalyserEquationPtr> mAnalyserEquations;
 
-    struct VariableKeyPair
+    std::unordered_map<uintptr_t, uintptr_t> mEquivalentVariableCache;
+
+    uintptr_t findVariableAddress(uintptr_t x)
     {
-        uintptr_t first;
-        uintptr_t second;
+        auto it = mEquivalentVariableCache.find(x);
+
+        if (it == mEquivalentVariableCache.end()) {
+            mEquivalentVariableCache[x] = x;
 
-        bool operator==(const VariableKeyPair &other) const
-        {
-            return (first == other.first) & (second == other.second);
+            return x;
         }
-    };
 
-    struct VariableKeyPairHash
-    {
-        size_t operator()(const VariableKeyPair &pair) const
-        {
-            // A simple and portable hash function for a pair of pointers.
+        if (it->second != x) {
+            it->second = findVariableAddress(it->second);
+        }
 
-            size_t hash = pair.first;
+        return it->second;
+    }
 
-            hash ^= pair.second + 0x9e3779b9 + (hash << 6) + (hash >> 2);
+    void uniteEquivalentVariableAddresses(uintptr_t x, uintptr_t y)
+    {
+        const uintptr_t &rootX = findVariableAddress(x);
+        const uintptr_t &rootY = findVariableAddress(y);
 
-            return hash;
+        if (rootX != rootY) {
+            mEquivalentVariableCache[rootY] = rootX;
         }
-    };
-
-    std::unordered_map<VariableKeyPair, bool, VariableKeyPairHash> mCachedEquivalentVariables;
+    }
 
     bool mNeedEqFunction = false;
     bool mNeedNeqFunction = false;
@@ -102,6 +104,9 @@ struct AnalyserModel::AnalyserModelImpl
 
     static AnalyserModelPtr create(const ModelPtr &model = nullptr);
 
+    void buildEquivalentVariablesCache(const ComponentPtr &component);
+    void buildEquivalentVariablesCache();
+
     AnalyserModelImpl(const ModelPtr &model);
 };
 

src/api/libcellml/analysermodel.h

@@ -600,14 +600,13 @@ class LIBCELLML_EXPORT AnalyserModel
      * Returns @c true if @p variable1 is equivalent to @p variable2 and
      * @c false otherwise.
      *
-     * To test for equivalence is time consuming, so caching is used to speed
-     * things up. During the analysis of a model, various tests are performed
-     * and their result cached. So, if you test two variables that were tested
-     * during the analysis then the cached result will be returned otherwise the
-     * two variables will be properly tested and their result cached. This works
-     * because an @ref AnalyserModel always refers to a static version of a
-     * @ref Model. However, this might break if a @ref Model is modified after it
-     * has been analysed.
+     * The function utilises caching which is constructed during the model
+     * analysis phase (@ref Analyser::analyseModel). The cache may become
+     * out of date if the model is changed after the model has been analysed.
+     *
+     * @note This function is primarily designed for use during model analysis
+     *       by the @ref Analyser. While external usage is not programmatically
+     *       restricted, it is not the primary intended use case.
      *
      * @param variable1 The @ref Variable to test if it is equivalent to
      * @p variable2.

tests/coverage/coverage.cpp

@@ -589,6 +589,21 @@ TEST(Coverage, analyserTypes)
     EXPECT_EQ("algebraic_variable", libcellml::AnalyserVariable::typeAsString(analyserModel->algebraicVariable(0)->type()));
 }
 
+TEST(Coverage, analyserAreEquivalentVariables)
+{
+    auto analyser = libcellml::Analyser::create();
+    auto parser = libcellml::Parser::create();
+    auto model = parser->parseModel(fileContents("generator/hodgkin_huxley_squid_axon_model_1952/model.cellml"));
+
+    analyser->analyseModel(model);
+
+    auto analyserModel = analyser->analyserModel();
+    EXPECT_FALSE(analyserModel->areEquivalentVariables(nullptr, nullptr));
+    auto variable = model->component("membrane")->variable("V");
+    EXPECT_FALSE(analyserModel->areEquivalentVariables(nullptr, variable));
+    EXPECT_FALSE(analyserModel->areEquivalentVariables(variable, nullptr));
+}
+
 void checkAstTypeAsString(const libcellml::AnalyserEquationAstPtr &ast)
 {
     if (ast != nullptr) {