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.

CommentFileSizeAuthor
#8 entity-field-definition-cache-1975486-8.patch3.5 KBBerdir
#8 entity-field-definition-cache-1975486-8-interdiff.txt2.17 KBBerdir
#1 entity-field-definition-cache-1975486.patch3.39 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
entity-field-definition-cache-1975486.patch3.39 KB

My 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.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Priority: Normal » Major

And 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.

swentel’s picture

swentel CreditAttribution: swentel commented

Looks sweet, should we cache per language also ?

Nitpick:

+++ b/core/lib/Drupal/Core/Entity/DatabaseStorageController.phpundefined
@@ -677,32 +678,38 @@ protected function invokeHook($hook, EntityInterface $entity) {
+          // An array keyed by bundle name containing the optional fields added by

80 chars limit.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
yched’s picture

yched CreditAttribution: yched commented
Status: Needs review » Active

That'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" ?

yched’s picture

yched CreditAttribution: yched commented
Status: Active » Needs review

CP

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: +Performance, +Entity Field API

Tagging.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
entity-field-definition-cache-1975486-8-interdiff.txt2.17 KB
entity-field-definition-cache-1975486-8.patch3.5 KB

Huh, 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.

yched’s picture

yched CreditAttribution: yched commented

Sure, 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 ?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Assigned: Unassigned » fago

Assigning to him, then.

Yes we have to see how this develops.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Status: Needs review » Reviewed & tested by the community

The 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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: fago » catch

Wow, #2 is impressive! :)

Since this is performance-y, assigning to catch to eyeball first.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

I 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!

Automatically closed -- issue fixed for 2 weeks with no activity.