Modernize C++ code - #144
Conversation
Six leaks, all in hand-written refcount glue and all independent of each other. Verified by measuring sys.getallocatedblocks() and RSS before and after; the pre-fix numbers are given below. - Path.coords() leaked one float per coordinate. PyList_Append takes its own reference, so the one from PyFloat_FromDouble was never released. Measured: 160,000 blocks over 20k calls on an 8-coordinate path. - draw_dealloc never deleted self->transform, so every Draw that called settransform() leaked an agg::trans_affine. Measured: 12.8 MB over 200k Draw+settransform pairs. settransform() itself already freed the old one on replacement; only teardown was missing. - Three constructor error paths marked "FIXME: cleanup"/"FIXME: release resources" leaked the half-built object: a rejected Symbol() descriptor, a Font() whose file will not load, and a Draw() given short image data. Measured: 20,000 blocks over 20k failed Symbol() calls. - Draw(image) assigned self->image and only increfed it after the tobytes round-trip, holding a borrowed reference in between. - The dict backing the color-resolution helper was deliberately leaked to keep a borrowed reference alive. It now takes a strong reference and releases the dict, and PyRun_String's result is checked and released. draw_new now nulls every pointer field immediately after PyObject_NEW, which is what makes Py_DECREF safe on those error paths: PyObject_NEW does not zero the allocation, and draw_dealloc deletes all six pointers. That is why the bad-mode path previously used PyObject_DEL. Also drops a stale comment in core.py claiming a Python-side pen/brush swap segfaults. It does not reproduce; it was a symptom of the NULL ob_type bug fixed in the previous commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pen, Brush, Font, Path and Draw were module-level factory functions returning opaque static-typed objects -- the standard CPython idiom when this was written in 2003, but it predates the type/class unification. They could not be subclassed, and Pen.color and Pen.width were unreachable from Python entirely. They are now heap types built with PyType_FromSpec, exposed on the module as real classes, and subclassable. Each PyType_Spec sits next to that type's own methods, which removes the reason tp_methods had to be assigned imperatively in aggdraw_init: the method arrays are defined far below the old static initializers and C++ has no tentative definitions, so they could not be named from there. Symbol stays a factory function. It returns a Path -- Symbol and Path have always been the same C type -- so it cannot become a type of its own without changing what it returns. Two invariants that heap types require: - The deallocators go through tp_free and then Py_DECREF(Py_TYPE(self)) rather than calling PyObject_DEL. path_new's direct call to path_dealloc becomes Py_DECREF for the same reason. - The _Check macros use PyObject_TypeCheck instead of an identity test. Py_TPFLAGS_BASETYPE is set, and an identity test would make the dispatcher in draw_adaptor::draw silently ignore a subclass of Pen or Brush. The draw_getattro and font_getattro strcmp-style chains are replaced by tp_getset descriptors, and Pen/Brush gain color and width. Draw.mode still reports the adaptor's label, so a BGRA surface still reads back as "RGBA"; that behaviour is unchanged and now carries a comment. core.py gains matching read-only properties, since doc/source/index.rst renders the API from a single automodule and anything absent from core.py is invisible in the docs. Verified the generated HTML, not just that sphinx-build -W passes. Checked with and without FreeType, since font_getset is guarded and Windows wheels ship without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tp_new is handed a keyword dict whether or not it parses one, so the
conversion from METH_VARARGS quietly started accepting and ignoring
keywords: Draw("RGB", (4,4), color="red") built a white surface instead
of raising. Restore the TypeError.
Pen, Brush and Font are unaffected -- they parse keywords deliberately
and always have.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This PR is probably stuck until I/we can decide what we want to do with |
|
Wow, this is a big undertaking!! I can review this more carefully when it's further along, my only 2 cents for now is that I'm slightly wary of the number of unintentional rendering changes and unexpected bugs a rewrite like this might introduce given the state of the test suite (having flashbacks to the aborted agg 2.4 update many moons ago). For peace of mind, my current thought would be to do something like the old Acid 2 test for browsers way back in the day, where you draw a test image using a bunch of different rendering functions that can serve as a reference. The test script would save the rendered image and then hash its contents. Once you have a hash for the current pre-rewrite version, you can add the test script to the test suite and have it compare the calculated hash against the known hash, so we'd always know when a change to the code actually changes the way things are drawn/rendered. For deliberate changes in rendering we'd just update the reference hash in the tests, but the idea would be to catch accidental changes in rendering more than anything else! |
|
Yes, absolutely. That's a great idea. And definitely, this should really be merged when things have been verified. I'm less concerned with these changes here as I'm not touching the actual code doing the drawing, just the wrapper that is aggdraw. With agg 2.4 changes, yeah that was...a mistake. I think I was so surprised by how many backwards incompatible changes the library had in a version bump where some of my coastline drawing use case just had completely different results. And with that, how we did simple tests with small images and they looked reasonable but then when I started doing larger images with lots of paths is when it was more obvious that things were way off. Let me see if I can set Claude on getting some skeleton together and I'll put it in another PR, eventually merge that, then rebase this PR. |
2 participants
This PR comes from a long time wish of mine to rewrite the C++ code (_aggdraw.cxx) to remove all Python 2 conditionals and API usage. I basically asked Claude to do the python 2 removal then I asked "this module uses factory functions to create objects, is this still done in modern times?" and Claude said no that's a 2003 common practice, not 2026.
The main things are marked in the Changelog. This PR should fix multiple memory leaks according to Claude. This PR should also be the first major step toward a Cython wrapper or maybe something more maintainable than this Python C API extension with a python wrapper. Not all class creation has been migrated to the newer APIs, but I'll have to do more reviewing to be sure what is the right path forward.
TOOD: I still need to review this completely which is why I'm marking it as a draft.