Skip to content

Commit 9496dd2

Browse files
jcupittjcupitt
authored
WIP -- work around php-ffi memory leaks (#171)
* work around a php-ffi memleak Due to a bug (I think?) in php-ffi we leaked c. 100 bytes for every call to getPspec(). This could really add up in long running programs. Thanks @levmv! See #167 * credit levmv in changelog * work around a php-ffi leak in arrayType php-ffi seems to leak if you use arrayType ... this commit switches to string arguments instead * tests pass * better test for disabled ffi see #172
1 parent 9a0ca1b commit 9496dd2

File tree

6 files changed

+58
-36
lines changed

6 files changed

+58
-36
lines changed

‎CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,13 @@
22

33
All notable changes to `:vips` will be documented in this file.
44

5+
## master
6+
7+
- refactor callBase() for maintainability
8+
- work around a php-ffi memory leak in getPspec() [levmv]
9+
- work around a php-ffi memory leak in arrayType()
10+
- better test for disabled ffi
11+
512
## 2.1.0 - 2022-10-11
613

714
- allow "-" as a name component separator [andrews05]

‎src/FFI.php‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,13 @@ private static function init(): void
201201
return;
202202
}
203203

204+
// try experimentally binding a bit of stdio ... if this fails, FFI
205+
// has probably not been installed or enabled, and php will throw a
206+
// useful error message
207+
$stdio = \FFI::cdef(<<<EOS
208+
int printf(const char *, ...);
209+
EOS);
210+
204211
$vips_libname = self::libraryName("libvips", 42);
205212
if (PHP_OS_FAMILY === "Windows") {
206213
$glib_libname = self::libraryName("libglib-2.0", 0);
@@ -252,7 +259,7 @@ private static function init(): void
252259
if ($vips === null) {
253260
array_shift($libraryPaths);
254261

255-
$msg = "Unable to find library '$vips_libname'";
262+
$msg = "Unable to open library '$vips_libname'";
256263
if (!empty($libraryPaths)) {
257264
$msg .= " in any of ['" . implode("', '", $libraryPaths) . "']";
258265
}
@@ -541,8 +548,8 @@ private static function init(): void
541548
unsigned int offset;
542549
} VipsArgumentClass;
543550
544-
int vips_object_get_argument (VipsObject* object,
545-
const char *name, GParamSpec** pspec,
551+
int vips_object_get_argument (VipsObject* object, const char *name,
552+
GParamSpec** pspec,
546553
VipsArgumentClass** argument_class,
547554
VipsArgumentInstance** argument_instance);
548555

‎src/GValue.php‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -147,8 +147,7 @@ public function set($value): void
147147
$value = [$value];
148148
}
149149
$n = count($value);
150-
$ctype = \FFI::arrayType(\FFI::type("int"), [$n]);
151-
$array = \FFI::new($ctype);
150+
$array = \FFI::new("int[$n]");
152151
for ($i = 0; $i < $n; $i++) {
153152
$array[$i] = $value[$i];
154153
}
@@ -161,8 +160,7 @@ public function set($value): void
161160
$value = [$value];
162161
}
163162
$n = count($value);
164-
$ctype = \FFI::arrayType(\FFI::type("double"), [$n]);
165-
$array = \FFI::new($ctype);
163+
$array = \FFI::new("double[$n]");
166164
for ($i = 0; $i < $n; $i++) {
167165
$array[$i] = $value[$i];
168166
}
@@ -189,8 +187,7 @@ public function set($value): void
189187
# we need to set the blob to a copy of the data that vips_lib
190188
# can own and free
191189
$n = strlen($value);
192-
$ctype = \FFI::arrayType(\FFI::type("char"), [$n]);
193-
$memory = \FFI::new($ctype, false, true);
190+
$memory = \FFI::new("char[$n]", false, true);
194191
for ($i = 0; $i < $n; $i++) {
195192
$memory[$i] = $value[$i];
196193
}

