Fix dirtyRecursive() performance for Series with many steps (#1598)

* Don't check closed iterations if they are dirty

* WIP: Alternative way of checking for errors

* Re-enable some tests

* Clear dirty files after flushing in ADIOS2

* [WIP] Track dirtyRecursive from the beginning

* Continue fixing things

* Further fixes

* Test cleanup

* CI fixes

* Replace drop() with a condition check

* Move printDirty to debug::printDirty

* Update include/openPMD/RecordComponent.hpp

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>

* Revert to older verification logic

It's more performant now with the new implementation for dirtyRecursive

* Document

* Remove stale changes

---------

Co-authored-by: Axel Huebl <axel.huebl@plasma.ninja>

include/openPMD/Iteration.hpp

@@ -380,16 +380,6 @@ class Iteration : public Attributable
      */
     void setStepStatus(StepStatus);
 
-    /*
-     * @brief Check recursively whether this Iteration is dirty.
-     *        It is dirty if any attribute or dataset is read from or written to
-     *        the backend.
-     *
-     * @return true If dirty.
-     * @return false Otherwise.
-     */
-    bool dirtyRecursive() const;
-
     /**
      * @brief Link with parent.
      *

include/openPMD/ParticleSpecies.hpp

@@ -44,16 +44,6 @@ class ParticleSpecies : public Container<Record>
 
     void read();
     void flush(std::string const &, internal::FlushParams const &) override;
-
-    /**
-     * @brief Check recursively whether this ParticleSpecies is dirty.
-     *        It is dirty if any attribute or dataset is read from or written to
-     *        the backend.
-     *
-     * @return true If dirty.
-     * @return false Otherwise.
-     */
-    bool dirtyRecursive() const;
 };
 
 namespace traits

include/openPMD/RecordComponent.hpp

@@ -69,6 +69,8 @@ namespace internal
          * Chunk reading/writing requests on the contained dataset.
          */
         std::queue<IOTask> m_chunks;
+
+        void push_chunk(IOTask &&task);
         /**
          * Stores the value for constant record components.
          * Ignored otherwise.
@@ -497,16 +499,6 @@ class RecordComponent : public BaseRecordComponent
     void storeChunk(
         auxiliary::WriteBuffer buffer, Datatype datatype, Offset o, Extent e);
 
-    /**
-     * @brief Check recursively whether this RecordComponent is dirty.
-     *        It is dirty if any attribute or dataset is read from or written to
-     *        the backend.
-     *
-     * @return true If dirty.
-     * @return false Otherwise.
-     */
-    bool dirtyRecursive() const;
-
     // clang-format off
 OPENPMD_protected
     // clang-format on

include/openPMD/RecordComponent.tpp

@@ -172,7 +172,7 @@ RecordComponent::loadChunk(std::shared_ptr<T> data, Offset o, Extent e)
         dRead.extent = extent;
         dRead.dtype = getDatatype();
         dRead.data = std::static_pointer_cast<void>(data);
-        rc.m_chunks.push(IOTask(this, dRead));
+        rc.push_chunk(IOTask(this, dRead));
     }
 }
 

include/openPMD/Series.hpp

@@ -837,6 +837,11 @@ OPENPMD_private
     AbstractIOHandler *IOHandler();
     AbstractIOHandler const *IOHandler() const;
 }; // Series
+
+namespace debug
+{
+    void printDirty(Series const &);
+}
 } // namespace openPMD
 
 // Make sure that this one is always included if Series.hpp is included,

include/openPMD/backend/Attributable.hpp

@@ -83,8 +83,15 @@ namespace internal
 
     template <typename, typename>
     class BaseRecordData;
+
+    class RecordComponentData;
 } // namespace internal
 
