Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Cache] memcache fix for J3.6.2, memcache(-d) performance improvements, less connections, add support for 'platform specific caching' #11565

Merged
merged 11 commits into from
Aug 30, 2016
Merged

[Cache] memcache fix for J3.6.2, memcache(-d) performance improvements, less connections, add support for 'platform specific caching' #11565

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b89158d
Fix memcache storage, refresh code
Aug 11, 2016
f5df02a
No static var
Aug 13, 2016
8c947c5
Revert ternary operator
Aug 15, 2016
a255c63
Add test whether a $index is an array
Aug 17, 2016
2886666
Merge with #11659
Aug 18, 2016
77086d2
Add flush method to storages
Aug 18, 2016
127885f
Replace 'replace' method by 'set' method
Aug 18, 2016
82ac607
Sync memcahe changes to memcached
Aug 18, 2016
3aa94f5
Fix issue with clean when getPlatformPrefix is set
Aug 18, 2016
6253625
Fix typo, avoid netsted if
Aug 19, 2016
4e5e96b
Next typo in msg Memcache => Memcached
Aug 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion build/travis/unit-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ psql -d joomla_ut -a -f "$BASE/tests/unit/schema/postgresql.sql"
# - ./build/travis/php-apache.sh
# Enable additional PHP extensions

if [[ $INSTALL_MEMCACHE == "yes" ]]; then phpenv config-add "$BASE/build/travis/phpenv/memcached.ini"; fi
if [[ $INSTALL_MEMCACHE == "yes" ]]; then phpenv config-add "$BASE/build/travis/phpenv/memcache.ini"; fi
if [[ $INSTALL_MEMCACHED == "yes" ]]; then phpenv config-add "$BASE/build/travis/phpenv/memcached.ini"; fi
if [[ $INSTALL_APC == "yes" ]]; then phpenv config-add "$BASE/build/travis/phpenv/apc-$TRAVIS_PHP_VERSION.ini"; fi
if [[ $INSTALL_APCU == "yes" && $TRAVIS_PHP_VERSION = 5.* ]]; then printf "\n" | pecl install apcu-4.0.10 && phpenv config-add "$BASE/build/travis/phpenv/apcu-$TRAVIS_PHP_VERSION.ini"; fi
Expand Down
12 changes: 12 additions & 0 deletions libraries/joomla/cache/storage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,18 @@ public function clean($group, $mode = null)
return true;
}

/**
* Flush all existing items in storage.
*
* @return boolean
*
* @since __DEPLOY_VERSION__
*/
public function flush()
{
return true;
}

/**
* Garbage collect expired cache data
*
Expand Down
209 changes: 92 additions & 117 deletions libraries/joomla/cache/storage/memcache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,6 @@ class JCacheStorageMemcache extends JCacheStorage
*/
protected static $_db = null;

/**
* Persistent session flag
*
* @var boolean
* @since 11.1
*/
protected $_persistent = false;

/**
* Payload compression level
*
Expand All @@ -52,7 +44,9 @@ public function __construct($options = array())
{
parent::__construct($options);

if (static::isSupported() && static::$_db === null)
$this->_compress = JFactory::getConfig()->get('memcache_compress', false) ? MEMCACHE_COMPRESSED : 0;

if (static::$_db === null)
{
$this->getConnection();
}
Expand All @@ -68,28 +62,59 @@ public function __construct($options = array())
*/
protected function getConnection()
{
$config = JFactory::getConfig();
$this->_persistent = $config->get('memcache_persist', true);
$this->_compress = $config->get('memcache_compress', false) == false ? 0 : MEMCACHE_COMPRESSED;
if (!static::isSupported())
{
throw new RuntimeException('Memcache Extension is not available');
}

$config = JFactory::getConfig();

$host = $config->get('memcache_server_host', 'localhost');
$port = $config->get('memcache_server_port', 11211);

// Create the memcache connection
static::$_db = new Memcache;
static::$_db->addserver($config->get('memcache_server_host', 'localhost'), $config->get('memcache_server_port', 11211), $this->_persistent);

$memcachetest = @static::$_db->connect($server['host'], $server['port']);
if ($config->get('memcache_persist', true))
{
$result = @static::$_db->pconnect($host, $port);
}
else
{
$result = @static::$_db->connect($host, $port);
}

if ($memcachetest == false)
if (!$result)
{
throw new RuntimeException('Could not connect to memcache server', 404);
static::$_db = null;

throw new RuntimeException('Could not connect to memcache server');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static::$_db should be nulled back out here. If it fails to connect, if you instantiate a second instance of this driver and the driver already has a static Memcache instance, that second instance will try to function as normal and won't throw an error.

}
}

