RAII-ify the interfaces between C++ and cpython #4

New Issue

john · 2024-03-09T18:14:10Z

john commented

2024-03-09 18:14:10 +00:00

notes say:
4 refactor: single responsibility and RAII changes to all systems that use PyObject*

Encapsulate all uses of Python
Banish all Py_INCREF and Py_DECREF from outside of the Python API code
shared pointers everywhere possible, wrap PyObject* or whatever it is in a C++ class everywhere else

notes say: 4 refactor: single responsibility and RAII changes to all systems that use PyObject* * Encapsulate all uses of Python * Banish all Py_INCREF and Py_DECREF from outside of the Python API code * shared pointers everywhere possible, wrap PyObject* or whatever it is in a C++ class everywhere else

john added the

Major Feature

Alpha Release Requirement

Refactoring & Cleanup

labels 2024-03-09 18:14:10 +00:00

john referenced this issue

2024-03-09 20:09:17 +00:00

Standardize Member Variable naming conventions #56

john referenced this issue

2024-03-09 20:54:16 +00:00

Standardize Font Handling #60

john referenced this issue

2024-03-09 21:09:27 +00:00

Grid - Entity - GridPointState updates via TCOD #64

john referenced this issue

2024-03-09 21:18:05 +00:00

Standardize Audio Handling #66

john commented

2024-03-10 19:40:32 +00:00

The sacred texts: https://docs.python.org/3/c-api/refcounting.html , https://en.cppreference.com/w/cpp/language/raii

Specifics on INCREF/DECREF

311 lines reference PyObject* (240 lines in UI.h/UI.cpp alone).
UI - every interface method uses PyObject.

GameEngine - Just a few references to PyObject, these all need to be cleaned out. Py_INCREF and Py_DECREF target - the timer's payload. What a mess!

Timer - directly stores PyObject, copies them, etc. No INCREF/DECREF (this bit me during 7DRL)

Scene - base class is INCREF, DECREF key_callable.

