The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
Spin-off from #3537863: Optimize field module's hook_entity_bundle_info() implementation.
If you have a lot of bundles, which is often the case for sites with webform and/or paragraphs enabled, then the per-bundle cache on EntityFieldManager::getFieldDefinitions() can lead to hundreds of cache sets on cold cache requests.
With webform especially the number of fields per bundle is low (actually zero), so it might be possible to do something like cache the bundle definitions for all bundles of an entity type in one entry to reduce i/o. This could mean more data to load when it's requested, but that will usually happen on 'cool' caches (e.g. when dynamic page cache is empty) and might not be very measurable in that context, whereas the impact on fully site-blocking cold caches would be quite big.
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|
Issue fork drupal-3537962
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
catch#3564973: Optimize field views data building adds another usage of this API (in a situation it will be called anyway) vs. doing raw FieldConfig loads. Doesn't really change anything about this issue, but the two will be cumulative.
Comment #3
catchHad a go at this. Because the field building itself is very bundle specific, it's easiest to get a list of bundles, build all the info, set the cache in just that one method. Also retained the individual cache items for each bundle - just building and setting them all at once. We could potentially move the building out to its own method although not sure it would necessarily make it more readable.
Saves about 10 cache gets and sets with Umami. If you have a site with 100 webforms, it will save 99 cache sets and gets. If a site has 50 paragraphs, 49 cache sets and gets etc. etc.
Comment #5
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #6
catchLots of test failures here.
Pretty sure
EntityDefinitionUpdateMultipleTypesTesthas broken logic - it's creating bundles on the entity_test_update entity type, this entity type afaict has no bundle info at all, so only ever has one bundle. However because of the way the entity type is implemented, it can have 'bundle fields' for non-existing bundles and EntityFieldManager doesn't notice. Once we loop over actually existing bundles and base the data on those, suddenly the missing bundle is actually missing.I added support for non-existent bundles to the method to see if that gets things back to green.
Comment #7
catchOK that works for bc.
I think we go ahead with more or less this since it maintains the existing behavior, then open a follow-up to deprecate support for non-existing fields and clean up all the tests that depend on it, but see what other people think.
Comment #8
catchOpened #3565232: EntityFieldManager::getFieldDefinitions() provides results for bundles that don't exist and added a @todo here.
Comment #9
berdirThis looks good and the test adjustments speak for themself. That said, similar to what we discussed in the field info hook issues, it kind of feels like we're optimizing the wrong thing here. Ultimately, I think we should avoid building bundle field definitions in loops instead of optimizing how we built them. Then again, with the field map issue that I haven't look at closely yet, this might remain like this. If that's the long-term fact, then we might want to consider to adjust the build (and the hooks) to build for all bundles. But I don't want to think about BC for that.
Comment #10
catchYes I think to get rid of all the loops it we'd need to add an alternative hook to hook_entity_bundle_field_info(), which applies to all bundles of the entity type, and deprecate hook_entity_bundle_field_info(). If we're able to take the field map issue as far as I think we can, then that might provide a better basis to work towards that point - still a lot of churn though.
Comment #11
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #12
catchRebased.
Comment #14
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #15
catchRebased - only performance tests needed updating so moving back to RTBC.
Comment #16
berdirPerformance tests on this looks great, this saves 43 separate cache reads and writes in our install profile.
It does make an assumption that if one bundle is a cache miss, then all of them are. True in case of cold caches and what we test, but if for example redis decides to drop a single cache entry, we'd likely do a lot more work than necessary. I don't know how often that is and if it's necessary to account for that. They should be read pretty frequently so
Creating two redis issues based on this.
a) Redis currently doesn't support an actual setMultiple(), I've only recently with the refactoring around the 2.x release fully understood what the differences between multi and pipeline are, and I think it might be possible to re-implement setMultiple() to use a pipeline across multiple writes: #3570199: Allow to execute multiple cache sets in a single pipeline
b) to prevent caches like these from being evicted, it might be useful to skip setting a TTL for some bins, to make a scenario like this less likely to happen: #3570205: Allow to set no TTL on items in certain bins
Comment #17
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #18
godotislateMerge conflict in performance tests.
Comment #19
catchRebased.
Comment #20
alexpottCommitted and pushed 31c0baa3c7d to main and 980839d4a41 to 11.x. Thanks!