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.

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

Make query cache rely on entity timestamp region #6001

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Upgrade to 2.5

## Minor BC BREAK: query cache key time is now a float

As of 2.5, the `QueryCacheEntry#time` property will contain a float value
instead of an integer in order to have more precision and also to be consistent
with the `TimestampCacheEntry#time`.

## Minor BC BREAK: discriminator map must now include all non-transient classes

It is now required that you declare the root of an inheritance in the
Expand Down
35 changes: 29 additions & 6 deletions lib/Doctrine/ORM/AbstractQuery.php
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
use Doctrine\DBAL\Cache\QueryCacheProfile;

use Doctrine\ORM\Cache;
use Doctrine\ORM\Query\ResultSetMapping;

/**
* Base contract for ORM queries. Base class for Query and NativeQuery.
Expand Down Expand Up @@ -991,32 +992,54 @@ private function executeIgnoreQueryCache($parameters = null, $hydrationMode = nu
private function executeUsingQueryCache($parameters = null, $hydrationMode = null)
{
$rsm = $this->getResultSetMapping();
$querykey = new QueryCacheKey($this->getHash(), $this->lifetime, $this->cacheMode ?: Cache::MODE_NORMAL);
$queryCache = $this->_em->getCache()->getQueryCache($this->cacheRegion);
$result = $queryCache->get($querykey, $rsm, $this->_hints);
$queryKey = new QueryCacheKey(
$this->getHash(),
$this->lifetime,
$this->cacheMode ?: Cache::MODE_NORMAL,
$this->getTimestampKey()
);

$result = $queryCache->get($queryKey, $rsm, $this->_hints);

if ($result !== null) {
if ($this->cacheLogger) {
$this->cacheLogger->queryCacheHit($queryCache->getRegion()->getName(), $querykey);
$this->cacheLogger->queryCacheHit($queryCache->getRegion()->getName(), $queryKey);
}

return $result;
}

$result = $this->executeIgnoreQueryCache($parameters, $hydrationMode);
$cached = $queryCache->put($querykey, $rsm, $result, $this->_hints);
$cached = $queryCache->put($queryKey, $rsm, $result, $this->_hints);

if ($this->cacheLogger) {
$this->cacheLogger->queryCacheMiss($queryCache->getRegion()->getName(), $querykey);
$this->cacheLogger->queryCacheMiss($queryCache->getRegion()->getName(), $queryKey);

if ($cached) {
$this->cacheLogger->queryCachePut($queryCache->getRegion()->getName(), $querykey);
$this->cacheLogger->queryCachePut($queryCache->getRegion()->getName(), $queryKey);
}
}

return $result;
}

/**
* @return \Doctrine\ORM\Cache\TimestampCacheKey|null
*/
private function getTimestampKey()
{
$entityName = reset($this->_resultSetMapping->aliasMap);

if (empty($entityName)) {
return null;
}

$metadata = $this->_em->getClassMetadata($entityName);

return new Cache\TimestampCacheKey($metadata->getTableName());
}

/**
* Get the result cache id to use to store the result set cache entry.
* Will return the configured id if it exists otherwise a hash will be
Expand Down
4 changes: 3 additions & 1 deletion lib/Doctrine/ORM/Cache/CacheConfiguration.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ public function setRegionsConfiguration(RegionsConfiguration $regionsConfig)
public function getQueryValidator()
{
if ($this->queryValidator === null) {
$this->queryValidator = new TimestampQueryCacheValidator();
$this->queryValidator = new TimestampQueryCacheValidator(
$this->cacheFactory->getTimestampRegion()
);
}

return $this->queryValidator;
Expand Down
50 changes: 23 additions & 27 deletions lib/Doctrine/ORM/Cache/Persister/Entity/AbstractEntityPersister.php
Original file line number Diff line number Diff line change
Expand Up @@ -287,17 +287,16 @@ private function storeJoinedAssociations($entity)
* @param array $orderBy
* @param integer $limit
* @param integer $offset
* @param integer $timestamp
*
* @return string
*/
protected function getHash($query, $criteria, array $orderBy = null, $limit = null, $offset = null, $timestamp = null)
protected function getHash($query, $criteria, array $orderBy = null, $limit = null, $offset = null)
{
list($params) = ($criteria instanceof Criteria)
? $this->persister->expandCriteriaParameters($criteria)
: $this->persister->expandParameters($criteria);

return sha1($query . serialize($params) . serialize($orderBy) . $limit . $offset . $timestamp);
return sha1($query . serialize($params) . serialize($orderBy) . $limit . $offset);
}

