From 7dff0c3b403b46be2630976462804a00cbb816c6 Mon Sep 17 00:00:00 2001 From: Volodymyr Panivko Date: Tue, 12 May 2026 18:37:36 +0200 Subject: [PATCH] THRIFT-5984: Cache function_exists() capability checks in PHP runtime Client: php MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Memoize PHP capability probes that previously ran on every hot call: - TBinarySerializer::serialize / deserialize — previously called function_exists('thrift_protocol_{write,read}_binary') on every invocation. Now cached via static nullable bool flags + hasAcceleratedWrite() / hasAcceleratedRead() helpers using the null-coalescing assignment (??=) operator. - TSocket::open — previously called function_exists() twice per open() to detect the sockets extension. Cached via hasSocketsExtension() helper. - TSocketPool — replace per-instance $useApcuCache property with static $hasApcuCache + hasApcuCache() helper for consistency with the rest of the cleanup. APCu availability is a process-level property, not per-pool. Each helper performs the check at most once per PHP process. Pure optimization — no behavior change. Tests: - Add cache resets in setUp() of TBinarySerializerTest, TSocketTest, TSocketPoolTest via reflection so mocked function_exists is re-evaluated per test method. - Register lib/php/src/ext/thrift_protocol/php_thrift_protocol.stub.php in phpstan.neon scanFiles so PHPStan knows about the C-extension functions guarded by the new helpers (previously the inline function_exists() gave PHPStan a narrowing hint that the helper hides). Part of the umbrella ticket THRIFT-5960 (PHP modernization). Generated-by: Claude Opus 4.7 --- .gitignore | 1 + lib/php/lib/Serializer/TBinarySerializer.php | 13 +++++++++++-- lib/php/lib/Transport/TSocket.php | 10 +++++++++- lib/php/lib/Transport/TSocketPool.php | 16 ++++++++-------- lib/php/phpstan.neon | 2 ++ .../Lib/Serializer/TBinarySerializerTest.php | 13 +++++++++++++ .../test/Unit/Lib/Transport/TSocketPoolTest.php | 6 ++++++ lib/php/test/Unit/Lib/Transport/TSocketTest.php | 6 ++++++ 8 files changed, 56 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index ae97e23bde..58a3732724 100644 --- a/.gitignore +++ b/.gitignore @@ -459,3 +459,4 @@ compiler/cpp/tests/bin/ compiler/cpp/tests/*.a compiler/cpp/tests/build/ compiler/cpp/build/ +/lib/php/coverage.xml diff --git a/lib/php/lib/Serializer/TBinarySerializer.php b/lib/php/lib/Serializer/TBinarySerializer.php index 888ae7e345..6f659d2dc2 100644 --- a/lib/php/lib/Serializer/TBinarySerializer.php +++ b/lib/php/lib/Serializer/TBinarySerializer.php @@ -36,6 +36,8 @@ */ class TBinarySerializer { + private static ?bool $hasAcceleratedProtocol = null; + // NOTE(rmarin): Because thrift_protocol_write_binary // adds a begin message prefix, you cannot specify // a transport in which to serialize an object. It has to @@ -45,7 +47,7 @@ public static function serialize($object) { $transport = new TMemoryBuffer(); $protocol = new TBinaryProtocolAccelerated($transport); - if (function_exists('thrift_protocol_write_binary')) { + if (self::hasAcceleratedProtocol()) { thrift_protocol_write_binary( $protocol, $object->getName(), @@ -68,7 +70,7 @@ public static function deserialize($string_object, $class_name, $buffer_size = 8 { $transport = new TMemoryBuffer(); $protocol = new TBinaryProtocolAccelerated($transport); - if (function_exists('thrift_protocol_read_binary')) { + if (self::hasAcceleratedProtocol()) { // NOTE (t.heintz) TBinaryProtocolAccelerated internally wraps our TMemoryBuffer in a // TBufferedTransport, so we have to retrieve it again or risk losing data when writing // less than 512 bytes to the transport (see the comment there as well). @@ -87,4 +89,11 @@ public static function deserialize($string_object, $class_name, $buffer_size = 8 return $object; } } + + private static function hasAcceleratedProtocol(): bool + { + return self::$hasAcceleratedProtocol ??= + function_exists('thrift_protocol_write_binary') + && function_exists('thrift_protocol_read_binary'); + } } diff --git a/lib/php/lib/Transport/TSocket.php b/lib/php/lib/Transport/TSocket.php index 5b3cbf9c86..6b31f81e6f 100644 --- a/lib/php/lib/Transport/TSocket.php +++ b/lib/php/lib/Transport/TSocket.php @@ -40,6 +40,8 @@ class TSocket extends TTransport */ public const DEFAULT_DEBUG_HANDLER = 'error_log'; + private static ?bool $hasSocketsExtension = null; + /** * Handle to PHP socket * @@ -191,7 +193,7 @@ public function open(): void throw new TException($error); } - if (function_exists('socket_import_stream') && function_exists('socket_set_option')) { + if (self::hasSocketsExtension()) { // warnings silenced due to bug https://bugs.php.net/bug.php?id=70939 $socket = socket_import_stream($this->handle); if ($socket !== false) { @@ -200,6 +202,12 @@ public function open(): void } } + private static function hasSocketsExtension(): bool + { + return self::$hasSocketsExtension ??= + function_exists('socket_import_stream') && function_exists('socket_set_option'); + } + public function close(): void { @fclose($this->handle); diff --git a/lib/php/lib/Transport/TSocketPool.php b/lib/php/lib/Transport/TSocketPool.php index 534f3401eb..726983dc5c 100644 --- a/lib/php/lib/Transport/TSocketPool.php +++ b/lib/php/lib/Transport/TSocketPool.php @@ -68,10 +68,7 @@ class TSocketPool extends TSocket */ private bool $alwaysTryLast = true; - /** - * Use apcu cache - */ - private bool $useApcuCache; + private static ?bool $hasApcuCache = null; /** * Socket pool constructor @@ -100,8 +97,6 @@ public function __construct( foreach ($hosts as $key => $host) { $this->addServer($host, $ports[$key]); } - - $this->useApcuCache = function_exists('apcu_fetch'); } /** @@ -276,11 +271,16 @@ public function open(): void */ private function apcuFetch($key, &$success = null) { - return $this->useApcuCache ? apcu_fetch($key, $success) : false; + return self::hasApcuCache() ? apcu_fetch($key, $success) : false; } private function apcuStore($key, $var, $ttl = 0) { - return $this->useApcuCache ? apcu_store($key, $var, $ttl) : false; + return self::hasApcuCache() ? apcu_store($key, $var, $ttl) : false; + } + + private static function hasApcuCache(): bool + { + return self::$hasApcuCache ??= function_exists('apcu_fetch'); } } diff --git a/lib/php/phpstan.neon b/lib/php/phpstan.neon index 19850154dc..504a9a6481 100644 --- a/lib/php/phpstan.neon +++ b/lib/php/phpstan.neon @@ -4,6 +4,8 @@ parameters: - lib bootstrapFiles: - test/bootstrap.php + scanFiles: + - src/ext/thrift_protocol/php_thrift_protocol.stub.php treatPhpDocTypesAsCertain: false excludePaths: - test/Resources/packages/* diff --git a/lib/php/test/Unit/Lib/Serializer/TBinarySerializerTest.php b/lib/php/test/Unit/Lib/Serializer/TBinarySerializerTest.php index 16298795c0..ddd45bb2af 100644 --- a/lib/php/test/Unit/Lib/Serializer/TBinarySerializerTest.php +++ b/lib/php/test/Unit/Lib/Serializer/TBinarySerializerTest.php @@ -28,11 +28,21 @@ use PHPUnit\Framework\TestCase; use PHPUnit\Framework\Attributes\DataProvider; use Test\Thrift\Unit\Lib\Fixture\TestSerializerStruct; +use Test\Thrift\Unit\Lib\ReflectionHelper; use Thrift\Serializer\TBinarySerializer; class TBinarySerializerTest extends TestCase { use PHPMock; + use ReflectionHelper; + + protected function setUp(): void + { + self::defineFunctionMock('Thrift\Serializer', 'function_exists'); + + $this->getAccessibleProperty(TBinarySerializer::class, 'hasAcceleratedProtocol') + ->setValue(null, null); + } #[DataProvider('serializeDeserializeDataProvider')] public function testSerializeAndDeserialize($stringVal, $intVal) @@ -136,6 +146,9 @@ public function testDeserializeWithAcceleratedExtension() $serialized = TBinarySerializer::serialize($object); + $this->getAccessibleProperty(TBinarySerializer::class, 'hasAcceleratedProtocol') + ->setValue(null, null); + $funcExists = $this->getFunctionMock('Thrift\Serializer', 'function_exists'); $funcExists->expects($this->atLeastOnce()) ->willReturn(true); diff --git a/lib/php/test/Unit/Lib/Transport/TSocketPoolTest.php b/lib/php/test/Unit/Lib/Transport/TSocketPoolTest.php index 45e40899e7..e726de4fba 100644 --- a/lib/php/test/Unit/Lib/Transport/TSocketPoolTest.php +++ b/lib/php/test/Unit/Lib/Transport/TSocketPoolTest.php @@ -30,6 +30,7 @@ use PHPUnit\Framework\Attributes\DataProvider; use Test\Thrift\Unit\Lib\ReflectionHelper; use Thrift\Exception\TException; +use Thrift\Transport\TSocket; use Thrift\Transport\TSocketPool; class TSocketPoolTest extends TestCase @@ -41,6 +42,11 @@ protected function setUp(): void { #need to be defined before the TSocketPool class definition self::defineFunctionMock('Thrift\Transport', 'function_exists'); + + $this->getAccessibleProperty(TSocketPool::class, 'hasApcuCache') + ->setValue(null, null); + $this->getAccessibleProperty(TSocket::class, 'hasSocketsExtension') + ->setValue(null, null); } #[DataProvider('constructDataProvider')] diff --git a/lib/php/test/Unit/Lib/Transport/TSocketTest.php b/lib/php/test/Unit/Lib/Transport/TSocketTest.php index be150e0953..16cc48907b 100644 --- a/lib/php/test/Unit/Lib/Transport/TSocketTest.php +++ b/lib/php/test/Unit/Lib/Transport/TSocketTest.php @@ -36,6 +36,12 @@ class TSocketTest extends TestCase use PHPMock; use ReflectionHelper; + protected function setUp(): void + { + $this->getAccessibleProperty(TSocket::class, 'hasSocketsExtension') + ->setValue(null, null); + } + #[DataProvider('openExceptionDataProvider')] public function testOpenException( $host,