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
Creating entity storage became more expensive due to loading the last installed definitions from cache. This isn't about improving that (that's #3131585: Performance regression caused by using the last installed entity definitions but more specifically to avoid doing it when we don't have to.
Steps to reproduce
One simple example is \Drupal\taxonomy\TermBreadcrumbBuilder and CommentBreadcrumbBuilder. As a breadcrumb builder, we instantiate it on every request, but only applies on comment/term pages, which is relatively rare and we only need the storage then.
A less beneficial case is \Drupal\node\Plugin\views\row\Rss. While we don't need it on a regular patch just to attach the display, it will still be loaded later on, but that's still useful.
Proposed resolution
Only inject the entity type manager, get the storage only when actually required.
This patch removes 4 queries (2x entity type + field definitions for each entity type) on the frontpage with disabled render cache.
Remaining tasks
Deal with BC.
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | reroll_diff_5_19.txt | 747 bytes | Spokje |
| #19 | entity-storages-3162827-19.patch | 7.33 KB | Spokje |
Comments
Comment #2
BerdirComment #3
BerdirThis isn't limited to constructors.
Noticed taxonomy_tokens() for example, which gets the taxonomy term storage any time it is called but it's only needed for some very specific tokens.
Comment #4
BerdirAnd yet another example in \Drupal\webform\WebformEntityAccessControlHandler::checkAccess(), which gets the submission storage just in case a displayed webform has some specific, rather rare features enabled.
Comment #5
BerdirAlso including taxonomy_tokens(). I guess there are plenty more of those, but I suppose we can't fix all of them here but should focus on the obvious ones?
Comment #7
BerdirRandom ckeditor test fail.
Comment #8
claudiu.cristea@Berdir, I faced another edge case that indicates we should not instantiate the entity storage in service constructors, see #2863986-79: Allow updating modules with new service dependencies.
Comment #9
BerdirYeah, there are a number of reasons to not do this. We also need a specific workaround for entity storages in \Drupal\Core\DependencyInjection\DependencySerializationTrait, and that won't work for other handlers.
Comment #10
claudiu.cristea@Berdir, I wonder if we cannot enforce this and deprecate current storage instantiations from contrib & custom modules. Checking the service arguments will not help too much as in most of the cases the entity type manager is passed and that is legit.
Comment #11
longwaveIs it possible to abstract this away at a higher level, ie. allow EntityTypeManager::getStorage() or getHandler() to return a lazy loader/proxy object that only actually instantiates the handler when a method is called on the handler?
Comment #12
BerdirI don't think we can enforce or lazy load it. Not without API breaks. One reason is that getStorage() might return a handle with additional interfaces/methods, e.g. \Drupal\node\NodeStorageInterface and someone might have type hinted on that.
IMHO the best we can do that is document this somewhere and lead by example in core.
Tat said, I'd prefer to focus this issue on specific cases that have a proven (performance) benefit, to make some progress on this, we could do follow-ups to convert more. There are dozens of such calls in core alone and some of them will be super annoying to deal with for BC and so on, for example \Drupal\Core\Entity\EntityListBuilder::__construct() with all its subclasses.
Comment #13
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedOuch, encouraging injecting the entity type manager everywhere that only a storage is required exposes a lot more API to classes. I've always encouraged folks to inject the bare minimum required into each class and that's often a storage handler for a specific entity type.
Could accessing the protected
$this->entityTypevariable instead be moved to a method which lazy loaded the definition?Comment #14
Berdir> I've always encouraged folks to inject the bare minimum required into each class and that's often a storage handler for a specific entity type.
I do agree, as long as it's about injecting _services_. But entity handlers, config objects and and also e.g. loggers should imho not be assigned to properties in the constructor.
And you need to "know" just as many classes/API's, unless you're "cheating" with the create() method. An entity handler isn't a service, you couldn't inject that in the constructor if it would be a service, that only works thanks to our create() workaround.
I don't really get the $this->entityType comment, but you could have a nodeStorage()->load() method to simplify the code if it's used multiple times, sure. But all examples here have just a single call.
Comment #15
andypost@Berdir what you think about usage handlers in route subscribers? For example "alter" ones... it looks totally fine to get storage handler there in constructor (except require to define exceptions) or I missing something?
Comment #17
daffie CreditAttribution: daffie commentedComment #18
SpokjeReroll of patch #5 against
9.2.xComment #19
SpokjeReroll of patch #5 against
9.2.x, take 2...Comment #20
SpokjeWhat a difference a
dayfull stop makes...Comment #21
daffie CreditAttribution: daffie commentedAll the changes in this patch are a couple of nice little performance gains.
Nowhere is there a change in functionality.
The method
Drupal\node\Plugin\views\row\Rss::__construct()can be removed, because it has the same parameters as its parent method. Therefore no API change.I see no need to add testing for this, we are not changing any functionality.
The patch delays the loading of the entity storage untill it is really needed. And by doing so, it was loaded when it was not needed.
For me it is RTBC.
Comment #23
catchThere are a couple of removed protected properties in the breadcrumb builders, but those shouldn't be extended so I think it is fine to do that (and without a change record) as long as this only lands in 9.2.x.
It'd be worth opening a follow-up to look at applying this pattern more elsewhere in core and also for documenting it better, but it makes sense to get the performance improvement in the critical path done here first.
Committed 6241c57 and pushed to 9.2.x. Thanks!
Comment #25
joachim CreditAttribution: joachim at TrainingCloud commented> IMHO the best we can do that is document this somewhere and lead by example in core.
The patch for this issue didn't add any documentation. Follow-up?
Comment #26
joachim CreditAttribution: joachim as a volunteer commentedFiled #3197773: add documentation about not injecting entity storage handlers.
I thought we had a documentation gate on issues?