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.
Among other things, field_info_instances($entity_type) is called on every request right now when getFieldDefinitions() is called. This breaks the design of FieldInfo, which assumes that there are no such calls on normal requests.
Comment | File | Size | Author |
---|---|---|---|
#8 | entity-field-definition-cache-1975486-8.patch | 3.5 KB | Berdir |
#8 | entity-field-definition-cache-1975486-8-interdiff.txt | 2.17 KB | Berdir |
#1 | entity-field-definition-cache-1975486.patch | 3.39 KB | Berdir |
Comments
Comment #1
BerdirMy big demo site goes down to 220 queries from 800 with this on the frontpage.
Will likely have a bunch of fails, let's see if it can be installed like this.
Comment #2
BerdirAnd we're talking about 4.5 vs 1.95 requests per second with our without this. The effect of course is only so big on a site like this, with 1600 yml files and hundreds of fields.
Comment #3
swentel CreditAttribution: swentel commentedLooks sweet, should we cache per language also ?
Nitpick:
80 chars limit.
Comment #4
BerdirComment #5
yched CreditAttribution: yched commentedThat's definitely needed as a stopgap fix. The current field_info_instances($entity_type) call on every page simply kills #1040790: _field_info_collate_fields() memory usage.
I don't think that's enough, though. It was determined in that other issue that the best caching pattern for field definitions was "per bundle", and we went to great lengths to implement it in FieldInfo. I'd think the same reasoning applies to "EntityNG field definitions" too (all the more as they "include" Field API field definitions).
Else we end up loading way too much cache data in memory (field definitions for all node types) to display a single node.
In other words, getFieldDefinitions() should be getFieldDefinitions($bundle) IMO, and that's the case we should optimize our cache for. And getFieldDefinitionsForAllBundles(), if supported at all, should be the un-optimized case.
Doesn't prevent current patch from going as is, of course. But can we open a followup for "cache by bundle" ?
Comment #6
yched CreditAttribution: yched commentedCP
Comment #7
BerdirTagging.
Comment #8
BerdirHuh, didn't expect that to come back green ;)
Re-roll to fix the comment and use the entity_info cache tag instead of resetCache(), let's see if that works too.
@yched: At the moment, the information stored in this cache is quite small and there are use cases where it's called where the bundle is not known (e.g. in rules or so, where only the entity type is known), so I'm not sure we can get away with a per-bundle cache only, discussed this with @fago before.
Comment #9
yched CreditAttribution: yched commentedSure, there are use cases to fetch the info across bundles, Views has a typical one (collect views data). My point is that this happens below a (views) cache point, not during a regular page request.
True, "entity fields" definitions are fairly small right now, so the overhead is not too critical. Not sure it will stay that way if/when "unify Field API and Entity fields" happen, but we'll see then.
This is good for me, but I guess @fago should RTBC ?
Comment #10
BerdirAssigning to him, then.
Yes we have to see how this develops.
Comment #11
fagoThe patch looks good to go - thanks for taking care of this!
@per-bundle cache: yeah, I can imagine this makes sense on sites with lots of per-node type fields although I'm not 100% sure as we avoid replicating per-bundle-data right now, what would happen then. Howsoever, let's discuss that in its own issue.
Related, I think we really should do #1893820: Manage entity field definitions in the entity manager or something related. Feedback wanted there.
Comment #12
webchickWow, #2 is impressive! :)
Since this is performance-y, assigning to catch to eyeball first.
Comment #13
catchI nearly knocked this back for not clearing the cache anyway, but then cache tags <3.
Feels like getting this per-bundle would be better, then add the double caching in the edge cases that need to get all the field definitions, but this is a massive regression at the moment and making it hard to even see other ones, so let's get it in.
Committed/pushed to 8.x, thanks!