Updated: Comment #N

Problem/Motivation

Not sure if this is a duplicate, but in #1194136-52: Re-organise core cache bins, I noticed the following cache gets:

cache: entity_base_field_definitions:node:en
cache: entity_bundle_field_definitions:node:article:en
cache_config: find:field.field.
cache_config: field.field.comment.comment_body, field.field.custom_block.body, field.field.node.body, field.field.node.comment, field.field.node.field_image, field.field.node.field_number, field.field.node.field_tags, field.field.user.user_picture

Looks like we're loading *all the fields* when loading field definitions. So that made me wonder why.

This is caused in FieldInstance::__create(), which is called from __wakeup() when unserializing the bundle field definitions, which calls
$field = field_info_field_by_id($values['field_uuid']);.

Now, that *used* to be intelligently pre-loaded in FieldInfo with the field map and everything when calling the instances through that, but that's no longer happening, as the field instances are now separately cached. So it goes through entity_load_by_properties(), which loads all entities and then filters out.

Proposed resolution

Not sure.. We could say it's something we need to fix in #2116363: Unified repository of field definitions (cache + API)?

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Title: Load field_config entity in FieldInstance::__construct() based on field name instead of UUID » FieldInstance::__construct() loads all field_config entities
yched’s picture

Not where I can really have a look atm, but if field_info_field_by_id($values['field_uuid']); is not cached, that looks wrong :-/.

Berdir’s picture

Wow, that "assign to yched" trick worked well :)

Yeah, the load by id is no longer prepared in the static cache because that would only happen after you call getInstances($entity_type, $bundle), which now nobody does anymore. At least not before getFieldDefinitions($entity_type, $bundle) is called.

yched’s picture

Priority: Normal » Major
Issue tags: +Entity Field API

Might be solved as part of the reshuffle in [#https://drupal.org/node/2116363], but bumping to at least major, this is bad...

Berdir’s picture

Yes, #2116363: Unified repository of field definitions (cache + API) does remove the uuid loading as there's no point in doing that anymore but it still means a config entity load per field instance in the field definitions list.

Maybe we can find a way to avoid the load on unserialize, because that's the real problem?

fago’s picture

Maybe we can find a way to avoid the load on unserialize, because that's the real problem?

Yeah, couldn't we go via getter method instead? Also, we could load the field storage definition via the EM instead doing a config entity load as I'd assume that's the cache which is usually warm.

Berdir’s picture

Using the field storage definitions of the entity manager sounds like a good idea, this was also improved a lot already by only doing the uuid based load only for deleted field instances in #2116363: Unified repository of field definitions (cache + API), but using the storage field definitions still means a single cache get for all fields of an entity type instead of many, a cache get that possibly need anyway.

I'm also trying to remove reasons that we need to load them at all for entities that are loaded from the entity cache once we have that. The biggest offender there right now is the language field, which we're always setting in __construct(), I hope that the introduction of the langcode entity key will help us to use the entity key cache mechanisms for this as well that we put in place for ID and stuff.

yched’s picture

Ideally, we'd remove the need to fetch the $field right from $instance __construct(), and move to lazy-loading in getField().

The loaded $field is currently used to perform validity checks and filling of a couple defaults on FieldInstanceConfig. Doing those steps right up in the case of "programmatic entity_create('field_instance)" still makes sense IMO, but that's not needed when reading an existing Instance from config or cache.

One way of differentiating those two cases is the presence of $values['uuid'] (although it might be a false positive in case of "default config import", when the yml file was just copied as-is from an active config record - we have not settled what the Config System would do in those cases : #2201501: Throw an exception if a UUID is defined in default configuration)
One other way could be to move the "post-create only" steps from __construct() to postCreate(), which possibly didn't exist back when FieldInstanceConfig was written.

Will try to work on a candidate patch for the above.
(thus, keeping the old "assigned to me" for now :-)

yched’s picture

Status: Active » Needs review
FileSize
10.77 KB

In progress, bot check.

yched’s picture

Should be ready for review.

Patch :
- Lazy-loads the FieldConfig when needed in FieldInstanceConfig::getField() rather than straight up in __construct()
- Thus, moves all internal uses of $this->field to $this->getField()
- Loads the FieldConfig from the EntityManager registry rather than reading it back from config (that code previously used field_info_field())
- Changes FieldInstanceConfig::getName() to use the internal name it already has, rather than fetching it from the Field, otherwise the $instance->getName() call in field_entity_bundle_field_info() would defeat our efforts here :-)

Status: Needs review » Needs work

The last submitted patch, 10: 2211723-FIC_construct-10.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
11.23 KB

Hm. Reroll ?

Status: Needs review » Needs work

The last submitted patch, 12: 2211723-FIC_construct-12.patch, failed testing.

yched’s picture

Hm, should have posted an interdiff :-/
The diff between #9 and #12 is mostly "read the FieldConfig from EM::getFieldStorageDefinitions()".

Will try to investigate, but might not be before a couple days.

Berdir’s picture

Status: Needs work » Needs review
FileSize
12.93 KB
4.21 KB

That error means phpunit fails/fatal errors in 90% of the cases. Run them manually to get useful errors. Just had to update the mock stuff.

The actual fatal error was because it still used the (deprecated) format_string(), changed that too and added a mising word.

Didn't profile this yet, will need to do it on a site with a bunch of fields but makes a lot of sense...

Berdir’s picture

Checked it, the load on demand is kind of an illusion when loading/rendering an entity because we do need that information (getType(), isTranslatable() and others are called) but in my case with 19 configurable fields, we're trading 19 separate config loads with information that is either a single cache get if it's not (as in my xhprof run) already loaded due to a entity query or something else.

yched’s picture

Ouch, got back to it before cheking my mail, and came to the same conclusion/fixes as #15 :-)
Thanks @Berdir !

Minor adjustements :
- I don't think the issue was format_string() in itself, but rather a silly call to it without a second argument because string has no placeholder. Palm meets forehead.
Both FieldConfig and FieldInstanceConfig have other existing calls to format_string(), so I'd rather leave the files consistent here and switch to String::format() in one pass later.
- The testCalculateDependencies() test seems to be ordered using a pattern of "use an object as mocked method return value right after defining that mocked object", so applied the same pattern for mocking EM::getFieldStorageDefinitions() (in practice : moved it up a bit).
- No need for the returnValueMap() when mocking EM::getStorage(), we only mock for the 'bundle_entity_type' param.

re #16: yeah, in practice lazy loading won't be "lazy" that often, but this might be enhanced in the future if needed :
- getType() : we do store the field_type in a FieldInstanceConfig YAML for config schema reasons, we just ditch it on load currently. We might work on removing the need to load the FieldConfig just to get the field type.
- isTranslatable() : well, that area is still in flux in #2143291: Clarify handling of field translatability :-)

Also, structuring the code like that has the other advantage of bringing us a lot closer to allowing #2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig, which I'm already working on and works great :-)

Berdir’s picture

The problem with format_string() is that it's a function and user functions don't exist in unit tests, that's why it fatal'd.

AFAIK, new code should always follow new coding standards and not use deprecated functions, the "clean up everything in a file" usually disrupts many other patches for no reason and often doesn't ever happen... but it's certainly not worth discussing/fighting about, just pointing out for the next time :)