Plenty of usage in McRFPy_API.cpp - naturally, but cleaning out the action system (#2) would reduce that a little

key_callable - Keyboard callback - controlled from Scene, has signature (key:str, action:str)
click_callable - mouse callback - controlled from UI, has signature (x:int, y:int, button:str, action:str)
target - timer callback - controlled from Timer and GameEngine, has signature (elapsed:int)

the concept I'm going to work with to solve this:

base class: PyCallable
constructor requires a PyObject*
uses PyObject *Py_XNewRef(PyObject *o) to make strong references to its target
copy constructor makes a new strong reference
destructor calls DECREF
Derived classes: PyTimerCallable, PyKeyCallable, and PyClickCallable which implement .call(...) where the args match the use case's requirements
Borrow all PyObject references for args, repr, exceptions, etc, or add them as fields and use the same logic as in PyCallable base class

Other thoughts:

https://docs.python.org/3/c-api/structures.html#c.Py_TYPE - may obviate the requirement for the RET_PY_INSTANCE macro and PyObjectsEnum method on UIDrawables.

The sacred texts: https://docs.python.org/3/c-api/refcounting.html , https://en.cppreference.com/w/cpp/language/raii Specifics on INCREF/DECREF * https://docs.python.org/3/glossary.html#term-strong-reference * https://docs.python.org/3/glossary.html#term-borrowed-reference --- 311 lines reference `PyObject*` (240 lines in UI.h/UI.cpp alone). UI - every interface method uses PyObject. GameEngine - Just a few references to PyObject, these all need to be cleaned out. Py_INCREF and Py_DECREF `target` - the timer's payload. What a mess! Timer - directly stores PyObject, copies them, etc. No INCREF/DECREF (this bit me during 7DRL) Scene - base class is INCREF, DECREF key_callable. Plenty of usage in McRFPy_API.cpp - naturally, but cleaning out the action system (#2) would reduce that a little --- * `key_callable` - Keyboard callback - controlled from `Scene`, has signature `(key:str, action:str)` * `click_callable` - mouse callback - controlled from `UI`, has signature `(x:int, y:int, button:str, action:str)` * `target` - timer callback - controlled from `Timer` and `GameEngine`, has signature `(elapsed:int)` the concept I'm going to work with to solve this: * base class: `PyCallable` * constructor requires a PyObject* * uses `PyObject *Py_XNewRef(PyObject *o)` to make strong references to its target * copy constructor makes a new strong reference * destructor calls DECREF * Derived classes: `PyTimerCallable`, `PyKeyCallable`, and `PyClickCallable` which implement `.call(...)` where the args match the use case's requirements * Borrow all PyObject references for args, repr, exceptions, etc, or add them as fields and use the same logic as in `PyCallable` base class --- Other thoughts: * https://docs.python.org/3/c-api/structures.html#c.Py_TYPE - may obviate the requirement for the RET_PY_INSTANCE macro and `PyObjectsEnum` method on UIDrawables.

john started working 2024-03-11 22:32:31 +00:00

john stopped working 2024-03-11 23:18:57 +00:00

46 minutes 26 seconds

john started working 2024-03-13 00:44:33 +00:00

john stopped working 2024-03-13 02:27:43 +00:00

1 hour 43 minutes

john commented

2024-03-13 02:32:17 +00:00

I think I'm almost done, see 05d9f6a882

remaining stuff to do:

sanity check when I'm not tired
Clean up commented out blocks of old code
stress-test: a Python script for mcrogueface, to try and pass the same Python objects in to be called under click/key/timer with a lot of register / deregister calls
squash commit & merge

I think I'm almost done, see 05d9f6a882 remaining stuff to do: * sanity check when I'm not tired * Clean up commented out blocks of old code * stress-test: a Python script for mcrogueface, to try and pass the same Python objects in to be called under click/key/timer with a lot of register / deregister calls * squash commit & merge

john commented

2024-03-13 02:37:47 +00:00

I hate it when I'm right.

the unique_ptr on UIDrawable broke the default copy constructor. Very minor fix, but very prescient comment...

![image](/attachments/0a65f511-dfe2-4530-b092-281fd55730bf) I hate it when I'm right. the unique_ptr on UIDrawable broke the default copy constructor. Very minor fix, but very prescient comment...

image.png

121 KiB

john added this to the The Datatype Overhaul project 2024-03-13 13:13:16 +00:00

john added this to the All Datatypes Behaving milestone 2024-03-13 13:13:18 +00:00

john started working 2024-03-15 02:18:58 +00:00

john commented

2024-03-15 03:21:17 +00:00

See the weirdness in 0a8f67e391
haven't entirely isolated it, but I think I'm garbage collecting a function before it's done running...?

I don't think this is a problem with Py_XNewRef - the segfault isn't in the PyCallable or GameEngine code. Py_DECREF is being called by McRFPy_API during the mcrfpy.delTimer call, and the segfault occurs after the Python function ends.

It might also be A Bad Thing (TM) to modify the timers map from inside GameEngine::testTimers (which is iterating over the map while also modifying it). I may have to do some gymnastics like "mark as to be deleted" and run deletions after timer evaluations.

See the weirdness in 0a8f67e391 haven't entirely isolated it, but I think I'm garbage collecting a function before it's done running...? I don't think this is a problem with Py_XNewRef - the segfault isn't in the PyCallable or GameEngine code. Py_DECREF is being called by McRFPy_API during the `mcrfpy.delTimer` call, and the segfault occurs after the Python function ends. It might also be A Bad Thing (TM) to modify the timers map from inside `GameEngine::testTimers` (which is iterating over the map while also modifying it). I may have to do some gymnastics like "mark as to be deleted" and run deletions after timer evaluations.

john stopped working 2024-03-15 03:21:23 +00:00

1 hour 2 minutes

john commented

2024-03-15 16:34:04 +00:00

Looking at an example for removing a key / value from a map while iterating over it on TechieDelight:

#include <iostream>
#include <unordered_map>
#include <unordered_set>
#include <string>
 
template<typename K, typename V>
void remove_keys(std::unordered_map<K, V> &m, const std::unordered_set<K> &keys)
{
    auto it = m.cbegin();
    while (it != m.cend())
    {
        if (keys.find(it->first) != keys.cend())
        {
            // supported in C++11
            it = m.erase(it);
        }
        else {
            ++it;
        }
    }
}

check if this is possible with std::map or if I need unordered_map to accomplish it
add "deleted" field to PyTimerCallable (or PyCallable? Click/Key may need to destroy their C++ reference, but they're unique_ptrs, not in containers)
convert GameEngine::testTimers to use a while loop instead of a for loop
if timer->deleted after testing it, it = timers.erase(it) to proceed after delete-in-place

Another possible element of it: delete timers during the loop if they are Py_None / NULL - no new field necessary, and no-op callbacks are cleaned up each frame

Looking at an example for removing a key / value from a map while iterating over it [on TechieDelight](https://www.techiedelight.com/remove-entries-map-iterating-cpp/): ```c++ #include <iostream> #include <unordered_map> #include <unordered_set> #include <string> template<typename K, typename V> void remove_keys(std::unordered_map<K, V> &m, const std::unordered_set<K> &keys) { auto it = m.cbegin(); while (it != m.cend()) { if (keys.find(it->first) != keys.cend()) { // supported in C++11 it = m.erase(it); } else { ++it; } } } ``` * check if this is possible with `std::map` or if I need `unordered_map` to accomplish it * add "deleted" field to PyTimerCallable (or PyCallable? Click/Key may need to destroy their C++ reference, but they're unique_ptrs, not in containers) * convert `GameEngine::testTimers` to use a while loop instead of a for loop * if `timer->deleted` after testing it, `it = timers.erase(it)` to proceed after delete-in-place Another possible element of it: delete timers during the loop if they are `Py_None` / `NULL` - no new field necessary, and no-op callbacks are cleaned up each frame

john started working 2024-03-15 23:52:15 +00:00

john commented

2024-03-16 00:01:49 +00:00

see commit: [ raii_pyobjects c9d5251 ] In-place map modification worked

I will remove comments, squish, and merge after friday night hangout time.

see commit: [ raii_pyobjects c9d5251 ] In-place map modification worked I will remove comments, squish, and merge after friday night hangout time.

john stopped working 2024-03-16 00:01:54 +00:00

9 minutes 39 seconds

john started working 2024-03-16 02:01:08 +00:00

john referenced this issue from a commit

2024-03-16 02:22:15 +00:00

Squashed commit of the following: [raii_pyobjects]