objectcache: use OPT_TCP_NODELAY for MemcachedPeclBagOStuff
authorAaron Schulz <aschulz@wikimedia.org>
Sat, 29 Jun 2019 19:12:03 +0000 (12:12 -0700)
committerAaron Schulz <aschulz@wikimedia.org>
Wed, 10 Jul 2019 21:58:02 +0000 (21:58 +0000)
Clean up the option setting code in __construct() as well
and make it more strict in terms of checking bogus options

Make mctest.php recognize when memcached is used more easily

Change-Id: I77be506e80478ddbb4fcfa44d82aba8162fa984f

includes/libs/objectcache/MemcachedBagOStuff.php
includes/libs/objectcache/MemcachedPeclBagOStuff.php
includes/libs/objectcache/MemcachedPhpBagOStuff.php
maintenance/mctest.php

index cfbf2b3..f75e780 100644 (file)
@@ -34,20 +34,6 @@ abstract class MemcachedBagOStuff extends BagOStuff {
                $this->segmentationSize = $params['maxPreferedKeySize'] ?? 917504; // < 1MiB
        }
 
-       /**
-        * Fill in some defaults for missing keys in $params.
-        *
-        * @param array $params
-        * @return array
-        */
-       protected function applyDefaultParams( $params ) {
-               return $params + [
-                       'compress_threshold' => 1500,
-                       'connect_timeout' => 0.5,
-                       'debug' => false
-               ];
-       }
-
        /**
         * Construct a cache key.
         *
index ea0090f..f6721ce 100644 (file)
@@ -32,83 +32,104 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
 
        /**
         * Available parameters are:
-        *   - servers:             The list of IP:port combinations holding the memcached servers.
-        *   - persistent:          Whether to use a persistent connection
-        *   - compress_threshold:  The minimum size an object must be before it is compressed
-        *   - timeout:             The read timeout in microseconds
-        *   - connect_timeout:     The connect timeout in seconds
-        *   - retry_timeout:       Time in seconds to wait before retrying a failed connect attempt
-        *   - server_failure_limit:  Limit for server connect failures before it is removed
-        *   - serializer:          May be either "php" or "igbinary". Igbinary produces more compact
-        *                          values, but serialization is much slower unless the php.ini option
-        *                          igbinary.compact_strings is off.
-        *   - use_binary_protocol  Whether to enable the binary protocol (default is ASCII) (boolean)
+        *   - servers:              List of IP:port combinations holding the memcached servers.
+        *   - persistent:           Whether to use a persistent connection
+        *   - compress_threshold:   The minimum size an object must be before it is compressed
+        *   - timeout:              The read timeout in microseconds
+        *   - connect_timeout:      The connect timeout in seconds
+        *   - retry_timeout:        Time in seconds to wait before retrying a failed connect attempt
+        *   - server_failure_limit: Limit for server connect failures before it is removed
+        *   - serializer:           Either "php" or "igbinary". Igbinary produces more compact
+        *                           values, but serialization is much slower unless the php.ini
+        *                           option igbinary.compact_strings is off.
+        *   - use_binary_protocol   Whether to enable the binary protocol (default is ASCII)
+        *   - allow_tcp_nagle_delay Whether to permit Nagle's algorithm for reducing packet count
         * @param array $params
-        * @throws InvalidArgumentException
         */
        function __construct( $params ) {
                parent::__construct( $params );
-               $params = $this->applyDefaultParams( $params );
+
+               // Default class-specific parameters
+               $params += [
+                       'compress_threshold' => 1500,
+                       'connect_timeout' => 0.5,
+                       'serializer' => 'php',
+                       'use_binary_protocol' => false,
+                       'allow_tcp_nagle_delay' => true
+               ];
 
                if ( $params['persistent'] ) {
                        // The pool ID must be unique to the server/option combination.
                        // The Memcached object is essentially shared for each pool ID.
                        // We can only reuse a pool ID if we keep the config consistent.
-                       $this->client = new Memcached( md5( serialize( $params ) ) );
-                       if ( count( $this->client->getServerList() ) ) {
-                               $this->logger->debug( __METHOD__ . ": persistent Memcached object already loaded." );
-                               return; // already initialized; don't add duplicate servers
-                       }
+                       $connectionPoolId = md5( serialize( $params ) );
+                       $client = new Memcached( $connectionPoolId );
+                       $this->initializeClient( $client, $params );
                } else {
-                       $this->client = new Memcached;
+                       $client = new Memcached;
+                       $this->initializeClient( $client, $params );
                }
 
-               if ( $params['use_binary_protocol'] ) {
-                       $this->client->setOption( Memcached::OPT_BINARY_PROTOCOL, true );
-               }
-
-               if ( isset( $params['retry_timeout'] ) ) {
-                       $this->client->setOption( Memcached::OPT_RETRY_TIMEOUT, $params['retry_timeout'] );
-               }
-
-               if ( isset( $params['server_failure_limit'] ) ) {
-                       $this->client->setOption( Memcached::OPT_SERVER_FAILURE_LIMIT, $params['server_failure_limit'] );
-               }
+               $this->client = $client;
 
                // The compression threshold is an undocumented php.ini option for some
                // reason. There's probably not much harm in setting it globally, for
                // compatibility with the settings for the PHP client.
                ini_set( 'memcached.compression_threshold', $params['compress_threshold'] );
+       }
 
