diff --git a/python_tests/test_memo_no_cache.py b/python_tests/test_memo_no_cache.py index 95c36593..f264e2b2 100644 --- a/python_tests/test_memo_no_cache.py +++ b/python_tests/test_memo_no_cache.py @@ -66,7 +66,7 @@ def test_excluding_no_cache_instances_from_P0_cache(db0_fixture): gc.collect() final_cache_size = db0.get_cache_stats()["P_size"]["P0"] # make sure cache utilization is low - assert abs(final_cache_size - initial_cache_size) < (350 << 10) + assert abs(final_cache_size - initial_cache_size) < (400 << 10) def test_fetching_no_cache_objects(db0_fixture): diff --git a/python_tests/test_memo_singleton.py b/python_tests/test_memo_singleton.py index b5862d79..6ba0dbd2 100644 --- a/python_tests/test_memo_singleton.py +++ b/python_tests/test_memo_singleton.py @@ -61,8 +61,8 @@ def test_find_singleton_static_scope(db0_fixture): assert db0.find_singleton(MemoDataPxSingleton) is None obj_1 = MemoDataPxSingleton(789) assert db0.find_singleton(MemoDataPxSingleton) is obj_1 - - + + def test_find_singleton(db0_fixture): assert db0.find_singleton(MemoTestSingleton) is None # create on default prefix @@ -81,4 +81,4 @@ def test_singleton_with_migrations(db0_fixture): def test_assembling_dyn_prefix_function(db0_fixture): assert __dyn_prefix(MemoTestSingleton) is None assert __dyn_prefix(MemoScopedSingleton) is not None - \ No newline at end of file + diff --git a/src/dbzero/bindings/python/iter/PyObjectIterable.cpp b/src/dbzero/bindings/python/iter/PyObjectIterable.cpp index 86f843b5..cfa112e6 100644 --- a/src/dbzero/bindings/python/iter/PyObjectIterable.cpp +++ b/src/dbzero/bindings/python/iter/PyObjectIterable.cpp @@ -102,7 +102,7 @@ namespace db0::python auto py_iter = PyObjectIteratorDefault_new(); py_iter->makeNew(py_iterable->ext().iter()); if (auto *iterator_pool = fixture->tryGet()) { - iterator_pool->add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get())); + iterator_pool->add(py_iter->getSharedPtr()); } return py_iter.steal(); } diff --git a/src/dbzero/bindings/python/iter/PyObjectIterator.cpp b/src/dbzero/bindings/python/iter/PyObjectIterator.cpp index 7da19ffa..55efae3a 100644 --- a/src/dbzero/bindings/python/iter/PyObjectIterator.cpp +++ b/src/dbzero/bindings/python/iter/PyObjectIterator.cpp @@ -3,6 +3,7 @@ #include "PyObjectIterator.hpp" #include +#include #include #include #include @@ -23,6 +24,13 @@ namespace db0::python { PY_DEALLOC_GUARD(); PY_API_FUNC + auto iterator_ptr = self->getSharedPtr(); + if (!!iterator_ptr) { + auto fixture = iterator_ptr->getFixture(); + if (auto *iterator_pool = fixture->tryGet()) { + iterator_pool->remove(iterator_ptr.get()); + } + } // destroy associated instance self->destroy(); Py_TYPE(self)->tp_free((PyObject*)self); diff --git a/src/dbzero/object_model/tags/ObjectIteratorPool.cpp b/src/dbzero/object_model/tags/ObjectIteratorPool.cpp index f745602d..7d22b4c3 100644 --- a/src/dbzero/object_model/tags/ObjectIteratorPool.cpp +++ b/src/dbzero/object_model/tags/ObjectIteratorPool.cpp @@ -7,43 +7,43 @@ namespace db0::object_model { - ObjectIterator *ObjectIteratorPool::getIterator(ObjectSharedExtPtr const &object) + std::shared_ptr ObjectIteratorPool::getIterator(const ObjectWeakPtr &object) { - auto *py_iterator = object.get(); - if (!py_iterator) { - return nullptr; - } - auto iterator_ptr = py_iterator->getSharedPtr(); - return iterator_ptr.get(); + return object.lock(); } - void ObjectIteratorPool::add(ObjectSharedExtPtr object) + void ObjectIteratorPool::add(const std::shared_ptr &object) { if (m_closed) { return; } - if (object.get() != nullptr) { - m_iterators.push_back(std::move(object)); + if (!object) { + return; + } + m_iterators.insert_or_assign(object.get(), object); + } + + void ObjectIteratorPool::remove(ObjectIterator *object_ptr) + { + if (!object_ptr) { + return; } + m_iterators.erase(object_ptr); } std::size_t ObjectIteratorPool::detach() { std::size_t detached_count = 0; - auto out = m_iterators.begin(); - for (auto it = m_iterators.begin(); it != m_iterators.end(); ++it) { - auto *iterator = getIterator(*it); + for (auto it = m_iterators.begin(); it != m_iterators.end();) { + auto iterator = getIterator(it->second); if (!iterator) { + it = m_iterators.erase(it); continue; } iterator->detach(); ++detached_count; - if (out != it) { - *out = std::move(*it); - } - ++out; + ++it; } - m_iterators.erase(out, m_iterators.end()); return detached_count; } @@ -59,17 +59,13 @@ namespace db0::object_model std::size_t ObjectIteratorPool::cleanup() { auto old_size = m_iterators.size(); - auto out = m_iterators.begin(); - for (auto it = m_iterators.begin(); it != m_iterators.end(); ++it) { - if (!getIterator(*it)) { - continue; - } - if (out != it) { - *out = std::move(*it); + for (auto it = m_iterators.begin(); it != m_iterators.end();) { + if (!getIterator(it->second)) { + it = m_iterators.erase(it); + } else { + ++it; } - ++out; } - m_iterators.erase(out, m_iterators.end()); return old_size - m_iterators.size(); } diff --git a/src/dbzero/object_model/tags/ObjectIteratorPool.hpp b/src/dbzero/object_model/tags/ObjectIteratorPool.hpp index 2606ed84..3d7086ad 100644 --- a/src/dbzero/object_model/tags/ObjectIteratorPool.hpp +++ b/src/dbzero/object_model/tags/ObjectIteratorPool.hpp @@ -5,22 +5,21 @@ #include #include -#include -#include -#include +#include +#include namespace db0::object_model { class ObjectIterator; - using PyObjectIterator = db0::python::PySharedWrapper; class ObjectIteratorPool { public: - using ObjectSharedExtPtr = db0::python::shared_py_object; + using ObjectWeakPtr = std::weak_ptr; - void add(ObjectSharedExtPtr object); + void add(const std::shared_ptr &object); + void remove(ObjectIterator *object_ptr); std::size_t detach(); std::size_t detach(std::uint64_t generation); std::size_t cleanup(); @@ -30,11 +29,11 @@ namespace db0::object_model bool isClosed() const; private: - std::vector m_iterators; + std::unordered_map m_iterators; std::uint64_t m_detach_generation = 0; bool m_closed = false; - static ObjectIterator *getIterator(ObjectSharedExtPtr const &object); + static std::shared_ptr getIterator(const ObjectWeakPtr &object); }; } diff --git a/tests/unit_tests/ObjectIteratorPoolTest.cpp b/tests/unit_tests/ObjectIteratorPoolTest.cpp index b7c30107..667eda3d 100644 --- a/tests/unit_tests/ObjectIteratorPoolTest.cpp +++ b/tests/unit_tests/ObjectIteratorPoolTest.cpp @@ -165,7 +165,7 @@ namespace tests DetachableUniqueAddressIterator *query_ptr = nullptr; auto py_iter = makePyIterator(fixture, query_ptr); - pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get())); + pool.add(py_iter->getSharedPtr()); ASSERT_EQ(pool.size(), 1u); ASSERT_EQ(pool.detach(), 1u); @@ -182,7 +182,7 @@ namespace tests DetachableUniqueAddressIterator *query_ptr = nullptr; auto py_iter = makePyIterator(fixture, query_ptr); - pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get())); + pool.add(py_iter->getSharedPtr()); ASSERT_EQ(pool.detach(1), 1u); ASSERT_EQ(query_ptr->m_detach_count, 1u); @@ -224,7 +224,7 @@ namespace tests DetachableUniqueAddressIterator *query_ptr = nullptr; auto py_iter = makePyIterator(fixture, query_ptr); - pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get())); + pool.add(py_iter->getSharedPtr()); py_iter->reset(); ASSERT_EQ(pool.cleanup(), 1u); @@ -232,6 +232,22 @@ namespace tests workspace.close(); } + TEST_F(ObjectIteratorPoolTest, testObjectIteratorPoolDropsDestroyedPythonIteratorsImmediately) + { + Workspace workspace("", {}, {}, {}, {}, pythonFixtureInitializer()); + auto fixture = workspace.getFixture(prefix_name); + auto &pool = fixture->get(); + DetachableUniqueAddressIterator *query_ptr = nullptr; + auto py_iter = makePyIterator(fixture, query_ptr); + + pool.add(py_iter->getSharedPtr()); + + ASSERT_EQ(pool.size(), 1u); + py_iter.reset(); + ASSERT_EQ(pool.size(), 0u); + workspace.close(); + } + TEST_F(ObjectIteratorPoolTest, testObjectIteratorPoolDetachCompactsExpiredNativeIterators) { Workspace workspace("", {}, {}, {}, {}, pythonFixtureInitializer()); @@ -242,8 +258,8 @@ namespace tests auto live_iter = makePyIterator(fixture, live_query_ptr); auto expired_iter = makePyIterator(fixture, expired_query_ptr); - pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(live_iter.get())); - pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(expired_iter.get())); + pool.add(live_iter->getSharedPtr()); + pool.add(expired_iter->getSharedPtr()); expired_iter->reset(); ASSERT_EQ(pool.detach(), 1u); @@ -261,7 +277,7 @@ namespace tests auto py_iter = makePyIterator(fixture, query_ptr); pool.close(); - pool.add(db0::object_model::ObjectIteratorPool::ObjectSharedExtPtr(py_iter.get())); + pool.add(py_iter->getSharedPtr()); ASSERT_TRUE(pool.isClosed()); ASSERT_EQ(pool.size(), 0u); @@ -270,6 +286,23 @@ namespace tests workspace.close(); } + TEST_F(ObjectIteratorPoolTest, testObjectIteratorPoolRegistrationDoesNotIncreasePythonRefcount) + { + Workspace workspace("", {}, {}, {}, {}, pythonFixtureInitializer()); + auto fixture = workspace.getFixture(prefix_name); + auto &pool = fixture->get(); + DetachableUniqueAddressIterator *query_ptr = nullptr; + auto py_iter = makePyIterator(fixture, query_ptr); + auto *py_object = reinterpret_cast(py_iter.get()); + auto refcount_before = Py_REFCNT(py_object); + + pool.add(py_iter->getSharedPtr()); + + ASSERT_EQ(Py_REFCNT(py_object), refcount_before); + ASSERT_EQ(pool.size(), 1u); + workspace.close(); + } + TEST_F(ObjectIteratorPoolTest, testObjectIteratorPoolRegistersForLiveFixturesButNotSnapshots) { {