id summary reporter owner description type status priority milestone component version resolution keywords cc blockedby blocking notify_on_close platform project 1023 Faster change tracking Tristan Croll Eric Pettersen "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& ptrs, const std::string& reason)` to `ChangeTracker` - modified `set_atom_coords()` in `molc.cpp` to accumulate the atoms into a `std::unordered_map>`, 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 #include #include +#include +#include #include #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& ptrs) { if (_discarding) return; - auto& g_changes = _global_type_changes[_ptr_to_type(static_cast::value_type>(nullptr))]; + //~ auto& g_changes = _global_type_changes[_ptr_to_type(static_cast::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(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(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(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(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(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(ptr)) == s_changes.created.end()) { @@ -180,15 +184,44 @@ public: } } } + + template void add_modified_set(Structure* s, const std::vector& ptrs, const std::string& reason) { + if (_discarding) + return; + //~ auto& g_changes = _global_type_changes[_ptr_to_type(static_cast::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::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(p)) == s_created.end()) + s_mod.insert(p); + } + } else { + for (auto p: ptrs) { + s_mod.insert(p); + } + } + s_changes.reasons.insert(reason); + } + + } + template 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(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& 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(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(atoms); + bool track_change=false; + std::unordered_map > 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(); }}}" enhancement closed moderate Performance fixed all ChimeraX