Avoid wasting Opcache memory with Paginator queries

This PR prevents the Paginator from causing OpCache "wasted memory" to increase _on every request_ when used with Symfony's `PhpFilesAdapter` as the cache implementation for the query cache.

Depending on configured thresholds, wasted memory this will either cause periodic opcache restarts or running out of memory and not being able to cache additional scripts ([Details](https://tideways.com/profiler/blog/fine-tune-your-opcache-configuration-to-avoid-caching-suprises)).

Fixes doctrine#9917, closes doctrine#10095.

There is a long story (doctrine#7820, doctrine#7821, doctrine#7837, doctrine#7865) behind how the Paginator can take care of DBAL type conversions when creating the pagination query. This conversion has to transform identifier values before they will be used as a query parameter, so it has to happen every time the Paginator is used.

For reasons, this conversion happens inside `WhereInWalker`. Tree walkers like this are used only during the DQL parsing/AST processing steps. Having a DQL query in the query cache short-cuts this step by fetching the parsing/processing result from the cache.

So, to make sure the conversion happens also with the query cache being enabled, this line

https://github.com/doctrine/orm/blob/1753d035005c1125c9fb4855c3fa629341e5734d/lib/Doctrine/ORM/Tools/Pagination/Paginator.php#L165

was added in doctrine#7837. It causes `\Doctrine\ORM\Query::parse()` to re-parse the query every time, but will also put the result into the query cache afterwards.

At this point, the setup described in doctrine#9917 – which, to my knowledge, is the default in Symfony + DoctrineBundle projects – will ultimately bring us to this code:

https://github.com/symfony/symfony/blob/4b3391725f2fc4a072e776974f00a992cbc70515/src/Symfony/Component/Cache/Adapter/PhpFilesAdapter.php#L248-L249

When writing a cache item with an already existing key, the driver has to make sure the opcache will honor the changed PHP file. This is what causes _wasted memory_ to increase.

Instead of using `\Doctrine\ORM\Query::expireQueryCache()`, which will force `\Doctrine\ORM\Query::parse()` to parse the query again before putting it into the cache, use `\Doctrine\ORM\Query::useQueryCache(false)`. The subtle difference is the latter will not place the processed query in the cache in the first place.

A test case is added to check that repeated use of the paginator does not call the cache to update existing keys. That should suffice to make sure we're not running into the issue, while at the same time not complicating tests by using the `PhpFilesAdapter` directly.

Note that in order to observe the described issue in tests, you will need to use the `PhpFilesDriver` and also make sure that OpCache is enabled and also activated for `php-cli` (which is running the unit tests).

This particular subquery generated/used by the Paginator is not put into the query cache. The DQL parsing/to-SQL conversion has to happen _every time_ the Paginator is used.

This, however, was already the case before this PR. In other words, this PR only changes that we do not store/update the cached result every time, but instead completely omit caching the query.

lib/Doctrine/ORM/Tools/Pagination/Paginator.php

@@ -162,7 +162,7 @@ public function getIterator()
  $whereInQuery->setFirstResult(0)->setMaxResults(null);
  $whereInQuery->setParameter(WhereInWalker::PAGINATOR_ID_ALIAS, $ids);
  $whereInQuery->setCacheable($this->query->isCacheable());
- $whereInQuery->expireQueryCache();
+ $whereInQuery->useQueryCache(false);
 
  $result = $whereInQuery->getResult($this->query->getHydrationMode());
  } else {

tests/Doctrine/Tests/ORM/Functional/Ticket/GH7820Test.php

@@ -13,6 +13,9 @@
 use Doctrine\ORM\Mapping\Id;
 use Doctrine\ORM\Tools\Pagination\Paginator;
 use Doctrine\Tests\OrmFunctionalTestCase;
+use PHPUnit\Framework\Assert;
+use Psr\Cache\CacheItemInterface;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
 
 use function array_map;
 use function is_string;
@@ -69,51 +72,56 @@ protected function setUp(): void
 
  public function testWillFindSongsInPaginator(): void
  {
- $query = $this->_em->getRepository(GH7820Line::class)
- ->createQueryBuilder('l')
- ->orderBy('l.lineNumber', Criteria::ASC)
- ->setMaxResults(100);
+ $lines = $this->fetchSongLinesWithPaginator();
 
- self::assertSame(
- self::SONG,
- array_map(static function (GH7820Line $line): string {
- return $line->toString();
- }, iterator_to_array(new Paginator($query)))
- );
+ self::assertSame(self::SONG, $lines);
  }
 
  /** @group GH7837 */
  public function testWillFindSongsInPaginatorEvenWithCachedQueryParsing(): void
  {
+ // Enable the query cache
  $this->_em->getConfiguration()
  ->getQueryCache()
  ->clear();
 
- $query = $this->_em->getRepository(GH7820Line::class)
- ->createQueryBuilder('l')
- ->orderBy('l.lineNumber', Criteria::ASC)
- ->setMaxResults(100);
+ // Fetch song lines with the paginator, also priming the query cache
+ $lines = $this->fetchSongLinesWithPaginator();
+ self::assertSame(self::SONG, $lines, 'Expected to return expected data before query cache is populated with DQL -> SQL translation. Were SQL parameters translated?');
 
- self::assertSame(
- self::SONG,
- array_map(static function (GH7820Line $line): string {
- return $line->toString();
- }, iterator_to_array(new Paginator($query))),
- 'Expected to return expected data before query cache is populated with DQL -> SQL translation. Were SQL parameters translated?'
- );
+ // Fetch song lines again
+ $lines = $this->fetchSongLinesWithPaginator();
+ self::assertSame(self::SONG, $lines, 'Expected to return expected data even when DQL -> SQL translation is present in cache. Were SQL parameters translated again?');
+ }
+
+ public function testPaginatorDoesNotForceCacheToUpdateEntries(): void
+ {
+ $this->_em->getConfiguration()->setQueryCache(new class extends ArrayAdapter {
+ public function save(CacheItemInterface $item): bool
+ {
+ Assert::assertFalse($this->hasItem($item->getKey()), 'The cache should not have to overwrite the entry');
+
+ return parent::save($item);
+ }
+ });
 
+ // "Prime" the cache (in fact, that should not even happen)
+ $this->fetchSongLinesWithPaginator();
+
+ // Make sure we can query again without overwriting the cache
+ $this->fetchSongLinesWithPaginator();
+ }
+
+ private function fetchSongLinesWithPaginator(): array
+ {
  $query = $this->_em->getRepository(GH7820Line::class)
  ->createQueryBuilder('l')
  ->orderBy('l.lineNumber', Criteria::ASC)
  ->setMaxResults(100);
 
- self::assertSame(
- self::SONG,
- array_map(static function (GH7820Line $line): string {
- return $line->toString();
- }, iterator_to_array(new Paginator($query))),
- 'Expected to return expected data even when DQL -> SQL translation is present in cache. Were SQL parameters translated again?'
- );
+ return array_map(static function (GH7820Line $line): string {
+ return $line->toString();
+ }, iterator_to_array(new Paginator($query)));
  }
 }