FieldInfo::getBundleInstances() caches Field & FieldInstance objects for a given [$entity_type, $bundle] in one single cached array (with separate 'fields' & 'instances' entries, but still in the same array).

That's a problem because:
- unserializing the array means unserializing all Field and FieldInstance structures
-- unserializing a FieldInstance currently triggers fetching the corresponding Field definition with FieldInfo::getField().
--- FieldInfo::getField() doesn't find the Field in its static cache, because we are in the process of unserializing them. So it fetches it from config()

So FieldInfo::getBundleInstances() triggers one extra config() read per instance for nothing.

Note:
#2073297: Remove cache of Field / FieldInstance objects has a broader proposal to reconsider Field/FieldInfo caching. But that would need to be benchmarked, and those benchmarks should happen against a state of HEAD that is not artificially slow because it's broken.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Status: Active » Needs review
FileSize
3.62 KB

Attached patch fixes this by caching fields and instances in separate cache entries, that can be unserialized separately.

This might not be the final state for this "caching" code in D8, I'd like to try avoiding serialization of Field & FieldInstance config entities and cache only config ids, relying on the config() cache for the objects themselves instead. But that would need to be benchmarked, and those benchmark should happen against a state of HEAD that is not artificially slow because it's broken.

Berdir’s picture

In response to the question asked in #2015697: Convert field type to typed data plugin for file and image modules, nope, this issue does not fix #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0, still getting the same failures and (and a apache segfault).

yched’s picture

Bench on a moderately complex example:
- two node types, each with: 5 shared fields + 5 fields of their own
- page listing 5 nodes of each type

HEAD: 238.923 ms
Patch: 230.992 ms -> -3.3%

Anonymous’s picture

looks like a good improvement from a simple cache addition to me.

can you open a follow up for the "just cache config ids" bit?

yched’s picture

Yup, created #2073297: Remove cache of Field / FieldInstance objects, I'll update the OP.
(note it's not really a followup, it's a task in itself, and this issue here is more like a hotfix for something that's currently broken)

yched’s picture

Issue tags: +Performance, +Field API

tagging

yched’s picture

Typo in code comment.

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e2e8b97 and pushed to 8.x. Thanks!

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

Anonymous’s picture