/**
* Get a cache_id string from an id/group pair
*
* @param string $id The cache data id
* @param string $group The cache data group
*
* @return string The cache_id string
*
* @since 11.1
*/
protected function _getCacheId($id, $group)
{
$prefix = JCache::getPlatformPrefix();
$length = strlen($prefix);
$cache_id = parent::_getCacheId($id, $group);

// Memcahed has no list keys, we do our own accounting, initialise key index
if (static::$_db->get($this->_hash . '-index') === false)
if ($length)
{
static::$_db->set($this->_hash . '-index', array(), $this->_compress, 0);
// Memcache use suffix instead of prefix
$cache_id = substr($cache_id, $length) . strrev($prefix);
}

return;
return $cache_id;
}

/**
Expand Down Expand Up @@ -122,7 +147,7 @@ public function getAll()

$data = array();

if (!empty($keys))
if (is_array($keys))
{
foreach ($keys as $key)
{
Expand Down Expand Up @@ -178,7 +203,7 @@ public function store($id, $group, $data)

$index = static::$_db->get($this->_hash . '-index');

if ($index === false)
if (!is_array($index))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the right change here.

Consider what variable types $index can have from line 179 .

$index = static::$_db->get($this->_hash . '-index');

static::$_db->get(...) .i.e Memcache::get It could have a string type if it was passed a string key, or it could have an array type if an array of keys was passed to the method, or it could return the exact value boolean false on failure.

IMO this change incorrectly assumes that if $index is just a string associated with a key that the value can be replaced with an empty array before appending new data to be stored. We can do that with false since a failure at a get() (empty value found) before set() doesn't affect storing new data since any valid errors are handled elsewhere.

My suggestion of using array_push() over the array modifier [] syntax does handle the $index is a string or an array without additional conditional checks. (although there is some overhead, so there is some small performance hit here)

If you want to use conditionals you would need something like the following and type cast the string case of $index

        if ($index === false)
        {
            $index = array();
        }
        elseif (!is_array($index))
        {
            $index = (array) $index;
        }

{
$index = array();
}
Expand All @@ -188,14 +213,10 @@ public function store($id, $group, $data)
$tmparr->size = strlen($data);

$index[] = $tmparr;
static::$_db->replace($this->_hash . '-index', $index, 0, 0);
static::$_db->set($this->_hash . '-index', $index, 0, 0);
$this->unlockindex();

// Prevent double writes, write only if it doesn't exist else replace
if (!static::$_db->replace($cache_id, $data, $this->_compress, $this->_lifetime))
{
static::$_db->set($cache_id, $data, $this->_compress, $this->_lifetime);
}
static::$_db->set($cache_id, $data, $this->_compress, $this->_lifetime);

return true;
}
Expand All @@ -221,22 +242,19 @@ public function remove($id, $group)

$index = static::$_db->get($this->_hash . '-index');

if ($index === false)
{
$index = array();
}

foreach ($index as $key => $value)
if (is_array($index))
{
if ($value->name == $cache_id)
foreach ($index as $key => $value)
{
unset($index[$key]);
if ($value->name == $cache_id)
{
unset($index[$key]);
static::$_db->set($this->_hash . '-index', $index, 0, 0);
break;
}
}

break;
}

static::$_db->replace($this->_hash . '-index', $index, 0, 0);
$this->unlockindex();

return static::$_db->delete($cache_id);
Expand Down Expand Up @@ -264,51 +282,54 @@ public function clean($group, $mode = null)

$index = static::$_db->get($this->_hash . '-index');

if ($index === false)
if (is_array($index))
{
$index = array();
}
$prefix = $this->_hash . '-cache-' . $group . '-';

$secret = $this->_hash;

foreach ($index as $key => $value)
{
if (strpos($value->name, $secret . '-cache-' . $group . '-') === 0 xor $mode != 'group')
foreach ($index as $key => $value)
{
static::$_db->delete($value->name, 0);
unset($index[$key]);
if (strpos($value->name, $prefix) === 0 xor $mode != 'group')
{
static::$_db->delete($value->name);
unset($index[$key]);
}
}

static::$_db->set($this->_hash . '-index', $index, 0, 0);
}

static::$_db->replace($this->_hash . '-index', $index, 0, 0);
$this->unlockindex();

return true;
}

/**
* Test to see if the storage handler is available.
* Flush all existing items in storage.
*
* @return boolean
*
* @since 12.1
* @since __DEPLOY_VERSION__
*/
public static function isSupported()
public function flush()
{
// First check if the PHP requirements are met
$supported = extension_loaded('memcache') && class_exists('Memcache');

if (!$supported)
if (!$this->lockindex())
{
return false;
}

// Now check if we can connect to the specified Memcache server
$config = JFactory::getConfig();

$memcache = new Memcache;
return static::$_db->flush();
}