yched’s picture

OK, makes sense. Reverted to String::format() in FieldInstanceConfig (but changing all occurrences, then, sorry, couldn't resist :-p).
I'll open a separate ticket to remove string_format() from FieldConfig as well.

yched’s picture

I'll open a separate ticket to remove string_format() from FieldConfig as well.

#2278501: Remove deprecated format_string() in FieldConfig

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Great. I only did minor changes here that yched changed again twice, so I think I'm qualified to RTBC this.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2f1e909 and pushed to 8.x. Thanks!

yched’s picture

Status: Fixed » Needs review
FileSize
658 bytes

Whoops, forgot one $this->field.

Berdir’s picture

Issue tags: +Needs tests

Hm, I guess this means we need tests for that?

yched’s picture

Yeah, added a unit test for FIC::toArray() - meant refactoring FieldInstanceConfigEntityUnitTest a bit.

Status: Needs review » Needs work

The last submitted patch, 25: 2211723-followup-24.patch, failed testing.

The last submitted patch, 25: 2211723-followup-24-test-only.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
FileSize
5.72 KB
411 bytes

Gah, somehow left one part uncommitted before rolling the patch.

Using methods rather than direct property access is better for mocks.

-test-only.patch is the same as in #25.

Berdir’s picture

  1. +++ b/core/modules/field/tests/src/FieldInstanceConfigEntityUnitTest.php
    @@ -73,27 +73,28 @@ public function setUp() {
    +        $this->field->getname() => $this->field,
    

    Super tiny nickpick: getName()

  2. +++ b/core/modules/field/tests/src/FieldInstanceConfigEntityUnitTest.php
    @@ -101,8 +102,7 @@ public function testCalculateDependencies() {
         $storage = $this->getMock('\Drupal\Core\Config\Entity\ConfigEntityStorageInterface');
    -    $storage
    -      ->expects($this->any())
    +    $storage->expects($this->any())
           ->method('load')
    

    I think this was actually correct, but doesn't really matter..

    The two are different because the second operate all on the same object while first is on $this and then returns the objects you're working with.

  3. +++ b/core/modules/field/tests/src/FieldInstanceConfigEntityUnitTest.php
    @@ -122,11 +122,42 @@ public function testCalculateDependencies() {
    +      'langcode' => 'und',
    

    Can we use the constant here?

  4. +++ b/core/modules/field/tests/src/FieldInstanceConfigEntityUnitTest.php
    @@ -122,11 +122,42 @@ public function testCalculateDependencies() {
    +      'label' => '',
    +      'description' => '', ¶
    +      'required' => FALSE,
    +      'default_value' => array(),
    +      'default_value_function' => '', ¶
    +      'settings' => array(),
    

    trailing spaces...

yched’s picture

1. Fixed
2. Are you reading the diff correctly ? This merely changes indentation to be consistent with the surrounding code.
3. Sure, I guess so, fixed
4. That's what you get from copy/pasting a var dump :-). Fixed

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
5.98 KB

2. Yep and I think the old way was "more correct" but doesn't matter.

Changed the constant to LanguageInterface by hand in the patch, I think I can still RTBC this, just wanted to avoid another re-roll cycle for that.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e0b25fc and pushed to 8.x. Thanks!

  • Commit e0b25fc on 8.x by alexpott:
    Issue #2211723 followup by yched, Berdir: FieldInstance::__construct()...

Status: Fixed » Closed (fixed)

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

yched’s picture