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
Again quoting myself:
There doesn't seem to be any kind of static caching for this (e.g. in \Drupal\Core\Entity\ContentEntityBase::isLatestRevision or in the storage), but you can see that quickedit_preprocess_field() and quickedit_entity_view_alter() is calling it not just for each entity but again for each *field* being rendered for that entity. So we're not just doing this call once but maybe 10 times (or 30, if you have a lot of fields being displayed) on a site with quickedit enabled.
Additionally, I noticed that there are ~4 requests even without quickedit due to the local tasks access checking that goes through the \Drupal\Core\ParamConverter\EntityConverter::convert() and they all separately call that method.
Proposed resolution
Add static caching to getLatestTranslationAffectedRevisionId() and getLatestRevisionId()
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | latest-revision-id-cache-2983887-8.patch | 5.38 KB | Berdir |
Comments
Comment #2
BerdirI'm expecting this to fail, e.g. in tests that save entities.
But, in my local testing, this saves about 12 queries. And it could be a lot more if you have more fields due to quickedit.
Comment #4
BerdirThis might fix the tests.
Comment #6
BerdirAnd this fixes the remaining fails. Only two tests needed a manual cache clear, due to the usual form submissions and then checking in the test.
Comment #7
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedIt seems that the patch from #2983837: Optimize EntityOperations::entityView() crept in here, let's remove it :)
Comment #8
BerdirRemoved that.
Comment #10
Wim LeersThis seems like a simple enough patch for a significant performance improvement.
Ideally we'd have profiling for concrete numbers, but + #2950869: Entity queries querying the latest revision very slow with lots of revisions indicating that even a single execution of the query can be very slow means that this is guaranteed to aid performance significantly on sites with enough revisions :)
Comment #11
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedIt would be great to get #2934026: Deprecate isLatestRevision, getLatestRevision, getLatestRevisionId in the ModerationInformation service done after this, since the methods in
ModerationInformationwont benefit from this static cache.Comment #14
catchEven for the fastest database query this would be worth doing so this doesn't really need profiling, slow queries generally only show up with lots of content.
Committed/pushed to 8.7.x and cherry-picked to 8.6.x. Thanks!
Comment #15
Wim LeersComment #17
tedbowJust wanted to note that I had a test in #2937199: Track Layout override revisions on entities which support revisioning that was passing fail because of this change.
Basically I had to make sure i called
$node_storage->resetCache([$revision_node->id()]);the second time I called$node_storage->getLatestRevisionId($revision_node->id()).I am wondering if this could be a BC break because if a class gets the storage handler for an entity type \Drupal\Core\Entity\EntityTypeManager::getHandler() is going to give me an existing instance if another class has asked for the storage handler. So if another class called
getLatestRevisionId()and then saved the entity. The next class that gets the storage handler will have to callresetCache()before the first time it callsgetLatestRevisionId().Is that correct?
Should we unset the
latestRevisionIdsindoPostSave()?Comment #18
BerdirYou had to do that in a functional test, just like this issue had to in two instances.
Saving an entity already calls resetCache() and invalidates the revision ID for the same request. The problem with functional tests is that their in-memory caches can't get notified about changes done by web requests and get out of sync. We already invalidate a lot of static caches after every POST request but not everything.
That's a common problem for functional tests, 99% of the time that resetCache() is needed is inside tests.