AnalyserModel: improved and fixed areEquivalentVariables().

Indeed, in our WebAssembly module, the key was 32-bit (while 64-bit in C++/Python) which caused collisions and incorrect cache hits.

So, we replaced the Cantor-pairing key and std::map-based cache with a typed VariableKeyPair and std::unordered_map. This makes the cached-equivalence lookup more explicit, portable, and efficient.

Author: agarny

src/analysermodel.cpp

@@ -496,25 +496,20 @@ bool AnalyserModel::areEquivalentVariables(const VariablePtr &variable1,
     // an AnalyserModel object refers to a static version of a model, which
     // means that we can safely cache the result of a call to that utility. In
     // turn, this means that we can speed up any feature (e.g., code generation)
-    // that also relies on that utility. When it comes to the key for the cache,
-    // we use the Cantor pairing function with the address of the two variables
-    // as parameters, thus ensuring the uniqueness of the key (see
-    // https://en.wikipedia.org/wiki/Pairing_function#Cantor_pairing_function).
+    // that also relies on that utility.
 
     auto v1 = reinterpret_cast<uintptr_t>(variable1.get());
     auto v2 = reinterpret_cast<uintptr_t>(variable2.get());
 
-    if (v2 < v1) {
-        v1 += v2;
-        v2 = v1 - v2;
-        v1 = v1 - v2;
+    if (v1 > v2) {
+        std::swap(v1, v2);
     }
 
-    auto key = ((v1 + v2) * (v1 + v2 + 1) >> 1U) + v2;
-    auto cacheKey = mPimpl->mCachedEquivalentVariables.find(key);
+    auto key = AnalyserModel::AnalyserModelImpl::VariableKeyPair{v1, v2};
+    auto it = mPimpl->mCachedEquivalentVariables.find(key);
 
-    if (cacheKey != mPimpl->mCachedEquivalentVariables.end()) {
-        return cacheKey->second;
+    if (it != mPimpl->mCachedEquivalentVariables.end()) {
+        return it->second;
     }
 
     auto res = libcellml::areEquivalentVariables(variable1, variable2);

src/analysermodel_p.h

@@ -16,7 +16,8 @@ limitations under the License.
 
 #pragma once
 
-#include <map>
+#include <cstdint>
+#include <unordered_map>
 
 #include "libcellml/analysermodel.h"
 
@@ -45,6 +46,33 @@ struct AnalyserModel::AnalyserModelImpl
 
     std::vector<AnalyserEquationPtr> mAnalyserEquations;
 
+    struct VariableKeyPair
+    {
+        uintptr_t first;
+        uintptr_t second;
+
+        bool operator==(const VariableKeyPair& other) const
+        {
+            return first == other.first && second == other.second;
+        }
+    };
+
+    struct VariableKeyPairHash
+    {
+        size_t operator()(const VariableKeyPair& pair) const
+        {
+            // A simple and portable hash function for a pair of pointers.
+
+            size_t hash = pair.first;
+
+            hash ^= pair.second + 0x9e3779b9 + (hash << 6) + (hash >> 2);
+
+            return hash;
+        }
+    };
+
+    std::unordered_map<VariableKeyPair, bool, VariableKeyPairHash> mCachedEquivalentVariables;
+
     bool mNeedEqFunction = false;
     bool mNeedNeqFunction = false;
     bool mNeedLtFunction = false;
@@ -72,8 +100,6 @@ struct AnalyserModel::AnalyserModelImpl
     bool mNeedAcschFunction = false;
     bool mNeedAcothFunction = false;
 
-    std::map<uintptr_t, bool> mCachedEquivalentVariables;
-
     static AnalyserModelPtr create(const ModelPtr &model = nullptr);
 
     AnalyserModelImpl(const ModelPtr &model);