Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
On admin user the frontpage has a couple of entity queries: (~100ms is the entire request in that example.)
| Function Name | Calls | Calls% | Incl. Wall Time (microsec) |
IWall% | Incl. MemUse (bytes) |
IMemUse% | Incl. PeakMemUse (bytes) |
IPeakMemUse% |
|---|---|---|---|---|---|---|---|---|
| Current Function | ||||||||
| Drupal\Core\Entity\Query\Sql\Query::execute | 3 | 12.0% | 10,008 | 9.2% | 2,209,344 | 12.4% | 2,242,776 | 12.5% |
| Exclusive Metrics for Current Function | 23 | 0.2% | 4,352 | 0.2% | 1,072 | 0.0% | ||
| Parent functions | ||||||||
| Drupal\comment\CommentManager::getCountNewComments | 2 | 66.7% | 5,076 | 50.7% | 724,328 | 32.8% | 735,008 | 32.8% |
| Drupal\Core\Entity\EntityStorageBase::loadByProperties | 1 | 33.3% | 4,932 | 49.3% | 1,485,016 | 67.2% | 1,507,768 | 67.2% |
| Child functions | ||||||||
| Drupal\Core\Entity\Query\Sql\Query::compile | 3 | 20.0% | 8,177 | 81.7% | 2,113,360 | 95.7% | 2,212,448 | 98.6% |
| Drupal\Core\Entity\Query\Sql\Query::result | 3 | 20.0% | 1,573 | 15.7% | 49,184 | 2.2% | 8,040 | 0.4% |
| Drupal\Core\Entity\Query\Sql\Query::prepare | 3 | 20.0% | 183 | 1.8% | 29,704 | 1.3% | 21,216 | 0.9% |
| Drupal\Core\Entity\Query\Sql\Query::finish | 3 | 20.0% | 30 | 0.3% | 8,928 | 0.4% | 0 | 0.0% |
| Drupal\Core\Entity\Query\Sql\Query::addSort | 3 | 20.0% | 22 | 0.2% | 3,816 | 0.2% | 0 | 0.0% |
which are not necessarily cheap to execute.
One part of the chain of functions is
| Function Name | Calls | Calls% | Incl. Wall Time (microsec) |
IWall% | Incl. MemUse (bytes) |
IMemUse% | Incl. PeakMemUse (bytes) |
IPeakMemUse% |
|---|---|---|---|---|---|---|---|---|
| Current Function | ||||||||
| Drupal\Core\Entity\Query\Sql\Tables::getTableMapping | 20 | 18.7% | 5,063 | 4.7% | 1,688,928 | 9.5% | 1,643,440 | 9.1% |
| Exclusive Metrics for Current Function | 87 | 1.7% | 3,096 | 0.2% | 2,096 | 0.1% | ||
| Parent function | ||||||||
| Drupal\Core\Entity\Query\Sql\Tables::addField | 20 | 100.0% | 5,063 | 100.0% | 1,688,928 | 100.0% | 1,643,440 | 100.0% |
| Child functions | ||||||||
| Drupal\Core\Entity\Sql\DefaultTableMapping::getAllColumns | 20 | 25.0% | 4,941 | 97.6% | 1,648,800 | 97.6% | 1,634,096 | 99.4% |
| Drupal\Core\Entity\EntityManager::getStorage | 20 | 25.0% | 20 | 0.4% | 696 | 0.0% | 624 | 0.0% |
| array_flip | 20 | 25.0% | 15 | 0.3% | 35,624 | 2.1% | 6,080 | 0.4% |
| Drupal\Core\Entity\Sql\SqlContentEntityStorage::getTableMapping | 20 | 25.0% | 0 | 0.0% | 712 | 0.0% | 544 | 0.0% |
which you can see, is not really cheap, but if you think about it, could be actually cached. The table mapping of an entity type should really not change between requests.
Proposed resolution
Maybe think about better ideas how to optimize it.
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | reroll_diff_25-27.txt | 3.02 KB | Akram Khan |
| #27 | 2531932-27.patch | 3.76 KB | Akram Khan |
| #25 | reroll_diff_23-25.txt | 9.24 KB | Medha Kumari |
| #25 | 2531932-25.patch | 3.93 KB | Medha Kumari |
| #23 | reroll_diff_22-23.txt | 6.3 KB | Akram Khan |
Comments
Comment #1
dawehnerAdding the plach tag.
Comment #2
BerdirThe loadByProperties() is the block repository, right?
We have #2479459: BlockRepository::getVisibleBlocksPerRegion() does an uncached entity query on every request for that.
And the comment count is because node links are a placeholder and need to be rebuilt on every page.
I started testing to remove that on #2530846: Fix and improve comment cache tag usage, will possibly open a separate issue for that part instead.
So I think we can avoid those specific queries on cached pages but yes, we can definitely look into improving the queries themself too. I also noticed that the query we actually build seems to join the same table for some conditions, which also makes the actual query slower.
Comment #3
dawehnerNo its not, these are the entity query run by shortcut.module and comment manager. The slowness I want to show here is
Drupal\Core\Entity\Query\Sql\Tables::getTableMapping, an inclusive walltime of 4.7% just for that.Comment #4
claudiu.cristeaLet's see. The biggest part here is cache invalidation.
@dawehner, can you benchmark this?
Comment #6
claudiu.cristeaChecking now if the
cache.defaultservice exists in the container because\Drupal\Core\Database\Driver\mysql\Schema& friends are called also during the install process when there's no full container. But, in that moment there's also no cache to clear so we can avoid the cache clear for that cases.@dawehner, I tried first to test this with
\Drupal::hasService('cache.default')but\Drupal::hasService('cache.default')always returns TRUE, even when installing. Is this a bug?Comment #20
larowlanComment #22
Arti Anil Pattewar CreditAttribution: Arti Anil Pattewar at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch #6 against D9.3.x as suggested in #20.
Comment #23
Akram KhanFixed some syntax and statement that are unused and Fixed CCF #22
Comment #24
Akram KhanComment #25
Medha KumariRerolled the patch #23 in Drupal 9.5.x.
Comment #26
SpokjeNeeds some PHPCS fixes.
Comment #27
Akram KhanAddress #26 and Fixed CCF #25
Comment #28
Akram KhanComment #29
joachim CreditAttribution: joachim as a volunteer commentedNeeds an IS update. The problem needs to be explained a bit better, and the proposed resolution explained.
Comment #30
phenaproximaPHP 7.1 allows us to catch multiple exception types at once, and PHP 8 allows for non-capturing catches. So, assuming this won't be backported to Drupal 9, this could be:
If this will be backported, then:
This seems a bit dangerous, because it writes a license for subclasses to NOT implement this method, under the mistaken assumption that it will be handled by the parent class.
I would suggest something like this instead:
Although that would be a backwards compatibility break, so...I guess dunno what to do here, exactly.
Comment #31
catchThe reroll in #27 is missing hunks of the patch that actually add the caching. #6 seems to be the last version of the patch with those changes.
Comment #32
Gunjan Rao Naik CreditAttribution: Gunjan Rao Naik commentedAdded patch against #27 in 10.1.x version
Comment #33
Gunjan Rao Naik CreditAttribution: Gunjan Rao Naik commentedComment #34
catch#27 is an incomplete patch, any further work here needs to start with a re-roll of #6 and ignore all the patches in-between.