From a3265b393b3c0e935e437f864c815e0485c46b6c Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 11 Nov 2022 16:19:18 +0000 Subject: [PATCH 1/5] 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 https://github.com/libvips/php-vips/issues/167 --- CHANGELOG.md | 5 +++++ src/FFI.php | 4 ++-- src/VipsObject.php | 31 ++++++++++++++++++++++--------- src/VipsOperation.php | 20 +++++++++++--------- 4 files changed, 40 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bfe47c7..8fc8e5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ All notable changes to `:vips` will be documented in this file. +## master + +- refactor callBase() for maintainability +- work around a php-ffi memory leak in getPspec() + ## 2.1.0 - 2022-10-11 - allow "-" as a name component separator [andrews05] diff --git a/src/FFI.php b/src/FFI.php index 6db84ff..8f29b87 100644 --- a/src/FFI.php +++ b/src/FFI.php @@ -541,8 +541,8 @@ private static function init(): void unsigned int offset; } VipsArgumentClass; -int vips_object_get_argument (VipsObject* object, - const char *name, GParamSpec** pspec, +int vips_object_get_argument (VipsObject* object, const char *name, + GParamSpec** pspec, VipsArgumentClass** argument_class, VipsArgumentInstance** argument_instance); diff --git a/src/VipsObject.php b/src/VipsObject.php index 05abdc6..6617aee 100644 --- a/src/VipsObject.php +++ b/src/VipsObject.php @@ -88,26 +88,39 @@ public function getDescription(): string // get the pspec for a property // NULL for no such name // very slow! avoid if possible - // FIXME add a cache for this thing + // FIXME add a cache for this thing, see code in pyvips public function getPspec(string $name): ?\FFI\CData { $name = str_replace("-", "_", $name); - $pspec = FFI::gobject()->new("GParamSpec*[1]"); - $argument_class = FFI::vips()->new("VipsArgumentClass*[1]"); - $argument_instance = FFI::vips()->new("VipsArgumentInstance*[1]"); + $pspec_array = FFI::gobject()->new("GParamSpec*[1]"); + $argument_class_array = FFI::vips()->new("VipsArgumentClass*[1]"); + $argument_instance_array = FFI::vips()->new("VipsArgumentInstance*[1]"); $result = FFI::vips()->vips_object_get_argument( $this->pointer, $name, - $pspec, - $argument_class, - $argument_instance + $pspec_array, + $argument_class_array, + $argument_instance_array ); if ($result != 0) { - return null; + $pspec = null; } else { - return $pspec[0]; + /* php-ffi seems to leak if we do the obvious $pspec_array[0] to + * get the return result ... instead, we must make a new pointer + * object and copy the value ourselves + * + * the returns values from vips_object_get_argument() are static, + * so this is safe + */ + $pspec = FFI::gobject()->new("GParamSpec*"); + $to_pointer = \FFI::addr($pspec); + $from_pointer = \FFI::addr($pspec_array); + $size = \FFI::sizeof($from_pointer); + \FFI::memcpy($to_pointer, $from_pointer, $size); } + + return $pspec; } // get the type of a property from a VipsObject diff --git a/src/VipsOperation.php b/src/VipsOperation.php index b1eb86e..cf212af 100644 --- a/src/VipsOperation.php +++ b/src/VipsOperation.php @@ -81,8 +81,9 @@ public static function newFromName($name): VipsOperation if ($pointer == null) { throw new Exception(); } + $operation = new VipsOperation($pointer); - return new VipsOperation($pointer); + return $operation; } public function setMatch($name, $match_image, $value): void @@ -223,6 +224,7 @@ public static function callBase( $operation = self::newFromName($operation_name); $operation->introspect = self::introspect($operation_name); + $introspect = $operation->introspect; /* the first image argument is the thing we expand constants to * match ... look inside tables for images, since we may be passing @@ -244,11 +246,11 @@ public static function callBase( * args, using instance if required, and only check the nargs after * this pass. */ - $n_required = count($operation->introspect->required_input); + $n_required = count($introspect->required_input); $n_supplied = count($arguments); $n_used = 0; - foreach ($operation->introspect->required_input as $name) { - if ($name == $operation->introspect->member_this) { + foreach ($introspect->required_input as $name) { + if ($name == $introspect->member_this) { if (!$instance) { $operation->unrefOutputs(); throw new Exception("instance argument not supplied"); @@ -291,8 +293,8 @@ public static function callBase( */ foreach ($options as $name => $value) { $name = str_replace("-", "_", $name); - if (!in_array($name, $operation->introspect->optional_input) && - !in_array($name, $operation->introspect->optional_output)) { + if (!in_array($name, $introspect->optional_input) && + !in_array($name, $introspect->optional_output)) { $operation->unrefOutputs(); throw new Exception("optional argument '$name' does not exist"); } @@ -309,7 +311,7 @@ public static function callBase( throw new Exception(); } $operation = new VipsOperation($pointer); - $operation->introspect = self::introspect($operation_name); + $operation->introspect = $introspect; # TODO .. need to attach input refs to output, see _find_inside in # pyvips @@ -317,14 +319,14 @@ public static function callBase( /* Fetch required output args (and modified input args). */ $result = []; - foreach ($operation->introspect->required_output as $name) { + foreach ($introspect->required_output as $name) { $result[$name] = $operation->get($name); } /* Any optional output args. */ $option_keys = array_keys($options); - foreach ($operation->introspect->optional_output as $name) { + foreach ($introspect->optional_output as $name) { $name = str_replace("-", "_", $name); if (in_array($name, $option_keys)) { $result[$name] = $operation->get($name); From 9f97e40c70e7537e1157ecdd26cf11f9caf5bab0 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 11 Nov 2022 16:21:32 +0000 Subject: [PATCH 2/5] credit levmv in changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fc8e5b..493954a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to `:vips` will be documented in this file. ## master - refactor callBase() for maintainability -- work around a php-ffi memory leak in getPspec() +- work around a php-ffi memory leak in getPspec() [levmv] ## 2.1.0 - 2022-10-11 From c9a66c696c175fda562a9b3165a13f902eb99db2 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Sat, 12 Nov 2022 11:05:13 +0000 Subject: [PATCH 3/5] work around a php-ffi leak in arrayType php-ffi seems to leak if you use arrayType ... this commit switches to string arguments instead --- CHANGELOG.md | 1 + src/GValue.php | 9 +++------ src/Image.php | 14 +++++--------- 3 files changed, 9 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 493954a..b0df0fd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to `:vips` will be documented in this file. - refactor callBase() for maintainability - work around a php-ffi memory leak in getPspec() [levmv] +- work around a php-ffi memory leak in arrayType() ## 2.1.0 - 2022-10-11 diff --git a/src/GValue.php b/src/GValue.php index 8560b90..6478352 100644 --- a/src/GValue.php +++ b/src/GValue.php @@ -147,8 +147,7 @@ public function set($value): void $value = [$value]; } $n = count($value); - $ctype = \FFI::arrayType(\FFI::type("int"), [$n]); - $array = \FFI::new($ctype); + $array = \FFI::new("int[$n]"); for ($i = 0; $i < $n; $i++) { $array[$i] = $value[$i]; } @@ -161,8 +160,7 @@ public function set($value): void $value = [$value]; } $n = count($value); - $ctype = \FFI::arrayType(\FFI::type("double"), [$n]); - $array = \FFI::new($ctype); + $array = \FFI::new("double[$n]"); for ($i = 0; $i < $n; $i++) { $array[$i] = $value[$i]; } @@ -189,8 +187,7 @@ public function set($value): void # we need to set the blob to a copy of the data that vips_lib # can own and free $n = strlen($value); - $ctype = \FFI::arrayType(\FFI::type("char"), [$n]); - $memory = \FFI::new($ctype, false, true); + $memory = \FFI::new("char[$n]", false, true); for ($i = 0; $i < $n; $i++) { $memory[$i] = $value[$i]; } diff --git a/src/Image.php b/src/Image.php index d599757..99c196c 100644 --- a/src/Image.php +++ b/src/Image.php @@ -797,8 +797,7 @@ public static function newFromArray( $width = count($array[0]); $n = $width * $height; - $ctype = \FFI::arrayType(\FFI::type("double"), [$n]); - $a = \FFI::new($ctype, true, true); + $a = \FFI::new("double[$n]", true, true); for ($y = 0; $y < $height; $y++) { for ($x = 0; $x < $width; $x++) { $a[$x + $y * $width] = $array[$y][$x]; @@ -975,8 +974,7 @@ public function writeToBuffer(string $suffix, array $options = []): string */ public function writeToMemory(): string { - $ctype = \FFI::arrayType(\FFI::type("size_t"), [1]); - $p_size = \FFI::new($ctype); + $p_size = \FFI::new("size_t[1]", $ctype); $pointer = FFI::vips()-> vips_image_write_to_memory($this->pointer, $p_size); @@ -1017,8 +1015,7 @@ public function writeToMemory(): string */ public function writeToArray(): array { - $ctype = \FFI::arrayType(\FFI::type("size_t"), [1]); - $p_size = \FFI::new($ctype); + $p_size = \FFI::new("size_t[1]"); $pointer = FFI::vips()-> vips_image_write_to_memory($this->pointer, $p_size); @@ -1027,10 +1024,9 @@ public function writeToArray(): array } // wrap pointer up as a C array of the right type - $n = $this->width * $this->height * $this->bands; $type_name = FFI::ftypes($this->format); - $ctype = \FFI::arrayType(\FFI::type($type_name), [$n]); - $array = \FFI::cast($ctype, $pointer); + $n = $this->width * $this->height * $this->bands; + $array = \FFI::cast("$type_name[$n]", $pointer); // copy to PHP memory as a flat array $result = []; From 13fb670cca15c0254e6160c94e20a4c27b01b5dc Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Sat, 12 Nov 2022 11:18:51 +0000 Subject: [PATCH 4/5] tests pass --- src/Image.php | 4 ++-- src/VipsObject.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Image.php b/src/Image.php index 99c196c..bbddf07 100644 --- a/src/Image.php +++ b/src/Image.php @@ -974,7 +974,7 @@ public function writeToBuffer(string $suffix, array $options = []): string */ public function writeToMemory(): string { - $p_size = \FFI::new("size_t[1]", $ctype); + $p_size = \FFI::new("size_t[1]"); $pointer = FFI::vips()-> vips_image_write_to_memory($this->pointer, $p_size); @@ -1026,7 +1026,7 @@ public function writeToArray(): array // wrap pointer up as a C array of the right type $type_name = FFI::ftypes($this->format); $n = $this->width * $this->height * $this->bands; - $array = \FFI::cast("$type_name[$n]", $pointer); + $array = \FFI::cast("{$type_name}[$n]", $pointer); // copy to PHP memory as a flat array $result = []; diff --git a/src/VipsObject.php b/src/VipsObject.php index 6617aee..74bb83b 100644 --- a/src/VipsObject.php +++ b/src/VipsObject.php @@ -107,10 +107,10 @@ public function getPspec(string $name): ?\FFI\CData $pspec = null; } else { /* php-ffi seems to leak if we do the obvious $pspec_array[0] to - * get the return result ... instead, we must make a new pointer + * get the return result ... instead, we must make a new pointer * object and copy the value ourselves * - * the returns values from vips_object_get_argument() are static, + * the returns values from vips_object_get_argument() are static, * so this is safe */ $pspec = FFI::gobject()->new("GParamSpec*"); From 4cc7ad54dffc6ce03252a69c1ab96c97de7b7857 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Sun, 13 Nov 2022 12:36:08 +0000 Subject: [PATCH 5/5] better test for disabled ffi see https://github.com/libvips/php-vips/issues/172 --- CHANGELOG.md | 1 + src/FFI.php | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0df0fd..8b215c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ All notable changes to `:vips` will be documented in this file. - refactor callBase() for maintainability - work around a php-ffi memory leak in getPspec() [levmv] - work around a php-ffi memory leak in arrayType() +- better test for disabled ffi ## 2.1.0 - 2022-10-11 diff --git a/src/FFI.php b/src/FFI.php index 8f29b87..f7a8277 100644 --- a/src/FFI.php +++ b/src/FFI.php @@ -201,6 +201,13 @@ private static function init(): void return; } + // try experimentally binding a bit of stdio ... if this fails, FFI + // has probably not been installed or enabled, and php will throw a + // useful error message + $stdio = \FFI::cdef(<<