return @$memcache->connect($config->get('memcache_server_host', 'localhost'), $config->get('memcache_server_port', 11211));
/**
* Test to see if the storage handler is available.
*
* @return boolean
*
* @since 12.1
*/
public static function isSupported()
{
return extension_loaded('memcache') && class_exists('Memcache');
}

/**
Expand All @@ -324,53 +345,34 @@ public static function isSupported()
*/
public function lock($id, $group, $locktime)
{
$returning = new stdClass;
$returning = new stdClass;
$returning->locklooped = false;

$looptime = $locktime * 10;

$cache_id = $this->_getCacheId($id, $group);

if (!$this->lockindex())
{
return false;
}

$index = static::$_db->get($this->_hash . '-index');

if ($index === false)
{
$index = array();
}

$tmparr = new stdClass;
$tmparr->name = $cache_id;
$tmparr->size = 1;

$index[] = $tmparr;
static::$_db->replace($this->_hash . '-index', $index, 0, 0);
$this->unlockindex();

$data_lock = static::$_db->add($cache_id . '_lock', 1, false, $locktime);
$data_lock = static::$_db->add($cache_id . '_lock', 1, 0, $locktime);

if ($data_lock === false)
{
$lock_counter = 0;

// Loop until you find that the lock has been released. That implies that data get from other thread has finished
// Loop until you find that the lock has been released.
// That implies that data get from other thread has finished.
while ($data_lock === false)
{
if ($lock_counter > $looptime)
{
$returning->locked = false;
$returning->locklooped = true;
break;
}

usleep(100);
$data_lock = static::$_db->add($cache_id . '_lock', 1, false, $locktime);
$data_lock = static::$_db->add($cache_id . '_lock', 1, 0, $locktime);
$lock_counter++;
}

$returning->locklooped = true;
}

$returning->locked = $data_lock;
Expand All @@ -391,32 +393,6 @@ public function lock($id, $group, $locktime)
public function unlock($id, $group = null)
{
$cache_id = $this->_getCacheId($id, $group) . '_lock';

if (!$this->lockindex())
{
return false;
}

$index = static::$_db->get($this->_hash . '-index');

if ($index === false)
{
$index = array();
}

foreach ($index as $key => $value)
{
if ($value->name == $cache_id)
{
unset($index[$key]);
}

break;
}

static::$_db->replace($this->_hash . '-index', $index, 0, 0);
$this->unlockindex();

return static::$_db->delete($cache_id);
}

Expand All @@ -430,7 +406,7 @@ public function unlock($id, $group = null)
protected function lockindex()
{
$looptime = 300;
$data_lock = static::$_db->add($this->_hash . '-index_lock', 1, false, 30);
$data_lock = static::$_db->add($this->_hash . '-index_lock', 1, 0, 30);
Copy link
Contributor

@photodude photodude Aug 15, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe false is preferred over bool 0 or 1 in our code style. false is also used in the Memcache::add() examples

so possibly better as false?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever the documented type is should be what's used. If PHP 7 scalar
typehinting were applied and it fails, it should be changed.

On Sunday, August 14, 2016, Walt Sorensen notifications@github.com wrote:

In libraries/joomla/cache/storage/memcache.php
#11565 (comment):

@@ -429,7 +357,7 @@ public function unlock($id, $group = null)
protected function lockindex()
{
$looptime = 300;

  •   $data_lock = static::$_db->add($this->_hash . '-index_lock', 1, false, 30);

  •   $data_lock = static::$_db->add($this->_hash . '-index_lock', 1, 0, 30);

I believe false is preferred over bool 0 or 1 in our code style. false is
also used in the Memcache::add() examples
http://php.net/manual/en/memcache.add.php#refsect1-memcache.add-examples

so possibly better as false.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/11565/files/3097947c737cb9739787a57ef99051cfa904e5f3#r74715747,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoSK_pnJyWLGnNk6NoWKRDDQRMEJGks5qf9APgaJpZM4JiqOc
.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another example http://php.net/manual/en/memcache.set.php#example-4966.
And definition:

bool Memcache::set ( string $key , mixed $var [, int $flag [, int $expire ]] )

IMHO false was used some time ago, now we should use 0.


if ($data_lock === false)
{
Expand All @@ -442,11 +418,10 @@ protected function lockindex()
if ($lock_counter > $looptime)
{
return false;
break;
}

usleep(100);
$data_lock = static::$_db->add($this->_hash . '-index_lock', 1, false, 30);
$data_lock = static::$_db->add($this->_hash . '-index_lock', 1, 0, 30);
$lock_counter++;
}
}
Expand Down