Faster change tracking

[Tristan Croll]

I've been playing around to see how coordinates in particular, and changes to atomic properties in general can be made a bit faster. The below changes lead to some quite reasonable gains in performance, without a huge increase in complexity. Timings for a 188k atom structure:

m = session.models.list[0]
a = m.atoms
coords = a.coords
%timeit a.coords = coords

existing code:
58.1 +/- 5.54 ms per iteration
new code:
23.5 +/- 1.37 ms per iteration

(Running a.coords +=0.1 on the new_frame trigger for 200 frames, sphere representation with simple lighting)

existing code:
250 +/- 30 ms per iteration
new code:
156 +/- 20 ms per iteration

Key changes:

• Modified ChangeTracker.h to only fill _global_type_changes when get_global_changes() is called - avoids doubling up on logic and seems to work out quite a bit faster. For instance, a.colors = colors now takes 4.12ms as opposed to 5.16ms without making any changes in molc.cpp. Without any of the other changes below, a.coords = coords is reduced from ~58 to ~30ms.
• added a boolean track_changes flag to Atom.set_coord()
• added a new function add_modified_set(Structure* s, const std::vector<C*>& ptrs, const std::string& reason) to ChangeTracker
• modified set_atom_coords() in molc.cpp to accumulate the atoms into a std::unordered_map<Structure*, std::vector<Atom*>>, then call add_modified_set() once done.

Sorry for poking around - this started because I was looking for tips on implementing a change tracker for ISOLDE's restraints classes, and kinda snowballed from there.

diff --git a/src/core/atomic/atomstruct_cpp/Atom.cpp b/src/core/atomic/atomstruct_cpp/Atom.cpp
index 946b4cd..a36a073 100644
--- a/src/core/atomic/atomstruct_cpp/Atom.cpp
+++ b/src/core/atomic/atomstruct_cpp/Atom.cpp
@@ -101,7 +101,7 @@ Atom::aniso_u() const
 }
 
 void
-Atom::_coordset_set_coord(const Point &coord)
+Atom::_coordset_set_coord(const Point &coord, bool track_change)
 {
     CoordSet *cs = structure()->active_coord_set();
     if (cs == nullptr) {
@@ -111,11 +111,11 @@ Atom::_coordset_set_coord(const Point &coord)
             cs = structure()->new_coord_set();
         structure()->set_active_coord_set(cs);
     }
-    set_coord(coord, cs);
+    set_coord(coord, cs, track_change);
 }
 
 void
-Atom::_coordset_set_coord(const Point &coord, CoordSet *cs)
+Atom::_coordset_set_coord(const Point &coord, CoordSet *cs, bool track_change)
 {
     if (structure()->active_coord_set() == nullptr)
         structure()->set_active_coord_set(cs);
@@ -135,10 +135,13 @@ Atom::_coordset_set_coord(const Point &coord, CoordSet *cs)
         graphics_changes()->set_gc_shape();
         graphics_changes()->set_gc_ribbon();
     } else {
-        cs->_coords[_coord_index] = coord;
-        graphics_changes()->set_gc_shape();
-        graphics_changes()->set_gc_ribbon();
-        change_tracker()->add_modified(structure(), cs, ChangeTracker::REASON_COORDSET);
+        //cs->_coords[_coord_index] = coord;
+        cs->_coords[_coord_index].set_xyz(coord[0],coord[1],coord[2]);
+        if (track_change) {
+            graphics_changes()->set_gc_shape();
+            graphics_changes()->set_gc_ribbon();
+            change_tracker()->add_modified(structure(), cs, ChangeTracker::REASON_COORDSET);
+        }
     }
 }
 