‎src/Image.php‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -797,8 +797,7 @@ public static function newFromArray(
797797
$width = count($array[0]);
798798

799799
$n = $width * $height;
800-
$ctype = \FFI::arrayType(\FFI::type("double"), [$n]);
801-
$a = \FFI::new($ctype, true, true);
800+
$a = \FFI::new("double[$n]", true, true);
802801
for ($y = 0; $y < $height; $y++) {
803802
for ($x = 0; $x < $width; $x++) {
804803
$a[$x + $y * $width] = $array[$y][$x];
@@ -975,8 +974,7 @@ public function writeToBuffer(string $suffix, array $options = []): string
975974
*/
976975
public function writeToMemory(): string
977976
{
978-
$ctype = \FFI::arrayType(\FFI::type("size_t"), [1]);
979-
$p_size = \FFI::new($ctype);
977+
$p_size = \FFI::new("size_t[1]");
980978

981979
$pointer = FFI::vips()->
982980
vips_image_write_to_memory($this->pointer, $p_size);
@@ -1017,8 +1015,7 @@ public function writeToMemory(): string
10171015
*/
10181016
public function writeToArray(): array
10191017
{
1020-
$ctype = \FFI::arrayType(\FFI::type("size_t"), [1]);
1021-
$p_size = \FFI::new($ctype);
1018+
$p_size = \FFI::new("size_t[1]");
10221019

10231020
$pointer = FFI::vips()->
10241021
vips_image_write_to_memory($this->pointer, $p_size);
@@ -1027,10 +1024,9 @@ public function writeToArray(): array
10271024
}
10281025

10291026
// wrap pointer up as a C array of the right type
1030-
$n = $this->width * $this->height * $this->bands;
10311027
$type_name = FFI::ftypes($this->format);
1032-
$ctype = \FFI::arrayType(\FFI::type($type_name), [$n]);
1033-
$array = \FFI::cast($ctype, $pointer);
1028+
$n = $this->width * $this->height * $this->bands;
1029+
$array = \FFI::cast("{$type_name}[$n]", $pointer);
10341030

10351031
// copy to PHP memory as a flat array
10361032
$result = [];

‎src/VipsObject.php‎

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -88,26 +88,39 @@ public function getDescription(): string
8888
// get the pspec for a property
8989
// NULL for no such name
9090
// very slow! avoid if possible
91-
// FIXME add a cache for this thing
91+
// FIXME add a cache for this thing, see code in pyvips
9292
public function getPspec(string $name): ?\FFI\CData
9393
{
9494
$name = str_replace("-", "_", $name);
95-
$pspec = FFI::gobject()->new("GParamSpec*[1]");
96-
$argument_class = FFI::vips()->new("VipsArgumentClass*[1]");
97-
$argument_instance = FFI::vips()->new("VipsArgumentInstance*[1]");
95+
$pspec_array = FFI::gobject()->new("GParamSpec*[1]");
96+
$argument_class_array = FFI::vips()->new("VipsArgumentClass*[1]");
97+
$argument_instance_array = FFI::vips()->new("VipsArgumentInstance*[1]");
9898
$result = FFI::vips()->vips_object_get_argument(
9999
$this->pointer,
100100
$name,
101-
$pspec,
102-
$argument_class,
103-
$argument_instance
101+
$pspec_array,
102+
$argument_class_array,
103+
$argument_instance_array
104104
);
105105

106106
if ($result != 0) {
107-
return null;
107+
$pspec = null;
108108
} else {
109-
return $pspec[0];
109+
/* php-ffi seems to leak if we do the obvious $pspec_array[0] to
110+
* get the return result ... instead, we must make a new pointer
111+
* object and copy the value ourselves
112+
*
113+
* the returns values from vips_object_get_argument() are static,
114+
* so this is safe
115+
*/
116+
$pspec = FFI::gobject()->new("GParamSpec*");
117+
$to_pointer = \FFI::addr($pspec);
118+
$from_pointer = \FFI::addr($pspec_array);
119+
$size = \FFI::sizeof($from_pointer);
120+
\FFI::memcpy($to_pointer, $from_pointer, $size);
110121
}
122+
123+
return $pspec;
111124
}
112125

113126
// get the type of a property from a VipsObject

‎src/VipsOperation.php‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,9 @@ public static function newFromName($name): VipsOperation
8181
if ($pointer == null) {
8282
throw new Exception();
8383
}
84+
$operation = new VipsOperation($pointer);
8485

85-
return new VipsOperation($pointer);
86+
return $operation;
8687
}
8788

8889
public function setMatch($name, $match_image, $value): void
@@ -223,6 +224,7 @@ public static function callBase(
223224

224225
$operation = self::newFromName($operation_name);
225226
$operation->introspect = self::introspect($operation_name);
227+
$introspect = $operation->introspect;
226228

227229
/* the first image argument is the thing we expand constants to
228230
* match ... look inside tables for images, since we may be passing
@@ -244,11 +246,11 @@ public static function callBase(
244246
* args, using instance if required, and only check the nargs after
245247
* this pass.
246248
*/
247-
$n_required = count($operation->introspect->required_input);
249+
$n_required = count($introspect->required_input);
248250
$n_supplied = count($arguments);
249251
$n_used = 0;
250-
foreach ($operation->introspect->required_input as $name) {
251-
if ($name == $operation->introspect->member_this) {
252+
foreach ($introspect->required_input as $name) {
253+
if ($name == $introspect->member_this) {
252254
if (!$instance) {
253255
$operation->unrefOutputs();
254256
throw new Exception("instance argument not supplied");
@@ -291,8 +293,8 @@ public static function callBase(
291293
*/
292294
foreach ($options as $name => $value) {
293295
$name = str_replace("-", "_", $name);
294-
if (!in_array($name, $operation->introspect->optional_input) &&
295-
!in_array($name, $operation->introspect->optional_output)) {
296+
if (!in_array($name, $introspect->optional_input) &&
297+
!in_array($name, $introspect->optional_output)) {
296298
$operation->unrefOutputs();
297299
throw new Exception("optional argument '$name' does not exist");
298300
}
@@ -309,22 +311,22 @@ public static function callBase(
309311
throw new Exception();
310312
}
311313
$operation = new VipsOperation($pointer);
312-
$operation->introspect = self::introspect($operation_name);
314+
$operation->introspect = $introspect;
313315

314316
# TODO .. need to attach input refs to output, see _find_inside in
315317
# pyvips
316318

317319
/* Fetch required output args (and modified input args).
318320
*/
319321
$result = [];
320-
foreach ($operation->introspect->required_output as $name) {
322+
foreach ($introspect->required_output as $name) {
321323
$result[$name] = $operation->get($name);
322324
}
323325

324326
/* Any optional output args.
325327
*/
326328
$option_keys = array_keys($options);
327-
foreach ($operation->introspect->optional_output as $name) {
329+
foreach ($introspect->optional_output as $name) {
328330
$name = str_replace("-", "_", $name);
329331
if (in_array($name, $option_keys)) {
330332
$result[$name] = $operation->get($name);

0 commit comments

Comments
 (0)