Turn rapidjson.Decoder into heap type - #227
Conversation
Sorry, something went wrong.
Each static type is uniq and global for all subinterpreters and can't access cached PyObjects. Decoder do use cached PyObjecs and must access cache stored in module rather than in static variables. Static type it can't access "proper module" due to its "per process singleton nature". The recomended way is to turn static types into heap types in order to use cached PyObjects.
|
This PR is a first step of work on the issue 226. Other types declared by the module must be converted to heap types as it's recommended in the cpython docs
The main purpose of having this small step as individual PR is getting early feedback on coding style issues or getting other comments which should be taken into account on further work. |
Sorry, something went wrong.
| "rapidjson.Decoder", /* name */ | ||
| sizeof(DecoderObject), /* basicsize */ | ||
| 0, /* itemsize */ | ||
| Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_IMMUTABLETYPE, /* flags */ |
There was a problem hiding this comment.
The type is flagged as Py_TPFLAGS_IMMUTABLETYPE to make this change more "noop-like". All static types are immutable. Decoder type remains immutable after it's transformation into heap-type. This flag can be dropped if it's ok to make Decoder to be closer to regular types defined in python.
Sorry, something went wrong.
| Py_INCREF(&Decoder_Type); | ||
| if (PyModule_AddObject(m, "Decoder", (PyObject*) &Decoder_Type) < 0) { | ||
| Py_DECREF(&Decoder_Type); | ||
| if (PyModule_AddObject(m, "Decoder", decoder_type.get()) < 0) |
There was a problem hiding this comment.
Using RAII here prolongates lifetime of a strong reference to decoder type to the end of the module exec function. It shouldn't affect type object lifetime since there are other strong references to it. But using RAII make the code smaller and less error prone.
Sorry, something went wrong.
espressolee
left a comment
There was a problem hiding this comment.
I checked exact head c390d85943e482807c5181d496a59e6a83699e59 and a clean cherry-pick onto current master at 00e9a146e958d42f84c81dcfdf39572280ecf01f. The heap-type/GC-traversal direction looks right, and the current-master suite passes. One reference-ownership issue blocks the success path as written.
[P1] Transfer the new reference after PyModule_AddObject() succeeds
At rapidjson.cpp:4106, PyModule_AddObject() steals the caller's reference to decoder_type on success. The PyStrongRef still considers itself the owner, so its destructor calls Py_DecRef() again when module_exec() returns. That decrements the heap type once more than the ownership contract permits and leaves the module's type reference under-counted.
The failure path should keep the RAII cleanup, while the success path should release it:
if (PyModule_AddObject(m, "Decoder", decoder_type.get()) < 0)
return -1;
decoder_type.release();This matches the documented contract: PyModule_AddObject() steals only on success, so the unique pointer remains responsible only when the call fails.
Controlled check on Python 3.12.13, same source and build:
- current PR code: sys.getrefcount(rapidjson.Decoder) == 8
- with the one-line release(): == 9
- full current-master suite with the fix: 929 passed, 17 skipped, 2 xfailed
The absolute count is implementation-sensitive; the controlled +1 is the ownership transfer that was missing. No other source change was needed.
Sorry, something went wrong.
Each static type is unique and global for all subinterpreters and can't access cached PyObjects. Decoder do use cached PyObjecs and must access cache stored in module rather than in static variables.
Static type can't access "proper module" due to its "per process singleton nature". The recommended way is to turn static types into heap types in order to use cached PyObjects.