Pillow support for cyclic garbage collection

[Lucas-C]

Hi!

I'm not really expert with CPython internal implementation, nor with Pillow internals, so I hope my question is not too silly 😊

I'm currently hunting for some unexpected memory usage in fpdf2 unit tests suite: py-pdf/fpdf2#641
My current best lead is related to Pillow.
I am using the latest Pillow version from source (I also tested some patches like the recent PR #7003):

pip install git+https://github.com/python-pillow/Pillow.git@main

I checked an old PyCon talk by Dave Malcolm on CPython memory usage,
and he mentions that the tp_traverse handler must be defined for C extensions that define container objects:

• the reference at 2:19 in his recorded talk on YouTube
• https://docs.python.org/3/c-api/gcsupport.html#supporting-cycle-detection

I checked Pillow C sources and currently none of the Python types defined provide an implementation for tp_traverse.
Hence, my question is basically: is that OK?

Given that none of the struct XXXObject defined by Pillow in src/*.c files make reference to other Python objects managed by the garbage collector, I think that it may be fine, but I wanted to ask the question there to be sure 😅

[radarhere]

currently none of the Python types defined provide an implementation for tp_traverse

Ah, such as

Pillow/src/_imaging.c

Line 3702 in 3166901

0, /*tp_traverse*/

Given that none of the struct XXXObject defined by Pillow in src/*.c files make reference to other Python objects managed by the garbage collector, I think that it may be fine

This is referring to https://docs.python.org/3/c-api/gcsupport.html

Types which do not store references to other objects, or which only store references to atomic types (such as numbers or strings), do not need to provide any explicit support for garbage collection.

I'm certainly not an expert in the C side of CPython, but my tentative theory is that this isn't necessary for us because the structure of our C objects might not interact with each other to ever create circular references. I wonder if @wiredfool can provide a better answer?

Setting aside your specific question, if you have a self-contained Pillow script with unexpected memory usage, feel free to post it.

[Lucas-C]

I'm certainly not an expert in the C side of CPython, but my tentative theory is that this isn't necessary for us because the structure of our C objects might not interact with each other to ever create circular references.

Yes, that makes sense.
I was a bit worried about it initially (and I kinda hoped it would explain my "leaking" memory issues),
but while redacting my initial message I ended up thinking there was not much to worry about.

Setting aside your specific question, if you have a self-contained Pillow script with unexpected memory usage, feel free to post it.

Thanks!
I'm working on it in PR py-pdf/fpdf2#721, but I have trouble getting accurate metrics for the actual memory usage... I'm trying to figure if I should track the process heap / RSS / PyMalloc tracemalloc / pympler statistics.
I'll open an issue once I have a reproducible test case.

[wiredfool]

As far as I recall, we don't have any possible cycles in our C python objects.

There are a couple of bits that might make it a little difficult to reason about the memory usage --

Pillow uses a cached memory allocator so that we're not malloc/freeing for every image. This can lead to memory growth proportional to the max size of all concurrent images in the process. This is in Storage.c: https://github.com/python-pillow/Pillow/blob/main/src/libImaging/Storage.c#L320 There are a couple of functions exposed on the internal im object, get_stats https://github.com/python-pillow/Pillow/blob/main/src/_imaging.c#L4159 . These might help you quantify how much memory is in the cached allocator.

Valgrind's memory leaks tool doesn't seem to be able to track python memory allocations very well, so there tend to be a lot of leaks from core python. I don't generally track leaks because of that. We do use valgrind to find memory leaks that increase via iterations, because tracking allocation locations and peak memory does get us a long way towards finding the leaks in C code.

I'd be surprised if there are many significant leaks in Pillow (at least in code that's been around for a year or more). There's certainly nothing that will commonly leak anything like O(ImageSize). I wouldn't be surprised if there are leaks of the O(Bytes) level.

-edit-
my quick valgrind massif tool (that generally runs the creatively named b.py):

#!/bin/bash

PROG=${2:-b.py}
valgrind --tool=massif python $PROG $1 &
PID=$!
wait $PID
ms_print massif.out.$PID | head -n 42

[radarhere]

Thanks

[Lucas-C]

Thank you @wiredfool!
This is very valuable input for my research 😊
I was already planning to test the steps from this documentation page: https://pillow.readthedocs.io/en/stable/reference/c_extension_debugging.html
Your comment gives me even more insight on things work, that's perfect!

[wiredfool]

That doc is what I wrote after the second or third time figuring it all out again because I hadn't written it down. Hope it helps you find what you're looking for.