@@ -1219,7 +1222,14 @@ Atom::set_color(const Rgba& rgba)
 void
 Atom::set_coord(const Coord& coord, CoordSet* cs)
 {
-    change_tracker()->add_modified(structure(), this, ChangeTracker::REASON_COORD);
+    set_coord(coord, cs, true);
+}
+
+void
+Atom::set_coord(const Coord& coord, CoordSet* cs, bool track_change)
+{
+    if (track_change)
+        change_tracker()->add_modified(structure(), this, ChangeTracker::REASON_COORD);
     if (cs == nullptr) {
         cs = structure()->active_coord_set();
         if (cs == nullptr) {
@@ -1237,7 +1247,7 @@ Atom::set_coord(const Coord& coord, CoordSet* cs)
         if (_coord_index == COORD_UNASSIGNED)
             _coord_index = _new_coord(coord);
     } else
-        _coordset_set_coord(coord, cs);
+        _coordset_set_coord(coord, cs, track_change);
 }
 
 void
diff --git a/src/core/atomic/atomstruct_cpp/Atom.h b/src/core/atomic/atomstruct_cpp/Atom.h
index 7688cbe..d9af900 100644
--- a/src/core/atomic/atomstruct_cpp/Atom.h
+++ b/src/core/atomic/atomstruct_cpp/Atom.h
@@ -115,8 +115,8 @@ private:
     Bonds  _bonds; // _bonds/_neighbors in same order
     mutable AtomType  _computed_idatm_type;
     unsigned int  _coord_index;
-    void  _coordset_set_coord(const Point &);
-    void  _coordset_set_coord(const Point &, CoordSet *cs);
+    void  _coordset_set_coord(const Point &, bool track_change=false);
+    void  _coordset_set_coord(const Point &, CoordSet *cs, bool track_change=false);
     bool  _display = true;
     DrawMode  _draw_mode = DrawMode::Sphere;
     const Element*  _element;
@@ -203,8 +203,10 @@ public:
     void  set_alt_loc(char alt_loc, bool create=false, bool _from_residue=false);
     void  set_aniso_u(float u11, float u12, float u13, float u22, float u23, float u33);
     void  set_bfactor(float);
-    void  set_coord(const Point& coord) { set_coord(coord, nullptr); }
+    void  set_coord(const Point& coord) { set_coord(coord, nullptr, true); }
     void  set_coord(const Point& coord, CoordSet* cs);
+    void  set_coord(const Point& coord, bool track_change) { set_coord(coord, nullptr, track_change); }
+    void  set_coord(const Point& coord, CoordSet* cs, bool track_change);
     void  set_computed_idatm_type(const char* it);
     void  set_draw_mode(DrawMode dm);
     void  set_idatm_type(const char* it);
diff --git a/src/core/atomic/atomstruct_cpp/ChangeTracker.h b/src/core/atomic/atomstruct_cpp/ChangeTracker.h
index 4e97271..f9551e5 100644
--- a/src/core/atomic/atomstruct_cpp/ChangeTracker.h
+++ b/src/core/atomic/atomstruct_cpp/ChangeTracker.h
@@ -19,6 +19,8 @@
 #include <array>
 #include <map>
 #include <set>
+#include <vector>
+#include <algorithm>
 #include <string>
 
 #include "imex.h"
@@ -39,6 +41,8 @@ class Structure;
 
 namespace atomstruct {
 
+
+
 class ATOMSTRUCT_IMEX Changes {
 public:
     // plain "set" (rather than "unordered_set") empirically faster to add_created() and clear()
@@ -111,8 +115,8 @@ public:
     void  add_created(Structure* s, C* ptr) {
         if (_discarding)
             return;
-        auto& g_changes = _global_type_changes[_ptr_to_type(ptr)];
-        g_changes.created.insert(ptr);
+        //~ auto& g_changes = _global_type_changes[_ptr_to_type(ptr)];
+        //~ g_changes.created.insert(ptr);
         if (_structure_okay(s)) {
             auto& s_changes = _structure_type_changes[s][_ptr_to_type(ptr)];
             s_changes.created.insert(ptr);
@@ -125,11 +129,11 @@ public:
     void  add_created(Structure* s, const std::set<C*>& ptrs) {
         if (_discarding)
             return;
-        auto& g_changes = _global_type_changes[_ptr_to_type(static_cast<typename std::set<C*>::value_type>(nullptr))];
+        //~ auto& g_changes = _global_type_changes[_ptr_to_type(static_cast<typename std::set<C*>::value_type>(nullptr))];
         // looping through and inserting individually empirically faster than the commented-out
         //   single call below, possibly due to the generic nature of that call
-        for (auto ptr: ptrs)
-            g_changes.created.insert(ptr);
+        //~ for (auto ptr: ptrs)
+            //~ g_changes.created.insert(ptr);
         //g_changes.created.insert(ptrs.begin(), ptrs.end());
         if (_structure_okay(s)) {
             auto& s_changes = _structure_type_changes[s]
@@ -143,14 +147,14 @@ public:
     void  add_modified(Structure* s, C* ptr, const std::string& reason) {
         if (_discarding)
             return;
-        auto& g_changes = _global_type_changes[_ptr_to_type(ptr)];
-        if (g_changes.created.find(static_cast<const void*>(ptr)) == g_changes.created.end()) {
-            // newly created objects don't also go in modified set
-            g_changes.modified.insert(ptr);
-            g_changes.reasons.insert(reason);
-        }
+        //~ auto& g_changes = _global_type_changes[_ptr_to_type(ptr)];
+        //~ if (g_changes.created.find(static_cast<const void*>(ptr)) == g_changes.created.end()) {
+            //~ // newly created objects don't also go in modified set
+            //~ g_changes.modified.insert(ptr);
+            //~ g_changes.reasons.insert(reason);
+        //~ }
         if (_structure_okay(s)) {
-        auto& s_changes = _structure_type_changes[s][_ptr_to_type(ptr)];
+            auto& s_changes = _structure_type_changes[s][_ptr_to_type(ptr)];
             if (s_changes.created.find(static_cast<const void*>(ptr)) == s_changes.created.end()) {
                 // newly created objects don't also go in modified set
                 s_changes.modified.insert(ptr);
@@ -163,13 +167,13 @@ public:
     void  add_modified(Structure* s, C* ptr, const std::string& reason, const std::string& reason2) {
         if (_discarding)
             return;
-        auto& g_changes = _global_type_changes[_ptr_to_type(ptr)];
-        if (g_changes.created.find(static_cast<const void*>(ptr)) == g_changes.created.end()) {
-            // newly created objects don't also go in modified set
-            g_changes.modified.insert(ptr);
-            g_changes.reasons.insert(reason);
-            g_changes.reasons.insert(reason2);
-        }
+        //~ auto& g_changes = _global_type_changes[_ptr_to_type(ptr)];
+        //~ if (g_changes.created.find(static_cast<const void*>(ptr)) == g_changes.created.end()) {
+            //~ // newly created objects don't also go in modified set
+            //~ g_changes.modified.insert(ptr);
+            //~ g_changes.reasons.insert(reason);
+            //~ g_changes.reasons.insert(reason2);
+        //~ }
         if (_structure_okay(s)) {
             auto& s_changes = _structure_type_changes[s][_ptr_to_type(ptr)];
             if (s_changes.created.find(static_cast<const void*>(ptr)) == s_changes.created.end()) {
@@ -180,15 +184,44 @@ public:
             }
         }
     }
+    
+    template<class C> void add_modified_set(Structure* s, const std::vector<C*>& ptrs, const std::string& reason) {
+        if (_discarding)
+            return;
+        //~ auto& g_changes = _global_type_changes[_ptr_to_type(static_cast<typename std::set<C*>::value_type>(nullptr))];
+        //~ auto &g_mod = g_changes.modified;
+        //~ for (auto p: ptrs) {
+            //~ g_mod.insert(p);
+        //~ }
+        //~ g_changes.reasons.insert(reason);
+        if (_structure_okay(s)) {
+            auto& s_changes = _structure_type_changes[s][_ptr_to_type(static_cast<typename std::set<C*>::value_type>(nullptr))];
+            auto& s_created = s_changes.created;
+            auto& s_mod = s_changes.modified;
+            if (s_created.size()) {
+                for (auto p: ptrs) {
+                    if (s_created.find(static_cast<const void *>(p)) == s_created.end())
+                        s_mod.insert(p);
+                }
+            } else {
+                for (auto p: ptrs) {
+                    s_mod.insert(p);
+                }
+            }
+            s_changes.reasons.insert(reason);
+        }
+        
+    }
+        
 
     template<class C>
     void  add_deleted(Structure* s, C* ptr) {
         if (_discarding)
             return;
-        auto& g_changes = _global_type_changes[_ptr_to_type(ptr)];
-        ++g_changes.num_deleted;
-        g_changes.created.erase(ptr);
-        g_changes.modified.erase(ptr);
+        //~ auto& g_changes = _global_type_changes[_ptr_to_type(ptr)];
+        //~ ++g_changes.num_deleted;
+        //~ g_changes.created.erase(ptr);
+        //~ g_changes.modified.erase(ptr);
         if (s == static_cast<void*>(ptr)) {
             _structure_type_changes.erase(s);
             _dead_structures.insert(s);
@@ -201,20 +234,48 @@ public:
     }
 
     bool  changed() const {
-        for (auto& changes: _global_type_changes) {
-            if (changes.changed())
-                return true;
+        for (auto &it: _structure_type_changes) {
+            for (auto &changes: it.second)
+                if (changes.changed())
+                    return true;
         }
         return false;
     }
+            
+        //~ for (auto& changes: _global_type_changes) {
+            //~ if (changes.changed())
+                //~ return true;
+        //~ }
+        //~ return false;
+    //~ }
     void  clear() {
-        for (auto& changes: _global_type_changes) changes.clear();
+        //~ for (auto& changes: _global_type_changes) changes.clear();
         _structure_type_changes.clear();
         _dead_structures.clear();
     }
-    const ChangesArray&  get_global_changes() const {
+    //~ const ChangesArray&  get_global_changes() const {
+        //~ return _global_type_changes;
+    //~ }
+    
+    const ChangesArray& get_global_changes() {
+        for (auto& changes: _global_type_changes) changes.clear();
+        for (auto &it: _structure_type_changes) {
+            auto &this_arr = it.second;
+            for (size_t i=0; i<_num_types; ++i) {
+                auto &target = _global_type_changes[i];
+                auto &from = this_arr[i];                
+                for (auto ptr: from.created)
+                    target.created.insert(ptr);
+                for (auto ptr: from.modified)
+                    target.modified.insert(ptr);
+                for (auto &reason: from.reasons)
+                    target.reasons.insert(reason);
+            }
+        }
         return _global_type_changes;
     }
+            
+    
     const std::map<Structure*, ChangesArray>&  get_structure_changes() const {
         return _structure_type_changes;
     }
diff --git a/src/core/atomic/molc.cpp b/src/core/atomic/molc.cpp
index b1762cc..ad31d1c 100755
--- a/src/core/atomic/molc.cpp
+++ b/src/core/atomic/molc.cpp
@@ -463,13 +463,44 @@ extern "C" EXPORT void atom_coord(void *atoms, size_t n, float64_t *xyz)
     }
 }
 
+//~ extern "C" EXPORT void set_atom_coord(void *atoms, size_t n, float64_t *xyz)
+//~ {
+    //~ Atom **a = static_cast<Atom **>(atoms);
+    //~ bool track_change=true;
+    //~ Coord coord;
+    //~ try {
+        //~ for (size_t i = 0; i != n; ++i) {
+            //~ coord.set_xyz(*xyz, *(xyz+1), *(xyz+2));
+            //~ (*a++)->set_coord(coord, track_change);
+            //~ xyz+=3;
+        //~ }
+    //~ } catch (...) {
+        //~ molc_error();
+    //~ }
+//~ }
+
+
 extern "C" EXPORT void set_atom_coord(void *atoms, size_t n, float64_t *xyz)
 {
     Atom **a = static_cast<Atom **>(atoms);
+    bool track_change=false;
+    std::unordered_map<Structure*, std::vector<Atom*> > s_map;
+    Coord coord;
     try {
         for (size_t i = 0; i != n; ++i) {
-            Real x = *xyz++, y = *xyz++, z = *xyz++;
-            a[i]->set_coord(Coord(x,y,z));
+            s_map[(*a)->structure()].push_back(*a);
+            coord.set_xyz(*xyz, *(xyz+1), *(xyz+2));
+            (*a++)->set_coord(coord, track_change);
+            xyz+=3;
+        }
+        for (auto &it: s_map) {
+            auto s = it.first;
+            auto &avec = it.second;
+            auto ct = s->change_tracker();
+            ct->add_modified_set(s, avec, ChangeTracker::REASON_COORD);
+            ct->add_modified(s, s->active_coord_set(), ChangeTracker::REASON_COORDSET);
+            s->set_gc_shape();
+            s->set_gc_ribbon();
         }
     } catch (...) {
         molc_error();

[Eric Pettersen]

Investigated the global change tracking suggestion and after testing it seems good. You did forget to set the 'num_deleted' attribute for global changes. I have committed the changes and pushed them.

Will now look into the second set of suggested changes.

--Eric

[Eric Pettersen]

The second set seems okay as well. I've committed them as well.

--Eric

[Eric Pettersen]

It turns out that constructing changes solely from structure changes discards changes to global pseudobonds and global pseudobond groups (they pass in nullptr for the Structure*). I have revamped the code to keep just those changes in the global change tracker and then supplement those with the structure changes when the changes are requested. It's in the git repository now and will be in the next build.

--Eric