-               // Set timeouts
-               $this->client->setOption( Memcached::OPT_CONNECT_TIMEOUT, $params['connect_timeout'] * 1000 );
-               $this->client->setOption( Memcached::OPT_SEND_TIMEOUT, $params['timeout'] );
-               $this->client->setOption( Memcached::OPT_RECV_TIMEOUT, $params['timeout'] );
-               $this->client->setOption( Memcached::OPT_POLL_TIMEOUT, $params['timeout'] / 1000 );
+       /**
+        * Initialize the client only if needed and reuse it otherwise.
+        * This avoids duplicate servers in the list and new connections.
+        *
+        * @param Memcached $client
+        * @param array $params
+        * @throws RuntimeException
+        */
+       private function initializeClient( Memcached $client, array $params ) {
+               if ( $client->getServerList() ) {
+                       $this->logger->debug( __METHOD__ . ": pre-initialized client instance." );
 
-               // Set libketama mode since it's recommended by the documentation and
-               // is as good as any. There's no way to configure libmemcached to use
-               // hashes identical to the ones currently in use by the PHP client, and
-               // even implementing one of the libmemcached hashes in pure PHP for
-               // forwards compatibility would require MemcachedClient::get_sock() to be
-               // rewritten.
-               $this->client->setOption( Memcached::OPT_LIBKETAMA_COMPATIBLE, true );
+                       return; // preserve persistent handle
+               }
 
-               // Set the serializer
-               $ok = false;
+               $this->logger->debug( __METHOD__ . ": initializing new client instance." );
+
+               $options = [
+                       // Network protocol (ASCII or binary)
+                       Memcached::OPT_BINARY_PROTOCOL => $params['use_binary_protocol'],
+                       // Set various network timeouts
+                       Memcached::OPT_CONNECT_TIMEOUT => $params['connect_timeout'] * 1000,
+                       Memcached::OPT_SEND_TIMEOUT => $params['timeout'],
+                       Memcached::OPT_RECV_TIMEOUT => $params['timeout'],
+                       Memcached::OPT_POLL_TIMEOUT => $params['timeout'] / 1000,
+                       // Avoid pointless delay when sending/fetching large blobs
+                       Memcached::OPT_TCP_NODELAY => !$params['allow_tcp_nagle_delay'],
+                       // Set libketama mode since it's recommended by the documentation
+                       Memcached::OPT_LIBKETAMA_COMPATIBLE => true
+               ];
+               if ( isset( $params['retry_timeout'] ) ) {
+                       $options[Memcached::OPT_RETRY_TIMEOUT] = $params['retry_timeout'];
+               }
+               if ( isset( $params['server_failure_limit'] ) ) {
+                       $options[Memcached::OPT_SERVER_FAILURE_LIMIT] = $params['server_failure_limit'];
+               }
                if ( $params['serializer'] === 'php' ) {
-                       $ok = $this->client->setOption( Memcached::OPT_SERIALIZER, Memcached::SERIALIZER_PHP );
+                       $options[Memcached::OPT_SERIALIZER] = Memcached::SERIALIZER_PHP;
                } elseif ( $params['serializer'] === 'igbinary' ) {
                        if ( !Memcached::HAVE_IGBINARY ) {
-                               throw new InvalidArgumentException(
+                               throw new RuntimeException(
                                        __CLASS__ . ': the igbinary extension is not available ' .
                                        'but igbinary serialization was requested.'
                                );
                        }
-                       $ok = $this->client->setOption( Memcached::OPT_SERIALIZER, Memcached::SERIALIZER_IGBINARY );
+                       $options[Memcached::OPT_SERIALIZER] = Memcached::SERIALIZER_IGBINARY;
                }
-               if ( !$ok ) {
-                       throw new InvalidArgumentException( __CLASS__ . ': invalid serializer parameter' );
+
+               if ( !$client->setOptions( $options ) ) {
+                       throw new RuntimeException(
+                               "Invalid options: " . json_encode( $options, JSON_PRETTY_PRINT )
+                       );
                }
 
                $servers = [];
@@ -121,26 +142,16 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
                                $servers[] = [ $host, false ]; // (ip or path, port)
                        }
                }
-               $this->client->addServers( $servers );
-       }
-
-       protected function applyDefaultParams( $params ) {
-               $params = parent::applyDefaultParams( $params );
-
-               if ( !isset( $params['use_binary_protocol'] ) ) {
-                       $params['use_binary_protocol'] = false;
-               }
 
-               if ( !isset( $params['serializer'] ) ) {
-                       $params['serializer'] = 'php';
+               if ( !$client->addServers( $servers ) ) {
+                       throw new RuntimeException( "Failed to inject server address list" );
                }
-
-               return $params;
        }
 
        protected function doGet( $key, $flags = 0, &$casToken = null ) {
                $this->debug( "get($key)" );
                if ( defined( Memcached::class . '::GET_EXTENDED' ) ) { // v3.0.0
+                       /** @noinspection PhpUndefinedClassConstantInspection */
                        $flags = Memcached::GET_EXTENDED;
                        $res = $this->client->get( $this->validateKeyEncoding( $key ), null, $flags );
                        if ( is_array( $res ) ) {
index 5a67a0d..5d82fee 100644 (file)
@@ -33,7 +33,6 @@ class MemcachedPhpBagOStuff extends MemcachedBagOStuff {
        /**
         * Available parameters are:
         *   - servers:             The list of IP:port combinations holding the memcached servers.
-        *   - debug:               Whether to set the debug flag in the underlying client.
         *   - persistent:          Whether to use a persistent connection
         *   - compress_threshold:  The minimum size an object must be before it is compressed
         *   - timeout:             The read timeout in microseconds
@@ -43,11 +42,15 @@ class MemcachedPhpBagOStuff extends MemcachedBagOStuff {
         */
        function __construct( $params ) {
                parent::__construct( $params );
-               $params = $this->applyDefaultParams( $params );
+
+               // Default class-specific parameters
+               $params += [
+                       'compress_threshold' => 1500,
+                       'connect_timeout' => 0.5
+               ];
 
                $this->client = new MemcachedClient( $params );
                $this->client->set_servers( $params['servers'] );
-               $this->client->set_debug( $params['debug'] );
        }
 
        public function setDebug( $debug ) {
index 98f0eb9..513edf3 100644 (file)
@@ -43,6 +43,8 @@ class McTest extends Maintenance {
        public function execute() {
                global $wgMainCacheType, $wgMemCachedTimeout, $wgObjectCaches;
 
+               $memcachedTypes = [ CACHE_MEMCACHED, 'memcached-php', 'memcached-pecl' ];
+
                $cache = $this->getOption( 'cache' );
                $iterations = $this->getOption( 'i', 100 );
                if ( $cache ) {
@@ -52,7 +54,7 @@ class McTest extends Maintenance {
                        $servers = $wgObjectCaches[$cache]['servers'];
                } elseif ( $this->hasArg( 0 ) ) {
                        $servers = [ $this->getArg( 0 ) ];
-               } elseif ( $wgMainCacheType === CACHE_MEMCACHED ) {
+               } elseif ( in_array( $wgMainCacheType, $memcachedTypes, true ) ) {
                        global $wgMemCachedServers;
                        $servers = $wgMemCachedServers;
                } elseif ( isset( $wgObjectCaches[$wgMainCacheType]['servers'] ) ) {