Avoid wasting Opcache memory with Paginator queries#10434
Merged
derrabus merged 1 commit intodoctrine:2.14.xfrom Jan 23, 2023
Merged
Avoid wasting Opcache memory with Paginator queries#10434derrabus merged 1 commit intodoctrine:2.14.xfrom
derrabus merged 1 commit intodoctrine:2.14.xfrom
Conversation
fc7afd1 to
9dd4f24
Compare
This was referenced Jan 21, 2023
Contributor
Author
|
@icedevelopment since you reported #9917, I'd like to ask you to try this one-line change and report if it fixes the issue for you. |
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.
9dd4f24 to
df60053
Compare
derrabus
approved these changes
Jan 23, 2023
Member
|
Nice catch @mpdude! |
Contributor
Author
|
Thank you @derrabus for the swift review and merge! |
Just tested this and it does fix it! 🎉 @derrabus any idea of the approximate release date of 2.14.2? |
Member
|
No. Use |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR prevents the Paginator from causing OpCache "wasted memory" to increase on every request when used with Symfony's
PhpFilesAdapteras 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).
Fixes #9917, closes #10095.
Problem analysis
There is a long story (#7820, #7821, #7837, #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
orm/lib/Doctrine/ORM/Tools/Pagination/Paginator.php
Line 165 in 1753d03
was added in #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 #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.
Suggested solution
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
PhpFilesAdapterdirectly.Note that in order to observe the described issue in tests, you will need to use the
PhpFilesDriverand also make sure that OpCache is enabled and also activated forphp-cli(which is running the unit tests).Performance remark
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.