Skip to content

Commit

Permalink
feature #17654 [Cache] Don't clone, serialize (nicolas-grekas)
Browse files Browse the repository at this point in the history
This PR was merged into the 3.1-dev branch.

Discussion
----------

[Cache] Don't clone, serialize

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Cloning is not required by PSR-6, adds overhead and doesn't prevent any mistake for nested objects. Let's simplify.
ArrayCache in turn should store serialized by default, so that data is deep-cloned. But since this can cause a perf issue for some, I propose to add a constructor flag to disable serializing. WDYT?

Commits
-------

8605417 [Cache] Don't clone, serialize
  • Loading branch information
nicolas-grekas committed Feb 3, 2016
2 parents 0bacaba + 8605417 commit b33eb05
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 24 deletions.
7 changes: 0 additions & 7 deletions src/Symfony/Component/Cache/Adapter/AbstractAdapter.php
Expand Up @@ -276,13 +276,6 @@ public function saveDeferred(CacheItemInterface $item)
if (!$item instanceof CacheItem) {
return false;
}
try {
$item = clone $item;
} catch (\Exception $e) {
$value = $item->get();
$type = is_object($value) ? get_class($value) : gettype($value);
CacheItem::log($this->logger, 'Failed to clone key "{key}" ({type})', array('key' => $key, 'type' => $type, 'exception' => $e));
}
$this->deferred[$item->getKey()] = $item;

return true;
Expand Down
35 changes: 26 additions & 9 deletions src/Symfony/Component/Cache/Adapter/ArrayAdapter.php
Expand Up @@ -25,12 +25,18 @@ class ArrayAdapter implements CacheItemPoolInterface, LoggerAwareInterface
{
use LoggerAwareTrait;

private $storeSerialized;
private $values = array();
private $expiries = array();
private $createCacheItem;

public function __construct($defaultLifetime = 0)
/**
* @param int $defaultLifetime
* @param bool $storeSerialized Disabling serialization can lead to cache corruptions when storing mutable values but increases performance otherwise.
*/
public function __construct($defaultLifetime = 0, $storeSerialized = true)
{
$this->storeSerialized = $storeSerialized;
$this->createCacheItem = \Closure::bind(
function ($key, $value, $isHit) use ($defaultLifetime) {
$item = new CacheItem();
Expand All @@ -51,10 +57,16 @@ function ($key, $value, $isHit) use ($defaultLifetime) {
*/
public function getItem($key)
{
if (!$isHit = $this->hasItem($key)) {
$value = null;
} elseif ($this->storeSerialized) {
$value = unserialize($this->values[$key]);
} else {
$value = $this->values[$key];
}
$f = $this->createCacheItem;
$isHit = $this->hasItem($key);

return $f($key, $isHit ? $this->values[$key] : null, $isHit);
return $f($key, $value, $isHit);
}

/**
Expand Down Expand Up @@ -125,13 +137,12 @@ public function save(CacheItemInterface $item)
if (0 > $lifetime) {
return true;
}

if (is_object($value)) {
if ($this->storeSerialized) {
try {
$value = clone $value;
$value = serialize($value);
} catch (\Exception $e) {
$type = is_object($value) ? get_class($value) : gettype($value);
CacheItem::log($this->logger, 'Failed to clone key "{key}" ({type})', array('key' => $key, 'type' => $type, 'exception' => $e));
CacheItem::log($this->logger, 'Failed to save key "{key}" ({type})', array('key' => $key, 'type' => $type, 'exception' => $e));

return false;
}
Expand Down Expand Up @@ -179,9 +190,15 @@ private function generateItems(array $keys)
$f = $this->createCacheItem;

foreach ($keys as $key) {
$isHit = isset($this->expiries[$key]) && ($this->expiries[$key] >= time() || !$this->deleteItem($key));
if (!$isHit = isset($this->expiries[$key]) && ($this->expiries[$key] >= time() || !$this->deleteItem($key))) {
$value = null;
} elseif ($this->storeSerialized) {
$value = unserialize($this->values[$key]);
} else {
$value = $this->values[$key];
}

yield $key => $f($key, $isHit ? $this->values[$key] : null, $isHit);
yield $key => $f($key, $value, $isHit);
}
}
}
9 changes: 1 addition & 8 deletions src/Symfony/Component/Cache/CacheItem.php
Expand Up @@ -31,13 +31,6 @@ final class CacheItem implements CacheItemInterface
private $lifetime;
private $defaultLifetime;

public function __clone()
{
if (is_object($this->value)) {
$this->value = clone $this->value;
}
}

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -112,7 +105,7 @@ public function expiresAfter($time)
*
* @internal
*/
public function log(LoggerInterface $logger = null, $message, $context = array())
public static function log(LoggerInterface $logger = null, $message, $context = array())
{
if ($logger) {
$logger->warning($message, $context);
Expand Down

0 comments on commit b33eb05

Please sign in to comment.