/**
Expand Down Expand Up @@ -368,17 +367,16 @@ public function load(array $criteria, $entity = null, $assoc = null, array $hint
}

//handle only EntityRepository#findOneBy
$timestamp = $this->timestampRegion->get($this->timestampKey);
$query = $this->persister->getSelectSQL($criteria, null, null, $limit, null, $orderBy);
$hash = $this->getHash($query, $criteria, null, null, null, $timestamp ? $timestamp->time : null);
$hash = $this->getHash($query, $criteria, null, null, null);
$rsm = $this->getResultSetMapping();
$querykey = new QueryCacheKey($hash, 0, Cache::MODE_NORMAL);
$queryKey = new QueryCacheKey($hash, 0, Cache::MODE_NORMAL, $this->timestampKey);
$queryCache = $this->cache->getQueryCache($this->regionName);
$result = $queryCache->get($querykey, $rsm);
$result = $queryCache->get($queryKey, $rsm);

if ($result !== null) {
if ($this->cacheLogger) {
$this->cacheLogger->queryCacheHit($this->regionName, $querykey);
$this->cacheLogger->queryCacheHit($this->regionName, $queryKey);
}

return $result[0];
Expand All @@ -388,15 +386,15 @@ public function load(array $criteria, $entity = null, $assoc = null, array $hint
return null;
}

$cached = $queryCache->put($querykey, $rsm, array($result));
$cached = $queryCache->put($queryKey, $rsm, array($result));

if ($this->cacheLogger) {
if ($result) {
$this->cacheLogger->queryCacheMiss($this->regionName, $querykey);
$this->cacheLogger->queryCacheMiss($this->regionName, $queryKey);
}

if ($cached) {
$this->cacheLogger->queryCachePut($this->regionName, $querykey);
$this->cacheLogger->queryCachePut($this->regionName, $queryKey);
}
}

Expand All @@ -408,32 +406,31 @@ public function load(array $criteria, $entity = null, $assoc = null, array $hint
*/
public function loadAll(array $criteria = array(), array $orderBy = null, $limit = null, $offset = null)
{
$timestamp = $this->timestampRegion->get($this->timestampKey);
$query = $this->persister->getSelectSQL($criteria, null, null, $limit, $offset, $orderBy);
$hash = $this->getHash($query, $criteria, null, null, null, $timestamp ? $timestamp->time : null);
$hash = $this->getHash($query, $criteria, null, null, null);
$rsm = $this->getResultSetMapping();
$querykey = new QueryCacheKey($hash, 0, Cache::MODE_NORMAL);
$queryKey = new QueryCacheKey($hash, 0, Cache::MODE_NORMAL, $this->timestampKey);
$queryCache = $this->cache->getQueryCache($this->regionName);
$result = $queryCache->get($querykey, $rsm);
$result = $queryCache->get($queryKey, $rsm);

if ($result !== null) {
if ($this->cacheLogger) {
$this->cacheLogger->queryCacheHit($this->regionName, $querykey);
$this->cacheLogger->queryCacheHit($this->regionName, $queryKey);
}

return $result;
}

$result = $this->persister->loadAll($criteria, $orderBy, $limit, $offset);
$cached = $queryCache->put($querykey, $rsm, $result);
$cached = $queryCache->put($queryKey, $rsm, $result);

if ($this->cacheLogger) {
if ($result) {
$this->cacheLogger->queryCacheMiss($this->regionName, $querykey);
$this->cacheLogger->queryCacheMiss($this->regionName, $queryKey);
}

if ($cached) {
$this->cacheLogger->queryCachePut($this->regionName, $querykey);
$this->cacheLogger->queryCachePut($this->regionName, $queryKey);
}
}

Expand Down Expand Up @@ -511,31 +508,30 @@ public function loadCriteria(Criteria $criteria)
$limit = $criteria->getMaxResults();
$offset = $criteria->getFirstResult();
$query = $this->persister->getSelectSQL($criteria);
$timestamp = $this->timestampRegion->get($this->timestampKey);
$hash = $this->getHash($query, $criteria, $orderBy, $limit, $offset, $timestamp ? $timestamp->time : null);
$hash = $this->getHash($query, $criteria, $orderBy, $limit, $offset);
$rsm = $this->getResultSetMapping();
$querykey = new QueryCacheKey($hash, 0, Cache::MODE_NORMAL);
$queryKey = new QueryCacheKey($hash, 0, Cache::MODE_NORMAL, $this->timestampKey);
$queryCache = $this->cache->getQueryCache($this->regionName);
$cacheResult = $queryCache->get($querykey, $rsm);
$cacheResult = $queryCache->get($queryKey, $rsm);

if ($cacheResult !== null) {
if ($this->cacheLogger) {
$this->cacheLogger->queryCacheHit($this->regionName, $querykey);
$this->cacheLogger->queryCacheHit($this->regionName, $queryKey);
}

return $cacheResult;
}

$result = $this->persister->loadCriteria($criteria);
$cached = $queryCache->put($querykey, $rsm, $result);
$cached = $queryCache->put($queryKey, $rsm, $result);

if ($this->cacheLogger) {
if ($result) {
$this->cacheLogger->queryCacheMiss($this->regionName, $querykey);
$this->cacheLogger->queryCacheMiss($this->regionName, $queryKey);
}

if ($cached) {
$this->cacheLogger->queryCachePut($this->regionName, $querykey);
$this->cacheLogger->queryCachePut($this->regionName, $queryKey);
}
}

Expand Down
8 changes: 4 additions & 4 deletions lib/Doctrine/ORM/Cache/QueryCacheEntry.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,18 +38,18 @@ class QueryCacheEntry implements CacheEntry
/**
* READ-ONLY: Public only for performance reasons, it should be considered immutable.
*
* @var integer Time creation of this cache entry
* @var float Time creation of this cache entry
Copy link
Member

Choose a reason for hiding this comment

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

Since there was a change here, did you check all usages of this property?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did but I definitely forgot to change ($entry->time + $key->lifetime) > time() to ((int) $entry->time + $key->lifetime) > time() on the validator

*/
public $time;

/**
* @param array $result
* @param integer $time
* @param array $result
* @param float $time
*/
public function __construct($result, $time = null)
{
$this->result = $result;
$this->time = $time ?: time();
$this->time = $time ?: microtime(true);
}

/**
Expand Down
29 changes: 21 additions & 8 deletions lib/Doctrine/ORM/Cache/QueryCacheKey.php
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,27 @@ class QueryCacheKey extends CacheKey
public $cacheMode;

/**
* @param string $hash Result cache id
* @param integer $lifetime Query lifetime
* @param integer $cacheMode Query cache mode
* READ-ONLY: Public only for performance reasons, it should be considered immutable.
*
* @var TimestampCacheKey|null
*/
public $timestampKey;

/**
* @param string $hash Result cache id
* @param integer $lifetime Query lifetime
* @param int $cacheMode Query cache mode
* @param TimestampCacheKey|null $timestampKey
Copy link
Member

Choose a reason for hiding this comment

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

When is null happening here, exactly?

Copy link
Member Author

Choose a reason for hiding this comment

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

*/
public function __construct($hash, $lifetime = 0, $cacheMode = Cache::MODE_NORMAL)
{
$this->hash = $hash;
$this->lifetime = $lifetime;
$this->cacheMode = $cacheMode;
public function __construct(
$hash,
$lifetime = 0,
$cacheMode = Cache::MODE_NORMAL,
TimestampCacheKey $timestampKey = null
) {
$this->hash = $hash;
$this->lifetime = $lifetime;
$this->cacheMode = $cacheMode;
$this->timestampKey = $timestampKey;
}
}
36 changes: 35 additions & 1 deletion lib/Doctrine/ORM/Cache/TimestampQueryCacheValidator.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,49 @@
*/
class TimestampQueryCacheValidator implements QueryCacheValidator
{
/**
* @var TimestampRegion
*/
private $timestampRegion;

/**
* @param TimestampRegion $timestampRegion
*/
public function __construct(TimestampRegion $timestampRegion)
{
$this->timestampRegion = $timestampRegion;
}

/**
* {@inheritdoc}
*/
public function isValid(QueryCacheKey $key, QueryCacheEntry $entry)
{
if ($this->regionUpdated($key, $entry)) {
return false;
}

if ($key->lifetime == 0) {
return true;
}

return ($entry->time + $key->lifetime) > time();
return ($entry->time + $key->lifetime) > microtime(true);
}

/**
* @param QueryCacheKey $key
* @param QueryCacheEntry $entry
*
* @return bool
*/
private function regionUpdated(QueryCacheKey $key, QueryCacheEntry $entry)
Copy link
Member

Choose a reason for hiding this comment

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

Docblock plz, even if it just says that a boolean is returned

Copy link
Member Author

Choose a reason for hiding this comment

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

😉

{
if ($key->timestampKey === null) {
Copy link
Member

Choose a reason for hiding this comment

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

The docblock states that it is never null: can you check that?

Copy link
Member Author

Choose a reason for hiding this comment

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

The docblock is a liar! Will fix that, thanks

return false;
}

$timestamp = $this->timestampRegion->get($key->timestampKey);

return $timestamp && $timestamp->time > $entry->time;
}
}
8 changes: 7 additions & 1 deletion tests/Doctrine/Tests/ORM/Cache/CacheConfigTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use Doctrine\ORM\Cache\CacheFactory;
use Doctrine\ORM\Cache\QueryCacheValidator;
use Doctrine\ORM\Cache\Logging\CacheLogger;
use Doctrine\ORM\Cache\TimestampRegion;
use Doctrine\Tests\DoctrineTestCase;

/**
Expand Down Expand Up @@ -67,6 +68,11 @@ public function testSetGetCacheFactory()

public function testSetGetQueryValidator()
{
$factory = $this->createMock(CacheFactory::class);
$factory->method('getTimestampRegion')->willReturn($this->createMock(TimestampRegion::class));

$this->config->setCacheFactory($factory);

$validator = $this->createMock(QueryCacheValidator::class);

$this->assertInstanceOf('Doctrine\ORM\Cache\TimestampQueryCacheValidator', $this->config->getQueryValidator());
Expand All @@ -75,4 +81,4 @@ public function testSetGetQueryValidator()

$this->assertEquals($validator, $this->config->getQueryValidator());
}
}
}
2 changes: 1 addition & 1 deletion tests/Doctrine/Tests/ORM/Cache/DefaultQueryCacheTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ public function testGetShouldIgnoreOldQueryCacheEntryResult()
array('id'=>2, 'name' => 'Bar')
);

$entry->time = time() - 100;
$entry->time = microtime(true) - 100;

$this->region->addReturn('get', $entry);
$this->region->addReturn('get', new EntityCacheEntry(Country::CLASSNAME, $entities[0]));
Expand Down
Loading