Memory leak with cpSpaceGetBodyPositions?

[GuillaumeBroggi]

Hi,

First, thanks for this amazing work.

I have been toying with the cpSpaceGetBodyPositions used in the batched position benchmark as a way of improving performances by not looping over each body.

I noticed that memory usage accumulates over iterations which seems to indicate that the memory used by cpSpaceGetBodyPositions is never released. I tried few approaches to force the garbage collection during the iteration, but this is my first contact with cffi and I had no success so far. Of course, I might be doing something totally wrong.

Here is a MRE:

• pymunk 6.5.1
• python 3.10.12
• ubuntu 23.04

import pymunk


s = None

def setup(num_bodies=10):
    global s
    s = pymunk.Space()

    for x in range(num_bodies):
        b = pymunk.Body()
        b.position = x / 5, x / 2
        s.add(b)
    s.step(1)


def batched():
    arr = pymunk.ffi.new("cpVectArr *", {"num": 0})
    arr.num = 0
    arr.max = 0
    pymunk.cp.cpSpaceGetBodyPositions(s._space, arr)

# Use a large number of bodies to notice the effect
setup(num_bodies=1000)

# Be careful, it's likely to run out of memory with a large number of iteration
iter = 500000
for _ in range(iter):
    batched()

This results in a python process using 8.6 GB of memory on my machine.

[viblo]

Thanks for the report. I will have to investigate a bit, seems like its not just the array because that should get free'd for each iteration. I also tried to call ffi.release on it, but that didnt help so have to check more carefully.

ps. If you later do something with the batched stuff please let me know. Would be great to hear some realworld experience of that (if it turns out to be very useful I will make it part of the Python Pymunk API directly, but first I wanted to see if someone actually used this feature)

[GuillaumeBroggi]

Thanks for the feedback.

I will definetelly use the batched api if I can solve the memory issue because I need to retrieve positions quite often. Otherwise, I will loop over the bodies. In any case, I'll let you know when the stuff is ready.

[viblo]

Ok, now I checked more carefully and its the internal array (arr.arr) that leaks, and you cannot free it without patching the c code Pymunk uses to make the array.

You can work around this by re-using the array each time. Just set its num to 0 and it will behave as an empty array. In a real-world scenario this would also be preferable, since then the array doesnt need to be recreated each loop iteration.

arr = pymunk.ffi.new("cpVectArr *", {"num": 0})
arr.max = 0
def batched():
    arr.num = 0
    pymunk.cp.cpSpaceGetBodyPositions(s._space, arr)`

I also remember why I only made an experiment and didn't make it into a full API at the time. Just getting the positions of all bodies is a bit limited. Lets say you have bodies that are not identical in the simulation, maybe they are circles and squares, and you want to draw them. Then it doesn't help to get an array of all positions unless you can map it to circle or square body.

When I think about this issue again what Im leaning towards is to have 3 functions: one to get positions of all bodies, one to get angle of all bodies and finally one to get body ids. All 3 methods would return the data in the same order, so that as long as you know how to handle different ids you could map it with the body_id array.

I think body position and body angle is the most important data. Other things doesnt normally change every step so can be fetched with the normal non-batched functions if needed.

Do you have some suggestions or comments? Does it sound reasonable, or is there another way?

[GuillaumeBroggi]

Thanks for the workaround!

That seems reasonable. In my view positions, angles, and the existence of contact as well as the interpenetration distance are the more important metrics.

Regarding body ids, so far I assumed that pymunk and chipmunk preserve ordering, although I did not run extensive validation. Thus, I store this information at body creation and I use the index of the returned data to map them to the bodies.

[viblo]

There is no guarantee, but I think as long as no bodies are removed the order will be the same. If bodies are removed Im not sure if the order will be kept.. Anyway, I think I dont want to guarantee the order even if it works that way now.

Then I have two more questions:

1. I wonder about the contacts. Do you imagine there will be enough collisions that a batch api is useful? Maybe you could expand a bit about what simulation you are planning to do? The reason I ask is that its not straight forward how to do a batched api for collisions, the Abiter class is a bit complex with the contact point set inside..
2. What will you do with the batched data? In the batched example I had a memoryview for easy acccess but is this the best?

[GuillaumeBroggi]

1. I am running rigid body simulations with ~ 1k to 10k bodies. It's not so much the physical behavior of my simulation that interests me as the absence of overlap. Thus, at each iteration, I would query the abiters with each_arbiter, looping over my bodies and extracting the contact distance. It works fine like that, but I guess that batching this operation would improve the performances.

2. Once I am happy with the overlapping, I extract the body positions for further processing outside of pymunk. Again, it works fine by looping over the bodies, but batching the operation makes sense.

Does it make sense to you?

[viblo]

I have fixed the memory leak issue and cleaned the functions a bit so there's a new and free function to use. And I updated batched benchmark code to not allocate a new array for each iteration, so its more obvious whats the best way to do things.

I also modified the function to return both positions and angles in two separate arrays. However, Im still not sure about how to best wrap this up into a more proper batched pymunk api, that can also support more cases (like contacts or other properties on the body) in a nice way.

Thanks for the followup, its great with some real feedback on whats useful and why 👍 . But how to best make the api is a different question, have to think a bit more about that.

[GuillaumeBroggi]

Thanks for addressing the issue!

I updated pymunk and tried to run the benchmark. It works fine with 1 body, but not with more. I am not sure why, I will try to investigate more.

pymunk.version 6.5.2
Testing bodies:1 batched
[0.46237451500019233, 0.4628837060004116, 0.4762237999998433, 0.48090006500024174, 0.48601555799996277]
Testing bodies:5 batched
realloc(): invalid next size
[1] 28201 IOT instruction (core dumped)

[viblo]

Oh! For me the benchmark ran fine without issues. But now I re-read the code and noticed some things in the c-code that might create issues. Maybe its dependent on OS and python version (or its just random) if it works anyway or breaks. Right now the batching is not really part of the public API of Pymunk, its more of an experiment, so there's no test that runs on built etc.

Regardless I pushed an update that might fix it. If you have the possibility it would be great if you could try it. There are pre-built wheels here: https://github.com/viblo/pymunk/actions/runs/6631724385 (you probably need to first uninstall pymunk, then install the specific wheel matching your environment directly)

[GuillaumeBroggi]

Yes, I can confirm the last wheels run on my machine (Ubuntu 23.04).

[viblo]

I got inspired and coded together an proper pymunk api for batch. Its now released in Pymunk 6.6.0.
Since Im still a bit unsure of it, I put it as a separate module, pymunk.batch and marked it as experimental meaning it might change in any Pymunk release. There's a example of its usage here: http://www.pymunk.org/en/latest/examples.html#colors-pyglet-batch-py and api docs here: http://www.pymunk.org/en/latest/pymunk.batch.html

If you try it out and have some feedback, dont hesitate to share it :)