diff --git a/.github/workflows/cmake.yml b/.github/workflows/cmake.yml index 857c8ea8373..e02bb74d348 100644 --- a/.github/workflows/cmake.yml +++ b/.github/workflows/cmake.yml @@ -33,6 +33,7 @@ jobs: CTEST_OUTPUT_ON_FAILURE : 1 BUILD_CONFIG : > -G Ninja + -DCMAKE_BUILD_TYPE=${{ matrix.build_type }} -DCMAKE_INSTALL_PREFIX=${{github.workspace}}/install -DCMAKE_PREFIX_PATH=/usr/local/opt/bison -DBUILD_SHARED_LIBS=OFF diff --git a/doc/libraries/parallel_runtime/futures.dox b/doc/libraries/parallel_runtime/futures.dox index cc20aa37183..1779dbc3324 100644 --- a/doc/libraries/parallel_runtime/futures.dox +++ b/doc/libraries/parallel_runtime/futures.dox @@ -77,7 +77,7 @@ second element of \c v you'll get a runtime exception. The fix (other than using arrays) is to initialize STL vectors and other containers from the special element returned by \c Future::default_initializer(), which if passed into the copy -constructor will cause it to behave just like the default contructor. +constructor will cause it to behave just like the default constructor. Thus, the following code is what you actually need to use an STL vector of futures \code diff --git a/doc/tutorial/test_runtime.cpp b/doc/tutorial/test_runtime.cpp index ecdc592255d..453b5c2cd9f 100644 --- a/doc/tutorial/test_runtime.cpp +++ b/doc/tutorial/test_runtime.cpp @@ -1,4 +1,5 @@ #include +#include int task (int i) { return i + 1; } @@ -9,18 +10,14 @@ int main(int argc, char* argv[]) { const auto me = world.rank(); const auto nproc = world.size(); - Future f; if (me == 0) { - f = world.taskq.add(&task, 0); - for (auto t = 1; t != 1000; ++t) { + for (auto t = 0; t != 1000; ++t) { const auto proc = t % nproc; - f = world.taskq.add( - proc, &task, f); + auto f = world.taskq.add(proc, &task, t); + assert(f.get() == t+1); } } world.gop.fence(); - if (f.get() != 1000) - abort(); finalize(); return 0; diff --git a/src/apps/dirac/fcwf.cc b/src/apps/dirac/fcwf.cc index ab81953ca31..dc03e7794bb 100644 --- a/src/apps/dirac/fcwf.cc +++ b/src/apps/dirac/fcwf.cc @@ -77,7 +77,7 @@ unsigned int Fcwf::size() const { return m_psi.size(); } -//copy contructor defaults to deep copy +//copy constructor defaults to deep copy //if this ever changes, you will need to change the copy() function, as it calls this Fcwf::Fcwf(const Fcwf& phi){ MADNESS_ASSERT(m_psi.size() == 0); diff --git a/src/examples/hatom_sf_dirac.cc b/src/examples/hatom_sf_dirac.cc index 41e284ed181..f6d42f6d916 100644 --- a/src/examples/hatom_sf_dirac.cc +++ b/src/examples/hatom_sf_dirac.cc @@ -294,7 +294,7 @@ void run(World& world) { gradV[axis] = real_factory_3d(world).functor(p).truncate_mode(0); } - + coord_3d lo = {0.0,0.0,-L}, hi = {0.0,0.0,L}; const int npt = 1001; diff --git a/src/madness/chem/CCStructures.h b/src/madness/chem/CCStructures.h index 4b84f8e694e..8cfd94dee5c 100644 --- a/src/madness/chem/CCStructures.h +++ b/src/madness/chem/CCStructures.h @@ -138,7 +138,7 @@ struct CCMessenger { /// Timer Structure struct CCTimer { - /// TDA_TIMER contructor + /// TDA_TIMER constructor /// @param[in] world the world /// @param[in] msg a string that contains the desired printout when info function is called CCTimer(World& world, std::string msg) : world(world), start_wall(wall_time()), start_cpu(cpu_time()), diff --git a/src/madness/mra/convolution1d.h b/src/madness/mra/convolution1d.h index 3f3926320b4..247cc667b67 100644 --- a/src/madness/mra/convolution1d.h +++ b/src/madness/mra/convolution1d.h @@ -862,12 +862,13 @@ namespace madness { iterator it = map.find(key); if (it == map.end()) { - map.insert(datumT(key, std::make_shared< GaussianConvolution1D >(k, + [[maybe_unused]] auto&& [tmpit, inserted] = map.insert(datumT(key, std::make_shared< GaussianConvolution1D >(k, Q(sqrt(expnt/constants::pi)), expnt, m, periodic ))); + MADNESS_ASSERT(inserted); it = map.find(key); //printf("conv1d: making %d %.8e\n",k,expnt); } diff --git a/src/madness/mra/funcimpl.h b/src/madness/mra/funcimpl.h index dcf44176fee..f719ab7db9f 100644 --- a/src/madness/mra/funcimpl.h +++ b/src/madness/mra/funcimpl.h @@ -5377,7 +5377,7 @@ namespace madness { const keyT& key = it->first; const FunctionNode& node = it->second; if (node.has_coeff()) { - map->insert(acc,key); + [[maybe_unused]] auto inserted = map->insert(acc,key); acc->second.push_back(std::make_pair(index,&(node.coeff()))); } } @@ -6641,9 +6641,12 @@ namespace madness { ar & id; World* world = World::world_from_id(id.get_world_id()); MADNESS_ASSERT(world); - ptr = static_cast< const FunctionImpl*>(world->ptr_from_id< WorldObject< FunctionImpl > >(id)); + auto ptr_opt = world->ptr_from_id< WorldObject< FunctionImpl > >(id); + if (!ptr_opt) + MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use a locally uninitialized object",0); + ptr = static_cast< const FunctionImpl*>(*ptr_opt); if (!ptr) - MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use a locally uninitialized object",0); + MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use an unregistered object",0); } else { ptr=nullptr; } @@ -6669,9 +6672,12 @@ namespace madness { ar & id; World* world = World::world_from_id(id.get_world_id()); MADNESS_ASSERT(world); - ptr = static_cast< FunctionImpl*>(world->ptr_from_id< WorldObject< FunctionImpl > >(id)); + auto ptr_opt = world->ptr_from_id< WorldObject< FunctionImpl > >(id); + if (!ptr_opt) + MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use a locally uninitialized object",0); + ptr = static_cast< FunctionImpl*>(*ptr_opt); if (!ptr) - MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use a locally uninitialized object",0); + MADNESS_EXCEPTION("FunctionImpl: remote operation attempting to use an unregistered object",0); } else { ptr=nullptr; } diff --git a/src/madness/mra/function_common_data.h b/src/madness/mra/function_common_data.h index 7597cdea35f..03fcd8ac3a2 100644 --- a/src/madness/mra/function_common_data.h +++ b/src/madness/mra/function_common_data.h @@ -188,7 +188,7 @@ namespace madness { if (found) { acc->second+=time; } else { - map2.insert(std::pair(-10,time)); + [[maybe_unused]] auto&& [it, inserted] = map2.insert(std::pair(-10,time)); } @@ -203,7 +203,7 @@ namespace madness { if (found) { acc->second+=1.0; } else { - map2.insert(std::pair(ilog,1)); + [[maybe_unused]] auto&& [it, inserted] = map2.insert(std::pair(ilog,1)); } } diff --git a/src/madness/mra/simplecache.h b/src/madness/mra/simplecache.h index 3ed2b39c7da..7aec2bff8fb 100644 --- a/src/madness/mra/simplecache.h +++ b/src/madness/mra/simplecache.h @@ -90,7 +90,7 @@ namespace madness { /// Set value associated with key ... gives ownership of a new copy to the container inline void set(const Key& key, const Q& val) { - cache.insert(pairT(key,val)); + [[maybe_unused]] auto&& [it, inserted] = cache.insert(pairT(key,val)); } inline void set(Level n, Translation l, const Q& val) { diff --git a/src/madness/world/future.h b/src/madness/world/future.h index cc07f9e7d7a..b93dadf70d8 100644 --- a/src/madness/world/future.h +++ b/src/madness/world/future.h @@ -531,7 +531,7 @@ namespace madness { /// \param[in] value The value to be assigned. inline void set(const T& value) { MADNESS_CHECK(f); - std::shared_ptr< FutureImpl > ff = f; // manage life time of f + std::shared_ptr< FutureImpl > ff = f; // manage lifetime of f ff->set(value); } @@ -541,7 +541,7 @@ namespace madness { /// \param[in] value The value to be assigned. inline void set(T&& value) { MADNESS_CHECK(f); - std::shared_ptr< FutureImpl > ff = f; // manage life time of f + std::shared_ptr< FutureImpl > ff = f; // manage lifetime of f ff->set(std::move(value)); } @@ -552,7 +552,7 @@ namespace madness { /// \param[in] input_arch Description needed. inline void set(const archive::BufferInputArchive& input_arch) { MADNESS_CHECK(f); - std::shared_ptr< FutureImpl > ff = f; // manage life time of f + std::shared_ptr< FutureImpl > ff = f; // manage lifetime of f ff->set(input_arch); } diff --git a/src/madness/world/group.cc b/src/madness/world/group.cc index 82d648fb68a..a190e1b0bd6 100644 --- a/src/madness/world/group.cc +++ b/src/madness/world/group.cc @@ -77,7 +77,8 @@ namespace madness { /// \param group The group to be removed from the registry void Group::unregister_group(const DistributedID& did) { group_registry_container::accessor acc; - group_registry.find(acc, did); + [[maybe_unused]] auto found = group_registry.find(acc, did); + MADNESS_ASSERT(found); group_registry.erase(acc); } diff --git a/src/madness/world/test_hashthreaded.cc b/src/madness/world/test_hashthreaded.cc index 28ee8f0ce23..7f1faacbd08 100644 --- a/src/madness/world/test_hashthreaded.cc +++ b/src/madness/world/test_hashthreaded.cc @@ -92,7 +92,10 @@ void test_coverage() { if (a[-1] != -99) cout << "was expecting -99 " << a[-1] << endl; if (a.size() != 1) cout << "was expecting size to be 1" << endl; - for (int i=0; i<10000; ++i) a.insert(datumT(i,i*99)); + for (int i=0; i<10000; ++i) { + [[maybe_unused]] auto&& [it, inserted] = a.insert(datumT(i,i*99)); + MADNESS_ASSERT(inserted); + } for (int i=0; i<10000; ++i) { pair r = a.insert(datumT(i,i*99)); @@ -134,7 +137,8 @@ void test_coverage() { count = 10001; for (int i=0; i<2000; i+=2) { iteratorT it = a.find(i); - a.erase(i); + [[maybe_unused]] auto erased = a.try_erase(i); + MADNESS_ASSERT(erased); count--; if (a.size() != count) cout << "size should have been " << count << " " << a.size() << endl; it = a.find(i); @@ -143,7 +147,7 @@ void test_coverage() { for (int i=2001; i<4000; i+=2) { iteratorT it = a.find(i); - if (a.erase(i) != 1) cout << "expected to have deleted one element " << i << endl; + if (!a.try_erase(i)) cout << "expected to have deleted one element " << i << endl; count--; if (a.size() != count) cout << "c. size should have been " << count << " " << a.size() << endl; it = a.find(i); @@ -167,7 +171,10 @@ void test_coverage() { for (int nelem=1; nelem<=10000; nelem*=10) { cout << "nelem " << nelem << endl; a.clear(); - for (int i=0; i v = random_perm(nentries); double insert_used = madness::cpu_time(); for (int i=0; i& a, size_t& count, double& sum } } else { - int ndel = a.erase(key); - if (ndel == 1) { + auto erased = a.try_erase(key); + if (erased) { sum -= value; count --; } diff --git a/src/madness/world/test_world.cc b/src/madness/world/test_world.cc index 3d6a2a4b132..24d9d696bd8 100644 --- a/src/madness/world/test_world.cc +++ b/src/madness/world/test_world.cc @@ -419,92 +419,120 @@ class Foo : public WorldObject { void test6(World& world) { PROFILE_FUNC; - ProcessID me = world.rank(); - ProcessID nproc = world.nproc(); - world.srand(73); // Everyone needs the same seed - Foo a(world, me*100); - const auto dbuf_sum = std::accumulate(a.dbuf().begin(), a.dbuf().end(), 0.0); - - if (me == 0) { + uniqueidT id; + { + ProcessID me = world.rank(); + ProcessID nproc = world.nproc(); + world.srand(73); // Everyone needs the same seed + Foo a(world, me * 100); + id = a.id(); + MADNESS_CHECK(world.ptr_from_id(id) && world.ptr_from_id(id).value() == &a); + MADNESS_CHECK(world.id_from_ptr(&a) && world.id_from_ptr(&a).value() == id); + const auto dbuf_sum = + std::accumulate(a.dbuf().begin(), a.dbuf().end(), 0.0); + + if (me == 0) { print(a.id()); - for (ProcessID p=0; p(1)).get() == p*100+1); + MADNESS_CHECK(a.send(p, &Foo::get1, 1).get() == p * 100 + 1); + MADNESS_CHECK(a.task(p, &Foo::get1, Future(1)).get() == + p * 100 + 1); - MADNESS_CHECK(a.send(p,&Foo::get2,1,2).get() == p*100+3); - MADNESS_CHECK(a.task(p,&Foo::get2,1,2).get() == p*100+3); + MADNESS_CHECK(a.send(p, &Foo::get2, 1, 2).get() == p * 100 + 3); + MADNESS_CHECK(a.task(p, &Foo::get2, 1, 2).get() == p * 100 + 3); - MADNESS_CHECK(a.send(p,&Foo::get3,1,2,3).get() == p*100+6); - MADNESS_CHECK(a.task(p,&Foo::get3,1,2,3).get() == p*100+6); + MADNESS_CHECK(a.send(p, &Foo::get3, 1, 2, 3).get() == p * 100 + 6); + MADNESS_CHECK(a.task(p, &Foo::get3, 1, 2, 3).get() == p * 100 + 6); - MADNESS_CHECK(a.send(p,&Foo::get4,1,2,3,4).get() == p*100+10); - MADNESS_CHECK(a.task(p,&Foo::get4,1,2,3,4).get() == p*100+10); + MADNESS_CHECK(a.send(p, &Foo::get4, 1, 2, 3, 4).get() == + p * 100 + 10); + MADNESS_CHECK(a.task(p, &Foo::get4, 1, 2, 3, 4).get() == + p * 100 + 10); - MADNESS_CHECK(a.send(p,&Foo::get5,1,2,3,4,5).get() == p*100+15); - MADNESS_CHECK(a.task(p,&Foo::get5,1,2,3,4,5).get() == p*100+15); + MADNESS_CHECK(a.send(p, &Foo::get5, 1, 2, 3, 4, 5).get() == + p * 100 + 15); + MADNESS_CHECK(a.task(p, &Foo::get5, 1, 2, 3, 4, 5).get() == + p * 100 + 15); - MADNESS_CHECK(a.task(p,&Foo::getbuf0,a.dbuf()).get() == p*100+dbuf_sum); + MADNESS_CHECK(a.task(p, &Foo::getbuf0, a.dbuf()).get() == + p * 100 + dbuf_sum); - MADNESS_CHECK(a.send(p,&Foo::get0c).get() == p*100); - MADNESS_CHECK(a.task(p,&Foo::get0c).get() == p*100); + MADNESS_CHECK(a.send(p, &Foo::get0c).get() == p * 100); + MADNESS_CHECK(a.task(p, &Foo::get0c).get() == p * 100); - MADNESS_CHECK(a.send(p,&Foo::get1c,1).get() == p*100+1); - MADNESS_CHECK(a.task(p,&Foo::get1c,1).get() == p*100+1); + MADNESS_CHECK(a.send(p, &Foo::get1c, 1).get() == p * 100 + 1); + MADNESS_CHECK(a.task(p, &Foo::get1c, 1).get() == p * 100 + 1); - MADNESS_CHECK(a.send(p,&Foo::get2c,1,2).get() == p*100+3); - MADNESS_CHECK(a.task(p,&Foo::get2c,1,2).get() == p*100+3); + MADNESS_CHECK(a.send(p, &Foo::get2c, 1, 2).get() == p * 100 + 3); + MADNESS_CHECK(a.task(p, &Foo::get2c, 1, 2).get() == p * 100 + 3); - MADNESS_CHECK(a.send(p,&Foo::get3c,1,2,3).get() == p*100+6); - MADNESS_CHECK(a.task(p,&Foo::get3c,1,2,3).get() == p*100+6); + MADNESS_CHECK(a.send(p, &Foo::get3c, 1, 2, 3).get() == p * 100 + 6); + MADNESS_CHECK(a.task(p, &Foo::get3c, 1, 2, 3).get() == p * 100 + 6); - MADNESS_CHECK(a.send(p,&Foo::get4c,1,2,3,4).get() == p*100+10); - MADNESS_CHECK(a.task(p,&Foo::get4c,1,2,3,4).get() == p*100+10); + MADNESS_CHECK(a.send(p, &Foo::get4c, 1, 2, 3, 4).get() == + p * 100 + 10); + MADNESS_CHECK(a.task(p, &Foo::get4c, 1, 2, 3, 4).get() == + p * 100 + 10); - MADNESS_CHECK(a.send(p,&Foo::get5c,1,2,3,4,5).get() == p*100+15); - MADNESS_CHECK(a.task(p,&Foo::get5c,1,2,3,4,5).get() == p*100+15); + MADNESS_CHECK(a.send(p, &Foo::get5c, 1, 2, 3, 4, 5).get() == + p * 100 + 15); + MADNESS_CHECK(a.task(p, &Foo::get5c, 1, 2, 3, 4, 5).get() == + p * 100 + 15); - MADNESS_CHECK(a.task(p,&Foo::getbuf0c,a.dbuf()).get() == p*100+dbuf_sum); + MADNESS_CHECK(a.task(p, &Foo::getbuf0c, a.dbuf()).get() == + p * 100 + dbuf_sum); } - } // me == 0 + } // me == 0 - for(ProcessID p=0; p!=nproc; ++p) { - a.send(p, &Foo::ping_am, me, 1); - a.task(p, &Foo::ping, me, 1); - } + for (ProcessID p = 0; p != nproc; ++p) { + a.send(p, &Foo::ping_am, me, 1); + a.task(p, &Foo::ping, me, 1); + } - world.gop.fence(); + world.gop.fence(); - // stress the large message protocol ... off by default - if (0) { - const auto dbuf_sum_long = std::accumulate(a.dbuf_long().begin(), a.dbuf_long().end(), 0.0); - const auto dbuf_sum_short = std::accumulate(a.dbuf_short().begin(), a.dbuf_short().end(), 0.0); + // stress the large message protocol ... off by default + if (0) { + const auto dbuf_sum_long = + std::accumulate(a.dbuf_long().begin(), a.dbuf_long().end(), 0.0); + const auto dbuf_sum_short = + std::accumulate(a.dbuf_short().begin(), a.dbuf_short().end(), 0.0); #if 0 // uncomment to STRESS the large msg protocol const size_t nmsg = 128; #else - const size_t nmsg = 1; + const size_t nmsg = 1; #endif - std::vector> results; - std::vector results_ref; - for(size_t m=0; m!=nmsg; ++m) { - for (ProcessID p=0; p> results; + std::vector results_ref; + for (size_t m = 0; m != nmsg; ++m) { + for (ProcessID p = 0; p < nproc; ++p) { + results.push_back(a.task(p, &Foo::getbuf0c, a.dbuf_long())); + results_ref.push_back(p * 100 + dbuf_sum_long); + results.push_back(a.task(p, &Foo::getbuf0c, a.dbuf_short())); + results_ref.push_back(p * 100 + dbuf_sum_short); + } + } + world.gop.fence(); + for (size_t r = 0; r != results.size(); r += 2) { + MADNESS_CHECK(results[r].get() == results_ref[r]); } - } - world.gop.fence(); - for(size_t r=0; r!=results.size(); r += 2) { - MADNESS_CHECK(results[r].get() == results_ref[r]); } } + // test that the object is gone + auto ptr_opt = world.ptr_from_id(id); +#ifndef NDEBUG + MADNESS_CHECK(ptr_opt && *ptr_opt == nullptr); +#else + MADNESS_CHECK(!ptr_opt); +#endif + print("test 6 (world object active message and tasks) seems to be working"); } diff --git a/src/madness/world/thread.h b/src/madness/world/thread.h index eb15cb4b825..bf50b3bc9db 100644 --- a/src/madness/world/thread.h +++ b/src/madness/world/thread.h @@ -960,7 +960,7 @@ namespace madness { count = 0; } - /// Contructor setting the specified task attributes. + /// Constructor setting the specified task attributes. /// \param[in] attr The task attributes. explicit PoolTaskInterface(const TaskAttributes& attr) diff --git a/src/madness/world/world.h b/src/madness/world/world.h index cb69bce28d8..592686daebb 100644 --- a/src/madness/world/world.h +++ b/src/madness/world/world.h @@ -55,11 +55,12 @@ // #endif // Standard C++ header files needed by MADworld.h +#include +#include // std::uint64_t #include #include // std::list +#include // std::optional #include // std::pair -#include -#include // std::uint64_t #ifdef HAVE_RANDOM #include @@ -169,7 +170,17 @@ namespace madness { typedef madness::ConcurrentHashMap map_id_to_ptrT; /// \todo Brief description of typedef needed. typedef madness::ConcurrentHashMap map_ptr_to_idT; - map_id_to_ptrT map_id_to_ptr; ///< \todo Verify: Map from the hash ID to a pointer. + + /// Maps unique ID of a global object to its local pointer + /// + /// \note Unique IDs are created by increasing `obj_id`. To be able to assign unique IDs to objects created in + /// different worlds, they must be created in exactly the same sequence in every process. Hence in general + /// object construction happens in main thread. Object construction first updates `obj_id`, then inserts ID + /// into this. The rest of code only knows about this object once it's been added to this map, hence + /// this map (NOT `obj_id`) defines the known objects. The only writers to map_id_to_ptr are + /// `{register,unregister}_ptr()`. + map_id_to_ptrT map_id_to_ptr; + /// inverse of map_id_to_ptr map_ptr_to_idT map_ptr_to_id; ///< \todo Verify: Map from a pointer to its unique hash ID. @@ -316,7 +327,7 @@ namespace madness { /// \return The number of processes in this \c World. ProcessID size() const { return mpi.size(); } - /// Returns the next universe-wide unique ID for objects created in this \c World. No comms. + /// Creates a new universe-wide unique ID for objects created in this \c World. No comms. /// You should consider using \c register_ptr(), \c unregister_ptr(), /// \c id_from_ptr(), or \c ptr_from_id() rather than using this directly. @@ -325,9 +336,13 @@ namespace madness { /// every process within the current \c World in order to avoid /// synchronization. /// - /// The value obj_id=0 is guaranteed to be invalid. - /// \return The next universe-wide unique ID for objects created in this \c World. - uniqueidT unique_obj_id() { return uniqueidT(_id,obj_id++); } + /// Unique IDs are created by increasing static counter. + /// The value obj_id=0 is invalid. + /// \return A new universe-wide unique ID for objects created in this \c World. + /// \warning This is not re-entrant, should be called from a single (typically, main) thread + uniqueidT make_unique_obj_id() { + return uniqueidT(_id,obj_id++); + } /// Associate a local pointer with a universe-wide unique ID. @@ -348,65 +363,68 @@ namespace madness { /// with a unique ID. /// \return The unique ID associated with the supplied data. template - uniqueidT register_ptr(T* ptr) { + [[nodiscard]] uniqueidT register_ptr(T* ptr) { MADNESS_ASSERT(sizeof(T*) == sizeof(void *)); - uniqueidT id = unique_obj_id(); - map_id_to_ptr.insert(std::pair(id,static_cast(ptr))); - map_ptr_to_id.insert(std::pair(static_cast(ptr),id)); + uniqueidT id = make_unique_obj_id(); + [[maybe_unused]] auto&& [it1, inserted1] = map_id_to_ptr.insert(std::pair(id,static_cast(ptr))); + MADNESS_ASSERT(inserted1); + [[maybe_unused]] auto&& [it2, inserted2] = map_ptr_to_id.insert(std::pair(static_cast(ptr),id)); + MADNESS_ASSERT(inserted2); return id; } - /// Unregister the unique ID for a local pointer. - - /// \tparam T The type of data to unregister. - /// \param[in] ptr The local pointer to unregister. - /// \todo Are there any problems to report if ptr is not already registered? - template - void unregister_ptr(T* ptr) { - uniqueidT id = id_from_ptr(ptr); // Will be zero if invalid - map_id_to_ptr.erase(id); - map_ptr_to_id.erase((void *) ptr); - } - - /// Unregister the unique ID for a local pointer via its ID. + /// Unregister a global object via its the unique ID. - /// This is the same as `unregister_ptr(world.ptr_from_id(id));`. - /// \tparam T The type of data to unregister. /// \param[in] id The unique ID of the data to unregister. - template void unregister_ptr(const uniqueidT id) { - T* const ptr = ptr_from_id(id); - map_id_to_ptr.erase(id); - map_ptr_to_id.erase((void *) ptr); + // START critical section + map_id_to_ptrT::accessor acc; + [[maybe_unused]] auto found = map_id_to_ptr.find(acc, id); + MADNESS_ASSERT(found); // make sure id is valid + + void* ptr = static_cast(acc->second); + + // if NDEBUG is not defined, keep unregistered ptr IDs in map_id_to_ptr else remove them +#ifdef NDEBUG + map_id_to_ptr.erase(acc); // END critical section +#else + MADNESS_ASSERT(ptr); // ensure ID is not expired + acc->second = nullptr; + acc.release(); // END critical section +#endif + [[maybe_unused]] auto erased2 = map_ptr_to_id.try_erase(ptr); + MADNESS_ASSERT(erased2); } /// Look up a local pointer from a world-wide unique ID. /// \tparam T The type of the data to look up. /// \param[in] id The unique ID of the data. - /// \return The local pointer or \c NULL if the ID is not found. + /// \return The local pointer (can be \c nullptr if \p id has been deregistered and + /// `NDEBUG` is not `#define`d); if \p id has not been registered then returns null optional template - T* ptr_from_id(uniqueidT id) const { - map_id_to_ptrT::const_iterator it = map_id_to_ptr.find(id); - if (it == map_id_to_ptr.end()) - return nullptr; - else - return (T*)(it->second); + [[nodiscard]] std::optional ptr_from_id(uniqueidT id) const { + map_id_to_ptrT::const_accessor acc; + [[maybe_unused]] auto found = map_id_to_ptr.find(acc, id); + if (!found) + return {}; + else + return {static_cast(acc->second)}; } /// Look up an ID from a local pointer. /// \tparam T The type of the data to look up. /// \param[in] ptr The local pointer. - /// \return The unique ID or \c invalidid if the pointer is not found. + /// \return Optional containing the unique ID if the pointer is registered, or a null optional if the pointer is not registered. template - const uniqueidT& id_from_ptr(T* ptr) const { - static uniqueidT invalidid(0,0); - map_ptr_to_idT::const_iterator it = map_ptr_to_id.find(ptr); - if (it == map_ptr_to_id.end()) - return invalidid; - else - return it->second; + [[nodiscard]] std::optional id_from_ptr(T* ptr) const { + map_ptr_to_idT::const_accessor acc; + [[maybe_unused]] auto found = map_ptr_to_id.find(acc, ptr); + if (!found) + return {}; + else + return {acc->second}; } #ifndef MADNESS_DISABLE_SHARED_FROM_THIS @@ -418,18 +436,19 @@ namespace madness { /// \return The pointer or a default constructed `std::shared_ptr` if /// the ID is not found. template - std::shared_ptr shared_ptr_from_id(uniqueidT id) const { - T* ptr = ptr_from_id(id); - return (ptr ? ptr->shared_from_this() : std::shared_ptr()); + [[nodiscard]] std::shared_ptr shared_ptr_from_id(uniqueidT id) const { + std::optional ptr_opt = ptr_from_id(id); + if (ptr_opt) MADNESS_ASSERT(*ptr_opt); + return (ptr_opt ? *ptr_opt->shared_from_this() : std::shared_ptr()); } /// Look up a unique ID from a local pointer. /// \tparam T The type of the data to look up. /// \param[in] ptr The local pointer. - /// \return The unique ID or an invalid ID if the pointer is not found. + /// \return Optional containing the unique ID, if the pointer is registered, or a null optional, if the pointer is not registered. template - const uniqueidT& id_from_ptr(std::shared_ptr& ptr) const { + [[nodiscard]] std::optional id_from_ptr(std::shared_ptr& ptr) const { return id_from_ptr(ptr.get()); } diff --git a/src/madness/world/world_object.h b/src/madness/world/world_object.h index a1b508c8bb6..e2595359c5a 100644 --- a/src/madness/world/world_object.h +++ b/src/madness/world/world_object.h @@ -401,32 +401,49 @@ namespace madness { /// \return Description needed. /// \todo Parameter/return descriptions needed. static bool is_ready(const uniqueidT& id, objT*& obj, const AmArg& arg, am_handlerT ptr) { - obj = static_cast(arg.get_world()->template ptr_from_id(id)); - - if (obj) { - if (obj->ready || arg.is_pending()) return true; - } - - MADNESS_PRAGMA_CLANG(diagnostic push) - MADNESS_PRAGMA_CLANG(diagnostic ignored "-Wundefined-var-template") - - ScopedMutex lock(pending_mutex); // BEGIN CRITICAL SECTION - - if (!obj) obj = static_cast(arg.get_world()->template ptr_from_id(id)); - - if (obj) { - if (obj->ready || arg.is_pending()) - return true; // END CRITICAL SECTION - } - const_cast(arg).set_pending(); - const_cast(pending).push_back(detail::PendingMsg(id, ptr, arg)); - - MADNESS_PRAGMA_CLANG(diagnostic pop) - - return false; // END CRITICAL SECTION + std::optional opt_obj = + arg.get_world()->template ptr_from_id(id); + if (opt_obj) { + // if opt_obj == nullptr, then this ID has already been deregistered + MADNESS_ASSERT(*opt_obj != nullptr); + obj = static_cast(*opt_obj); + } else + obj = nullptr; + + if (obj) { + if (obj->ready || arg.is_pending()) + return true; + } + + MADNESS_PRAGMA_CLANG(diagnostic push) + MADNESS_PRAGMA_CLANG(diagnostic ignored "-Wundefined-var-template") + + ScopedMutex lock(pending_mutex); // BEGIN CRITICAL SECTION + + if (!obj) { + std::optional opt_obj = + arg.get_world()->template ptr_from_id(id); + if (opt_obj) { + // if opt_obj == nullptr, then this ID has already been deregistered + MADNESS_ASSERT(*opt_obj != nullptr); + obj = static_cast(*opt_obj); + } else + obj = nullptr; + } + + if (obj) { + if (obj->ready || arg.is_pending()) + return true; // END CRITICAL SECTION + } + const_cast(arg).set_pending(); + const_cast(pending).push_back( + detail::PendingMsg(id, ptr, arg)); + + MADNESS_PRAGMA_CLANG(diagnostic pop) + + return false; // END CRITICAL SECTION } - /// Handler for an incoming AM. /// \todo Descriptions needed. @@ -1359,8 +1376,8 @@ namespace madness { virtual ~WorldObject() { if(initialized()) { - MADNESS_ASSERT_NOEXCEPT(World::exists(&world) && world.ptr_from_id(id())); - world.unregister_ptr(static_cast(this)); + MADNESS_ASSERT_NOEXCEPT(World::exists(&world) && world.ptr_from_id(id()) && *world.ptr_from_id(id()) == this); + world.unregister_ptr(this->id()); } } }; @@ -1383,8 +1400,10 @@ namespace madness { ar & id; World* world = World::world_from_id(id.get_world_id()); MADNESS_ASSERT(world); - ptr = world->ptr_from_id< WorldObject >(id); - if (!ptr) MADNESS_EXCEPTION("WorldObj: remote operation attempting to use a locally uninitialized object",0); + auto ptr_opt = world->ptr_from_id< WorldObject >(id); + if (!ptr_opt) MADNESS_EXCEPTION("WorldObj: remote operation attempting to use a locally uninitialized object",0); + ptr = *ptr_opt; + if (!ptr) MADNESS_EXCEPTION("WorldObj: remote operation attempting to use a locally deregistered object",0); } }; @@ -1420,8 +1439,10 @@ namespace madness { ar & id; World* world = World::world_from_id(id.get_world_id()); MADNESS_ASSERT(world); - ptr = world->ptr_from_id< WorldObject >(id); - if (!ptr) MADNESS_EXCEPTION("WorldObj: remote operation attempting to use a locally uninitialized object",0); + auto ptr_opt = world->ptr_from_id< WorldObject >(id); + if (!ptr_opt) MADNESS_EXCEPTION("WorldObj: remote operation attempting to use a locally uninitialized object",0); + ptr = *ptr_opt; + if (!ptr) MADNESS_EXCEPTION("WorldObj: remote operation attempting to use a locally deregistered object",0); } }; diff --git a/src/madness/world/worlddc.h b/src/madness/world/worlddc.h index 9fe8611f545..d375c7b9de3 100644 --- a/src/madness/world/worlddc.h +++ b/src/madness/world/worlddc.h @@ -515,7 +515,8 @@ namespace madness { if (dest == me) { // Was using iterator ... try accessor ????? accessor acc; - local.insert(acc,datum.first); + // N.B. key might already exist if want to simply replace + [[maybe_unused]] auto inserted = local.insert(acc,datum.first); acc->second = datum.second; } else { @@ -542,7 +543,8 @@ namespace madness { void erase(const keyT& key) { ProcessID dest = owner(key); if (dest == me) { - local.erase(key); + [[maybe_unused]] auto erased = local.try_erase(key); + MADNESS_ASSERT(erased); } else { void(implT::*eraser)(const keyT&) = &implT::erase; @@ -622,7 +624,8 @@ namespace madness { MEMFUN_RETURNT(memfunT) itemfun(const keyT& key, memfunT memfun) { accessor acc; - local.insert(acc, key); + // N.B. key may already exist, this is just to ensure lock is held by acc + [[maybe_unused]] auto inserted = local.insert(acc, key); return (acc->second.*memfun)(); } @@ -631,7 +634,8 @@ namespace madness { MEMFUN_RETURNT(memfunT) itemfun(const keyT& key, memfunT memfun, const arg1T& arg1) { accessor acc; - local.insert(acc, key); + // N.B. key may already exist, this is just to ensure lock is held by acc + [[maybe_unused]] auto inserted = local.insert(acc, key); return (acc->second.*memfun)(arg1); } @@ -640,7 +644,8 @@ namespace madness { MEMFUN_RETURNT(memfunT) itemfun(const keyT& key, memfunT memfun, const arg1T& arg1, const arg2T& arg2) { accessor acc; - local.insert(acc, key); + // N.B. key may already exist, this is just to ensure lock is held by acc + [[maybe_unused]] auto inserted = local.insert(acc, key); return (acc->second.*memfun)(arg1,arg2); } @@ -649,7 +654,8 @@ namespace madness { MEMFUN_RETURNT(memfunT) itemfun(const keyT& key, memfunT memfun, const arg1T& arg1, const arg2T& arg2, const arg3T& arg3) { accessor acc; - local.insert(acc, key); + // N.B. key may already exist, this is just to ensure lock is held by acc + [[maybe_unused]] auto inserted = local.insert(acc, key); return (acc->second.*memfun)(arg1,arg2,arg3); } @@ -658,7 +664,8 @@ namespace madness { MEMFUN_RETURNT(memfunT) itemfun(const keyT& key, memfunT memfun, const arg1T& arg1, const arg2T& arg2, const arg3T& arg3, const arg4T& arg4) { accessor acc; - local.insert(acc, key); + // N.B. key may already exist, this is just to ensure lock is held by acc + [[maybe_unused]] auto inserted = local.insert(acc, key); return (acc->second.*memfun)(arg1,arg2,arg3,arg4); } @@ -667,7 +674,8 @@ namespace madness { MEMFUN_RETURNT(memfunT) itemfun(const keyT& key, memfunT memfun, const arg1T& arg1, const arg2T& arg2, const arg3T& arg3, const arg4T& arg4, const arg5T& arg5) { accessor acc; - local.insert(acc, key); + // N.B. key may already exist, this is just to ensure lock is held by acc + [[maybe_unused]] auto inserted = local.insert(acc, key); return (acc->second.*memfun)(arg1,arg2,arg3,arg4,arg5); } @@ -676,7 +684,8 @@ namespace madness { MEMFUN_RETURNT(memfunT) itemfun(const keyT& key, memfunT memfun, const arg1T& arg1, const arg2T& arg2, const arg3T& arg3, const arg4T& arg4, const arg5T& arg5, const arg6T& arg6) { accessor acc; - local.insert(acc, key); + // N.B. key may already exist, this is just to ensure lock is held by acc + [[maybe_unused]] auto inserted = local.insert(acc, key); return (acc->second.*memfun)(arg1,arg2,arg3,arg4,arg5,arg6); } @@ -686,7 +695,8 @@ namespace madness { itemfun(const keyT& key, memfunT memfun, const arg1T& arg1, const arg2T& arg2, const arg3T& arg3, const arg4T& arg4, const arg5T& arg5, const arg6T& arg6, const arg7T& arg7) { accessor acc; - local.insert(acc, key); + // N.B. key may already exist, this is just to ensure lock is held by acc + [[maybe_unused]] auto inserted = local.insert(acc, key); return (acc->second.*memfun)(arg1,arg2,arg3,arg4,arg5,arg6,arg7); } diff --git a/src/madness/world/worldhashmap.h b/src/madness/world/worldhashmap.h index 78487767c28..9c5164a09bd 100644 --- a/src/madness/world/worldhashmap.h +++ b/src/madness/world/worldhashmap.h @@ -458,20 +458,21 @@ namespace madness { this->clear(); hashfun = h.hashfun; for (const_iterator p=h.begin(); p!=h.end(); ++p) { - insert(*p); + [[maybe_unused]] auto&& [it, inserted] = insert(*p); + MADNESS_ASSERT(inserted); } } return *this; } - std::pair insert(const datumT& datum) { + [[nodiscard]] std::pair insert(const datumT& datum) { int bin = hash_to_bin(datum.first); std::pair result = bins[bin].insert(datum,entryT::NOLOCK); return std::pair(iterator(this,bin,result.first),result.second); } /// Returns true if new pair was inserted; false if key is already in the map and the datum was not inserted - bool insert(accessor& result, const datumT& datum) { + [[nodiscard]] bool insert(accessor& result, const datumT& datum) { result.release(); int bin = hash_to_bin(datum.first); std::pair r = bins[bin].insert(datum,entryT::WRITELOCK); @@ -480,7 +481,7 @@ namespace madness { } /// Returns true if new pair was inserted; false if key is already in the map and the datum was not inserted - bool insert(const_accessor& result, const datumT& datum) { + [[nodiscard]] bool insert(const_accessor& result, const datumT& datum) { result.release(); int bin = hash_to_bin(datum.first); std::pair r = bins[bin].insert(datum,entryT::READLOCK); @@ -489,23 +490,24 @@ namespace madness { } /// Returns true if new pair was inserted; false if key is already in the map - inline bool insert(accessor& result, const keyT& key) { + [[nodiscard]] inline bool insert(accessor& result, const keyT& key) { return insert(result, datumT(key,valueT())); } /// Returns true if new pair was inserted; false if key is already in the map - inline bool insert(const_accessor& result, const keyT& key) { + [[nodiscard]] inline bool insert(const_accessor& result, const keyT& key) { return insert(result, datumT(key,valueT())); } - std::size_t erase(const keyT& key) { - if (bins[hash_to_bin(key)].del(key,entryT::NOLOCK)) return 1; - else return 0; + [[nodiscard]] bool try_erase(const keyT& key) { + if (bins[hash_to_bin(key)].del(key,entryT::NOLOCK)) return true; + else return false; } void erase(const iterator& it) { if (it == end()) MADNESS_EXCEPTION("ConcurrentHashMap: erase(iterator): at end", true); - erase(it->first); + [[maybe_unused]] auto erased = try_erase(it->first); + MADNESS_ASSERT(erased); } void erase(accessor& item) { @@ -519,21 +521,21 @@ namespace madness { item.unset(); } - iterator find(const keyT& key) { + [[nodiscard]] iterator find(const keyT& key) { int bin = hash_to_bin(key); entryT* entry = bins[bin].find(key,entryT::NOLOCK); if (!entry) return end(); else return iterator(this,bin,entry); } - const_iterator find(const keyT& key) const { + [[nodiscard]] const_iterator find(const keyT& key) const { int bin = hash_to_bin(key); const entryT* entry = bins[bin].find(key,entryT::NOLOCK); if (!entry) return end(); else return const_iterator(this,bin,entry); } - bool find(accessor& result, const keyT& key) { + [[nodiscard]] bool find(accessor& result, const keyT& key) { result.release(); int bin = hash_to_bin(key); entryT* entry = bins[bin].find(key,entryT::WRITELOCK); @@ -542,7 +544,7 @@ namespace madness { return foundit; } - bool find(const_accessor& result, const keyT& key) const { + [[nodiscard]] bool find(const_accessor& result, const keyT& key) const { result.release(); int bin = hash_to_bin(key); entryT* entry = bins[bin].find(key,entryT::READLOCK); @@ -555,38 +557,38 @@ namespace madness { for (unsigned int i=0; i it = insert(datumT(key,valueT())); return it.first->second; } - iterator begin() { + [[nodiscard]] iterator begin() { return iterator(this,true); } - const_iterator begin() const { + [[nodiscard]] const_iterator begin() const { return cbegin(); } - const_iterator cbegin() const { + [[nodiscard]] const_iterator cbegin() const { return const_iterator(this,true); } - iterator end() { + [[nodiscard]] iterator end() { return iterator(this,false); } - const_iterator end() const { + [[nodiscard]] const_iterator end() const { return cend(); } - const_iterator cend() const { + [[nodiscard]] const_iterator cend() const { return const_iterator(this,false); } diff --git a/src/madness/world/worldref.h b/src/madness/world/worldref.h index d15f0735eb3..10afc56069e 100644 --- a/src/madness/world/worldref.h +++ b/src/madness/world/worldref.h @@ -274,7 +274,7 @@ namespace madness { /// \param key The key of the \c RemoteReference object to be unregistered. /// \throw MadnessException If \c key is not found in the pointer map. static void unregister_ptr_(void* key) { - std::size_t erased = pimpl_map_.erase(key); + auto erased = pimpl_map_.try_erase(key); if (!erased) MADNESS_EXCEPTION("worldref: unregister_ptr failed", erased); }