Problem/Motivation

Split from #2345611: [pp-1] Load user entity in Cookie AuthenticationProvider instead of using manual queries

See also #3572625: Calling $entity->getTranslatedField() results in an entity-sized memory leak and #3573982: Circular references / memory leaks in ItemList.

Creating FieldItemList objects - e.g. what happens when you call $entity->get() is relativel expensive.

In #2345611: [pp-1] Load user entity in Cookie AuthenticationProvider instead of using manual queries adding a FieldItemList creation on requests where there previously wasn't one adds a lot of overhead.

In the other two issues, creating lots of FieldItemList objects, e.g. by iterating over a lot of entities and retrieving field values from them, it also results in a memory leak due to circular references.

For #2345611: [pp-1] Load user entity in Cookie AuthenticationProvider instead of using manual queries we can't really avoid the service instantiation/object creation overhead without actually avoiding it.

For the memory leak we should try to actually fix that, but the method added here does offer a way to workaround that too, and as importantly, it should give us a 'memory baseline' to compare against when the other issues are fixed.

Steps to reproduce

Proposed resolution

Add a new EntityFieldValueTrait with a protected getFieldValue() method. This covers the user module use case, and allow experimentation in contrib.

We don't want to make this a public API method anywhere because it doesn't behave as consistently as ::get(). It's also possible that some use-cases for the method could be resolved by #3572625: Calling $entity->getTranslatedField() results in an entity-sized memory leak and similar issues.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3574012

Command icon 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:

Comments

catch created an issue. See original summary.

catch’s picture

Status: Active » Needs review
catch’s picture

Issue tags: +Needs tests

This needs at least some basic test coverage - load an entity, call this method to get a field value, then probably get the same thing via the regular field items and compare the values etc. but moving to needs review for the API.

Reminder we have a specific core use case in #2345611: [pp-1] Load user entity in Cookie AuthenticationProvider instead of using manual queries where as far as we know this is non-optional to enable that issue to happen without a performance regression. Until we fix the memory leaks in the field/typed data system it will also have some utility in contrib/custom code dealing with large numbers of entities but hopefully we can fix the memory leaks as well, since it'll be a pretty fragile solution to that problem (any call to $entity->get() will cause the memory leak again).

berdir’s picture

I think there's quite some overlap between this and #3565858: Provide non-magic shortcuts for getting first field properties.

My concern is that a method that bypasses field objects opens possibilities of inconsistent behavior and some things like computed properties or fields won't work. It's inconsistent and magic trickery to work around performance issues and might make DX even worse.

I originally came up with this, but was never happy about it nor convinced that we should do this, which is one reason why I didn't push it further.

catch’s picture

So I somewhat agree with #5 even though I spun this out of the other issue, but I was pretty shocked by the scale of the memory leak in typed data and this is out of desperation.

The page I'm testing against loads 500 entities + some references from those entities, and then gets some field values from them. It was taking 220mb of memory despite the entity LRU memory cache. With this MR and converting the ::get() calls to ::getFieldValue() the memory usage goes down to 60mb. So that is 170mb spent creating field item lists that can't be reclaimed. Not everything could be converted, that is probably another 10mb of memory leak according to some additional testing I did due to the remaining field item list creation.

That page is quite extreme but I think we could easily be seeing single or even double digit mb memory leaks on 'normal' pages with a decent number of entities (views listings, comments, paragraphs, LB + content blocks etc.).

My concern is that a method that bypasses field objects opens possibilities of inconsistent behavior and some things like computed properties or fields won't work.

This is true, we can probably throw a bad argument exception for computed fields quite easily. For properties it might be possible to get the typed data definition for the field and throw a bad argument exception in that case too, but in such a way we don't reference the type data and re-introduce the memory leak.

I'm hoping to keep going on the other two issues that should actually fix the memory leak - there is a probably viable fix for about half of it, found what I think is the root cause of the other half yesterday but don't have a viable fix for that yet.

catch’s picture

One possible option here. Instead of adding this generically to the entity API, we could add it to a trait.

Then user module could use it in #2345611: [pp-1] Load user entity in Cookie AuthenticationProvider instead of using manual queries, and contrib/custom modules could use the trait if they want to, but it would make the entire thing opt-in.

User module could also even override the trait method to implement an allow list for specific field names too.

catch’s picture

Status: Needs review » Needs work

Discussed a bit with @berdir in slack, we came up with something like this:

1. Move it to a trait
2. Make it a protected @internal method on that trait.
3. User module can implement specific getter methods relying on the trait, so that it's not exposed as a public API anywhere else at all.
4. Custom entities could still use the trait - import the method as an alias, have a public method that calls it, that sort of thing.

catch’s picture

Status: Needs work » Needs review

MR now does what's in #8. The user entity and delete form changes are verbatim from #2345611: [pp-1] Load user entity in Cookie AuthenticationProvider instead of using manual queries, which means that issue would then only be about changing the cookie authentication handler.

By implementing this on the user entity we have some implicit test coverage now too.

catch’s picture

Title: Add getFieldValue() method to ContentEntityBase » Add a getFieldValue() method to bypass typed data overhead for specific use cases
andypost’s picture

smustgrave’s picture

Status: Needs review » Needs work

Sorry to be that guy can update the summary with proposed solution since it's about adding a new trait?

smustgrave’s picture

btw +1 for this always hate having to do get('field')->value

catch’s picture

Issue summary: View changes
catch’s picture

Status: Needs work » Needs review

Updated the issue summary.

smustgrave’s picture

Know it's tagged for tests but User.php using it and user tests passing does that count? Or would it be good to have some kind of test for different field types?

andypost’s picture

Issue tags: -Needs tests

I bet this coverage is enough

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Test coverage looks great to me

godotislate’s picture

Status: Reviewed & tested by the community » Needs work

One open comment on the MR from alexpott and I added a question on the MR as well.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Applied Alex's suggestion, and replied to the MR comment which I'm ambivalent about. Moving back to RTBC because that might mean no further changes, but also don't really mind if we eat the exception early either just not sure it's necessary here.

alexpott’s picture

Version: main » 11.x-dev
Category: Task » Feature request
Status: Reviewed & tested by the community » Fixed

Committed 37a345a and pushed to main. Thanks!
Committed 643fb00 and pushed to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 643fb000 on 11.x
    feat: #3574012 Add a getFieldValue() method to bypass typed data...

  • alexpott committed 37a345a2 on main
    feat: #3574012 Add a getFieldValue() method to bypass typed data...
alexpott’s picture

Status: Fixed » Needs work
Issue tags: +Needs change record

I just merged this but realised we should have a CR to tell people about it after the fact. Can someone add one - thanks! Can be set to fixed once it it exists.

berdir’s picture

The trait is currently marked as @internal. I'm unsure if we should have a CR about that?

Maybe we can have a follow-up to decide if we want to make it non-internal and then tell the world about it?

alexpott’s picture

Status: Needs work » Fixed
Issue tags: -Needs change record

@berdir thats a good call. Let's decide whether to announce this in a follow-up

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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