+namespace debug
+{
+    void printDirty(Series const &);
+}
+
 /** @brief Layer to manage storage of attributes associated with file objects.
  *
  * Mandatory and user-defined Attributes and their data for every object in the
@@ -109,6 +116,8 @@ class Attributable
     friend class Series;
     friend class Writable;
     friend class WriteIterations;
+    friend class internal::RecordComponentData;
+    friend void debug::printDirty(Series const &);
 
 protected:
     // tag for internal constructor
@@ -378,11 +387,49 @@ OPENPMD_protected
 
     bool dirty() const
     {
-        return writable().dirty;
+        return writable().dirtySelf;
+    }
+    /** O(1).
+     */
+    bool dirtyRecursive() const
+    {
+        return writable().dirtyRecursive;
+    }
+    void setDirty(bool dirty_in)
+    {
+        auto &w = writable();
+        w.dirtySelf = dirty_in;
+        setDirtyRecursive(dirty_in);
     }
-    bool &dirty()
+    /* Amortized O(1) if dirty_in is true, else O(1).
+     *
+     * Must be used carefully with `dirty_in == false` since it is assumed that
+     * all children are not dirty.
+     *
+     * Invariant of dirtyRecursive:
+     *   this->dirtyRecursive implies parent->dirtyRecursive.
+     *
+     * Hence:
+     *
+     * * If dirty_in is true: This needs only go up far enough until a parent is
+     *   found that itself is dirtyRecursive.
+     * * If dirty_in is false: Only sets `this` to `dirtyRecursive == false`.
+     *   The caller must ensure that the invariant holds (e.g. clearing
+     *   everything during flushing or reading logic).
+     */
+    void setDirtyRecursive(bool dirty_in)
     {
-        return writable().dirty;
+        auto &w = writable();
+        w.dirtyRecursive = dirty_in;
+        if (dirty_in)
+        {
+            auto current = w.parent;
+            while (current && !current->dirtyRecursive)
+            {
+                current->dirtyRecursive = true;
+                current = current->parent;
+            }
+        }
     }
     bool written() const
     {
@@ -417,7 +464,7 @@ inline bool Attributable::setAttribute(std::string const &key, T value)
         error::throwNoSuchAttribute(out_of_range_msg(key));
     }
 
-    dirty() = true;
+    setDirty(true);
     auto it = attri.m_attributes.lower_bound(key);
     if (it != attri.m_attributes.end() &&
         !attri.m_attributes.key_comp()(key, it->first))

include/openPMD/backend/BaseRecord.hpp

@@ -494,15 +494,6 @@ class BaseRecord
     virtual void
     flush_impl(std::string const &, internal::FlushParams const &) = 0;
 
-    /**
-     * @brief Check recursively whether this BaseRecord is dirty.
-     *        It is dirty if any attribute or dataset is read from or written to
-     *        the backend.
-     *
-     * @return true If dirty.
-     * @return false Otherwise.
-     */
-    bool dirtyRecursive() const;
     void eraseScalar();
 }; // BaseRecord
 
@@ -999,25 +990,12 @@ inline void BaseRecord<T_elem>::flush(
     }
 
     this->flush_impl(name, flushParams);
-    // flush_impl must take care to correctly set the dirty() flag so this
-    // method doesn't do it
-}
-
-template <typename T_elem>
-inline bool BaseRecord<T_elem>::dirtyRecursive() const
-{
-    if (this->dirty())
+    if (flushParams.flushLevel != FlushLevel::SkeletonOnly)
     {
-        return true;
+        this->setDirty(false);
     }
-    for (auto const &pair : *this)
-    {
-        if (pair.second.dirtyRecursive())
-        {
-            return true;
-        }
-    }
-    return false;
+    // flush_impl must take care to correctly set the dirty() flag so this
+    // method doesn't do it
 }
 
 template <typename T_elem>

include/openPMD/backend/PatchRecordComponent.hpp

@@ -143,7 +143,7 @@ inline void PatchRecordComponent::load(std::shared_ptr<T> data)
     dRead.dtype = getDatatype();
     dRead.data = std::static_pointer_cast<void>(data);
     auto &rc = get();
