libvips / php-vips

php binding for libvips
MIT License
615 stars 25 forks source link

Memory leak #167

Closed levmv closed 2 years ago

levmv commented 2 years ago

php 8.1 in cli mode, php-vips@dev-master and libvips 8.13.3

Simple script:

function vips() {
    $image = Image::newFromFile("../test.jpg");
}

Config::cacheSetMax(0);

for($i=0;$i<10000;$i++) {
    vips();
    if($i % 100 === 0) {
        echo (memory_get_usage()/1024)."\n";
        gc_collect_cycles();
    }
}

prints constantly increasing numbers. Is there memory leak somewhere or am I missing something?

jcupitt commented 2 years ago

Hi @levmv,

I think that's just php. I tried:

#!/usr/bin/env php
<?php

require __DIR__ . '/vendor/autoload.php';
use Jcupitt\Vips;

function vips($filename) {
    $image = Vips\Image::newFromFile($filename);
}

Vips\Config::cacheSetMax(0);

for($i=0;$i<10;$i++) {
    vips($argv[1]);
    gc_collect_cycles();
    echo $i . ", " . (memory_get_usage()/1024)."\n";
}   

echo "gc status:\n";
var_dump(gc_status());

Vips\FFI::shutDown();

And ran like this:

$ VIPS_LEAK=1 ./soak.php ~/pics/k2.jpg
0, 1044.0546875
1, 1044.2734375
2, 1044.4609375
3, 1044.6484375
4, 1044.8359375
5, 1045.0234375
6, 1045.2109375
7, 1045.3984375
8, 1045.5859375
9, 1045.7734375
gc status:
array(4) {
  ["runs"]=>
  int(10)
  ["collected"]=>
  int(0)
  ["threshold"]=>
  int(10001)
  ["roots"]=>
  int(0)
}
vips_threadset_free: peak of 0 threads
memory: high-water mark 0 bytes
$ 

The env var VIPS_LEAK enables the libvips leak checker, and you can see it found no leaked objects. You can add:

    Vips\VipsObject::printAll();

To get libvips to dump a table of all extant libvips objects, and it doesn't grow.

There's about 20kb lost per iteration, but I think that's just stuff inside php somewhere. I tried with valgrind:

$ valgrind --leak-check=full php soak.php ~/pics/k2.jpg
==24576== Memcheck, a memory error detector
==24576== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==24576== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==24576== Command: php soak.php /home/john/pics/k2.jpg
==24576== 
0, 1044.328125
1, 1044.546875
2, 1044.734375
3, 1044.921875
4, 1045.109375
5, 1045.296875
6, 1045.484375
7, 1045.671875
8, 1045.859375
9, 1046.046875
gc status:
array(4) {
  ["runs"]=>
  int(10)
  ["collected"]=>
  int(0)
  ["threshold"]=>
  int(10001)
  ["roots"]=>
  int(0)
}
==24576== 
==24576== HEAP SUMMARY:
==24576==     in use at exit: 1,048,812 bytes in 12,485 blocks
==24576==   total heap usage: 67,231 allocs, 54,746 frees, 21,578,247 bytes allocated
==24576== 
==24576== LEAK SUMMARY:
==24576==    definitely lost: 0 bytes in 0 blocks
==24576==    indirectly lost: 0 bytes in 0 blocks
==24576==      possibly lost: 0 bytes in 0 blocks
==24576==    still reachable: 389,611 bytes in 2,872 blocks
==24576==         suppressed: 525,185 bytes in 8,652 blocks
==24576== Reachable blocks (those to which a pointer was found) are not shown.
==24576== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==24576== 
==24576== For lists of detected and suppressed errors, rerun with: -s
==24576== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Again, no leaks.

To diagnose further, I suppose we'd need a debug build of php and some knowledge of the internals. Perhaps the php FFI module has some issues, or is caching more than it should?

jcupitt commented 2 years ago

I found a few php bugs around this, eg.:

https://github.com/php/php-src/issues/8433

So I suppose we're seeing leaks in the php-ffi module, or perhaps in php-vips itself, if it's using php-ffi incorrectly somewhere.

Someone brave needs to try to make a standalone example that leaks with just php-ffi.

levmv commented 2 years ago

I tried to pocking around, but my experience and knowledge is clearly not enough for such a job.

Probably a stupid observation: its' leaking only when use VipsObject::set. Tried to place return here VipsOperation.php#L266, unset operation, etc. Issue is still exist. But no issue before setMatch.

jcupitt commented 2 years ago

I made a small reproducer:

#!/usr/bin/env php
<?php

require __DIR__ . '/vendor/autoload.php';
use Jcupitt\Vips;

Vips\Config::cacheSetMax(0);

$name = "VipsForeignLoadJpegFile";
$operation = Vips\VipsOperation::newFromName($name);

echo "iteration, growth (bytes)\n";
$prev = 0;
for ($i = 0; $i < 10; $i++) {
    $filename_gtype = $operation->getType("filename");
    gc_collect_cycles();
    $now = memory_get_usage();
    $use = $now - $prev;
    $prev = $now;

    echo "$i, $use\n";
}

I see:

$ ./soak.php 
iteration, growth (bytes)
0, 711664
1, 96
2, 96
3, 96
4, 96
5, 416
6, 96
7, 96
8, 96
9, 96

So you can see it's leaking 96 bytes in every getType() call.

The leak itself is in getPspec():

https://github.com/libvips/php-vips/blob/master/src/VipsObject.php#L88-L111

It's this line:

            return $pspec[0];

That make php-ffi copy the pspec structure for some reason I don't quite understand. I've not found a good solution yet, I'll keep poking at it.

levmv commented 2 years ago

O, so great! Thanks a lot!

jcupitt commented 2 years ago

I think I fixed it and made a PR. Thank you for reporting this @levmv! I credited you in the changelog, I hope that's OK.

Would you be able to do some testing?

levmv commented 2 years ago

Simple tests shows everything ok. On one real, but simple project - same (ok). Now trying to test it on big project.

About credit: I do not deserve that, but thanks!

levmv commented 2 years ago

A rather complex long-running script is working fine now. Memory consumption is stable.

jcupitt commented 2 years ago

Ah that's great! Thank you for checking this.

I'll add a small cache for getPspec() as well in that PR -- it should speed things up by a few percent.

jcupitt commented 2 years ago

Let's close this and have a new issue for any further problems. Thanks again for reporting this and making a reproducer.

jcupitt commented 2 years ago

... could you add a note to that PR with your test results?