From 8f2407b518f8eba2f48cec06bdb76a09b9eb078d Mon Sep 17 00:00:00 2001 From: John McCardle Date: Sun, 28 Dec 2025 00:30:31 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20EntityCollection=20iterator=20O(n=C2=B2)?= =?UTF-8?q?=20=E2=86=92=20O(n)=20with=20100=C3=97=20speedup=20(closes=20#1?= =?UTF-8?q?59)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: EntityCollection iterator used index-based access on std::list, causing O(n) traversal per element access (O(n²) total for iteration). Root cause: Each call to next() started from begin() and advanced index steps: std::advance(l_begin, self->index-1); // O(index) for linked list! Solution: - Store actual std::list iterators (current, end) instead of index - Increment iterator directly in next() - O(1) operation - Cache Entity and Iterator type lookups to avoid repeated dict lookups Benchmark results (2,000 entities): - Before: 13.577ms via EntityCollection - After: 0.131ms via EntityCollection - Speedup: 103× 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .gitignore | 1 + src/UIGrid.cpp | 64 +++++++++++++++++++++++++++----------------------- src/UIGrid.h | 5 ++-- 3 files changed, 39 insertions(+), 31 deletions(-) diff --git a/.gitignore b/.gitignore index 4bd78c1..2a59fe4 100644 --- a/.gitignore +++ b/.gitignore @@ -26,6 +26,7 @@ mcrogueface.github.io scripts/ tcod_reference .archive +.mcp.json # Keep important documentation and tests !CLAUDE.md diff --git a/src/UIGrid.cpp b/src/UIGrid.cpp index 7edbc68..e44454a 100644 --- a/src/UIGrid.cpp +++ b/src/UIGrid.cpp @@ -2113,38 +2113,37 @@ int UIEntityCollectionIter::init(PyUIEntityCollectionIterObject* self, PyObject* PyObject* UIEntityCollectionIter::next(PyUIEntityCollectionIterObject* self) { + // Check for collection modification during iteration if (self->data->size() != self->start_size) { PyErr_SetString(PyExc_RuntimeError, "collection changed size during iteration"); return NULL; } - if (self->index > self->start_size - 1) + // Check if we've reached the end + if (self->current == self->end) { PyErr_SetNone(PyExc_StopIteration); return NULL; } - self->index++; - auto vec = self->data.get(); - if (!vec) - { - PyErr_SetString(PyExc_RuntimeError, "the collection store returned a null pointer"); - return NULL; - } - // Advance list iterator since Entities are stored in a list, not a vector - auto l_begin = (*vec).begin(); - std::advance(l_begin, self->index-1); - auto target = *l_begin; - + + // Get current element and advance iterator - O(1) operation + auto target = *self->current; + ++self->current; + // Return the stored Python object if it exists (preserves derived types) if (target->self != nullptr) { Py_INCREF(target->self); return target->self; } - + // Otherwise create and return a new Python Entity object - auto type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Entity"); - auto o = (PyUIEntityObject*)type->tp_alloc(type, 0); + // Cache the Entity type to avoid repeated dictionary lookups (#159) + static PyTypeObject* cached_entity_type = nullptr; + if (!cached_entity_type) { + cached_entity_type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "Entity"); + } + auto o = (PyUIEntityObject*)cached_entity_type->tp_alloc(cached_entity_type, 0); auto p = std::static_pointer_cast(target); o->data = p; return (PyObject*)o; @@ -2153,9 +2152,12 @@ PyObject* UIEntityCollectionIter::next(PyUIEntityCollectionIterObject* self) PyObject* UIEntityCollectionIter::repr(PyUIEntityCollectionIterObject* self) { std::ostringstream ss; - if (!self->data) ss << ""; + if (!self->data) ss << ""; else { - ss << "data->size() << " child objects, @ index " << self->index << ")>"; + // Calculate current position by distance from end + auto remaining = std::distance(self->current, self->end); + auto total = self->data->size(); + ss << ""; } std::string repr_str = ss.str(); return PyUnicode_DecodeUTF8(repr_str.c_str(), repr_str.size(), "replace"); @@ -3060,26 +3062,30 @@ int UIEntityCollection::init(PyUIEntityCollectionObject* self, PyObject* args, P PyObject* UIEntityCollection::iter(PyUIEntityCollectionObject* self) { - // Get the iterator type from the module to ensure we have the registered version - PyTypeObject* iterType = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "UIEntityCollectionIter"); - if (!iterType) { - PyErr_SetString(PyExc_RuntimeError, "Could not find UIEntityCollectionIter type in module"); - return NULL; + // Cache the iterator type to avoid repeated dictionary lookups (#159) + static PyTypeObject* cached_iter_type = nullptr; + if (!cached_iter_type) { + cached_iter_type = (PyTypeObject*)PyObject_GetAttrString(McRFPy_API::mcrf_module, "UIEntityCollectionIter"); + if (!cached_iter_type) { + PyErr_SetString(PyExc_RuntimeError, "Could not find UIEntityCollectionIter type in module"); + return NULL; + } + // Keep a reference to prevent it from being garbage collected + Py_INCREF(cached_iter_type); } - + // Allocate new iterator instance - PyUIEntityCollectionIterObject* iterObj = (PyUIEntityCollectionIterObject*)iterType->tp_alloc(iterType, 0); - + PyUIEntityCollectionIterObject* iterObj = (PyUIEntityCollectionIterObject*)cached_iter_type->tp_alloc(cached_iter_type, 0); + if (iterObj == NULL) { - Py_DECREF(iterType); return NULL; // Failed to allocate memory for the iterator object } iterObj->data = self->data; - iterObj->index = 0; + iterObj->current = self->data->begin(); // Start at beginning + iterObj->end = self->data->end(); // Cache end iterator iterObj->start_size = self->data->size(); - Py_DECREF(iterType); return (PyObject*)iterObj; } diff --git a/src/UIGrid.h b/src/UIGrid.h index 6adf3c1..8c10491 100644 --- a/src/UIGrid.h +++ b/src/UIGrid.h @@ -222,8 +222,9 @@ public: typedef struct { PyObject_HEAD std::shared_ptr>> data; - int index; - int start_size; + std::list>::iterator current; // Actual list iterator - O(1) increment + std::list>::iterator end; // End iterator for bounds check + int start_size; // For detecting modification during iteration } PyUIEntityCollectionIterObject; class UIEntityCollectionIter {