attempt to fix issue #1859 by caching the index and avoid indexOf() call

for iterations

Author: guj

include/openPMD/Iteration.hpp

@@ -129,6 +129,15 @@ namespace internal
          */
         StepStatus m_stepStatus = StepStatus::NoStep;
 
+        /**
+         * Cached copy of the key under which this Iteration lives in
+         * <code>Series::iterations</code>.  Populated when the iteration
+         * object is created/inserted.  This allows constant-time lookup
+         * of the owning map entry instead of a linear scan in
+         * <code>Series::indexOf()</code>.
+         */
+        uint64_t m_iterationIndex = 0;
+
         /**
          * Information on a parsing request that has not yet been executed.
          * Otherwise empty.
@@ -246,6 +255,18 @@ class Iteration : public Attributable
      */
     bool closed() const;
 
+    /**
+     * @brief Get the cached iteration index.
+     *        This is the key under which this iteration is stored in the
+     *        Series::iterations map. Used for testing the index caching
+     *        optimization.
+     *
+     * @return The cached iteration index.
+     */
+    uint64_t getCachedIterationIndex() const
+    {
+        return get().m_iterationIndex;
+    }
     /**
      * @brief Has the iteration been parsed yet?
               If not, it will contain no structure yet.

include/openPMD/backend/ContainerImpl.tpp

@@ -20,7 +20,7 @@
  */
 
 #include "openPMD/backend/Container.hpp"
-
+#include "openPMD/Iteration.hpp"  // needed for index caching in operator[]
 /*
  * Instantiations in src/backend/Container.cpp
  * This file exists so that our tests can include the Container class with
@@ -149,6 +149,12 @@ auto Container<T, T_key, T_container>::operator[](key_type const &key)
         {
             ret.writable().ownKeyWithinParent = std::to_string(key);
         }
+        // remember our key inside the iteration object itself so that
+        // Series::indexOf becomes constant/ log-time instead of linear.
+        if constexpr (std::is_same_v<T, Iteration>)
+        {
+            ret.get().m_iterationIndex = key;
+        }
         traits::GenerationPolicy<T> gen;
         gen(ret);
         return ret;
@@ -181,6 +187,10 @@ auto Container<T, T_key, T_container>::operator[](key_type &&key)
         {
             ret.writable().ownKeyWithinParent = std::to_string(std::move(key));
         }
+        if constexpr (std::is_same_v<T, Iteration>)
+        {
+            ret.get().m_iterationIndex = key;
+        }
         traits::GenerationPolicy<T> gen;
         gen(ret);
         return ret;
@@ -201,13 +211,23 @@ template <typename T, typename T_key, typename T_container>
 auto Container<T, T_key, T_container>::insert(value_type const &value)
     -> std::pair<iterator, bool>
 {
-    return container().insert(value);
+    auto res = container().insert(value);
+    if constexpr (std::is_same_v<T, Iteration>)
+    {
+        res.first->second.get().m_iterationIndex = res.first->first;
+    }
+    return res;
 }
 template <typename T, typename T_key, typename T_container>
 auto Container<T, T_key, T_container>::insert(value_type &&value)
     -> std::pair<iterator, bool>
 {
-    return container().insert(value);
+    auto res = container().insert(value);
+    if constexpr (std::is_same_v<T, Iteration>)
+    {
+        res.first->second.get().m_iterationIndex = res.first->first;
+    }
+    return res;
 }
 template <typename T, typename T_key, typename T_container>
 auto Container<T, T_key, T_container>::insert(

src/Series.cpp

@@ -2604,11 +2604,26 @@ std::string Series::iterationFilename(IterationIndex_t i)
 Series::iterations_iterator Series::indexOf(Iteration const &iteration)
 {
     auto &series = get();
+    // first try the cached index; if it points to the correct entry return it
+    auto idx = iteration.get().m_iterationIndex;
+    if (idx != 0) // zero is default/unset but index 0 is valid, so we still check
+    {
+        auto it = series.iterations.find(idx);
+        if (it != series.iterations.end() &&
+            &it->second.Attributable::get() == &iteration.Attributable::get())
+        {
+            return it;
+        }
+        // if the cached index is stale (shouldn't happen), fall back
+    }
+    // fallback to linear scan for safety
     for (auto it = series.iterations.begin(); it != series.iterations.end();
          ++it)
     {
         if (&it->second.Attributable::get() == &iteration.Attributable::get())
         {
+            // update cache so future calls are fast
+            it->second.get().m_iterationIndex = it->first;
             return it;
         }
     }

test/SerialIOTest.cpp

@@ -5404,6 +5404,42 @@ TEST_CASE("variableBasedSingleIteration", "[serial][adios2]")
     }
 }
 
+
+// verify that the iteration index is cached inside the Iteration object and
+// that Series::indexOf can make use of it to find the map entry without
+// scanning all iterations.
+TEST_CASE("iterationIndexCaching", "[serial][adios2]")
+{
+    using auxiliary::remove_file;
+    std::string file = "tmp-index-cache.bp5";
+    // ensure a clean slate
+    remove_file(file);
+
+    // write a few iterations and close them
+    {
+        Series s(file, Access::CREATE_LINEAR);
+        for (uint64_t i = 0; i < 3; ++i)
+        {
+            auto &it = s.iterations[i];
+            it.setTime<double>(static_cast<double>(i));
+            it.close();
+        }
+        s.flush();
+    }
+
+    // reopen read-only and inspect internal index
+    {
+        Series s(file, Access::READ_ONLY);
+        for (auto &pair : s.iterations)
+        {
+            auto &iter = pair.second;
+            REQUIRE(iter.getCachedIterationIndex() == pair.first);
+        }
+    }
+
+    remove_file(file);
+}
+
 namespace epsilon
 {
 template <typename T>