-    rc.m_chunks.push(IOTask(this, dRead));
+    rc.push_chunk(IOTask(this, dRead));
 }
 
 template <typename T>
@@ -182,7 +182,7 @@ inline void PatchRecordComponent::store(uint64_t idx, T data)
     dWrite.dtype = dtype;
     dWrite.data = std::make_shared<T>(data);
     auto &rc = get();
-    rc.m_chunks.push(IOTask(this, std::move(dWrite)));
+    rc.push_chunk(IOTask(this, std::move(dWrite)));
 }
 
 template <typename T>
@@ -211,6 +211,6 @@ inline void PatchRecordComponent::store(T data)
     dWrite.dtype = dtype;
     dWrite.data = std::make_shared<T>(data);
     auto &rc = get();
-    rc.m_chunks.push(IOTask(this, std::move(dWrite)));
+    rc.push_chunk(IOTask(this, std::move(dWrite)));
 }
 } // namespace openPMD

include/openPMD/backend/Writable.hpp

@@ -44,6 +44,7 @@ template <typename FilePositionType>
 class AbstractIOHandlerImplCommon;
 template <typename>
 class Span;
+class Series;
 
 namespace internal
 {
@@ -55,6 +56,11 @@ namespace detail
     class ADIOS2File;
 }
 
+namespace debug
+{
+    void printDirty(Series const &);
+}
+
 /** @brief Layer to mirror structure of logical data and persistent data in
  * file.
  *
@@ -94,6 +100,7 @@ class Writable final
     friend std::string concrete_bp1_file_position(Writable *);
     template <typename>
     friend class Span;
+    friend void debug::printDirty(Series const &);
 
 private:
     Writable(internal::AttributableData *);
@@ -135,7 +142,25 @@ OPENPMD_private
         IOHandler = nullptr;
     internal::AttributableData *attributable = nullptr;
     Writable *parent = nullptr;
-    bool dirty = true;
+
+    /** Tracks if there are unwritten changes for this specific Writable.
+     *
+     * Manipulate via Attributable::dirty() and Attributable::setDirty().
+     */
+    bool dirtySelf = true;
+    /**
+     * Tracks if there are unwritten changes anywhere in the
+     * tree whose ancestor this Writable is.
+     *
+     * Invariant: this->dirtyRecursive implies parent->dirtyRecursive.
+     *
+     * dirtySelf and dirtyRecursive are separated since that allows specifying
+     * that `this` is not dirty, but some child is.
+     *
+     * Manipulate via Attributable::dirtyRecursive() and
+     * Attributable::setDirtyRecursive().
+     */
+    bool dirtyRecursive = true;
     /**
      * If parent is not null, then this is a key such that:
      * &(*parent)[key] == this

src/IO/ADIOS/ADIOS2File.cpp

@@ -20,6 +20,7 @@
  */
 
 #include "openPMD/IO/ADIOS/ADIOS2File.hpp"
+#include "openPMD/Error.hpp"
 #include "openPMD/IO/ADIOS/ADIOS2IOHandler.hpp"
 #include "openPMD/auxiliary/Environment.hpp"
 
@@ -1188,7 +1189,12 @@ AdvanceStatus ADIOS2File::advance(AdvanceMode mode)
 
 void ADIOS2File::drop()
 {
-    m_buffer.clear();
+    if (!m_buffer.empty())
+    {
+        throw error::Internal(
+            "ADIOS2 backend: File data for '" + m_file +
+            "' dropped, but there were enqueued operations.");
+    }
 }
 
 static std::vector<std::string> availableAttributesOrVariablesPrefixed(

src/IO/ADIOS/ADIOS2IOHandler.cpp

@@ -543,6 +543,7 @@ ADIOS2IOHandlerImpl::flush(internal::ParsedFlushParams &flushParams)
             p.second->drop();
         }
     }
+    m_dirty.clear();
     return res;
 }