Calling $entity->getTranslatedField() results in an entity-sized memory leak

Haven't 100% tracked down the memory leak, at least nowhere near a point to try to actually fix it vs. the workaround here but I think it is probably something like this:

- When an entity is loaded, we populated $entity->values with all the field values.

- When you request $entity->get($field_name)->value; it's only then that a FieldItemList + FieldItem object are created.

- The list is set back on the entity as a kind of 'static cache' so that it's not instantiated again when it's requested.

- the item list class has as $this->parent property via typed data, which is a reference.

So the fields have the entity as a property, and the entity as the fields as a property - creates a circular reference. This circular reference may prevent PHP from garbage collection when an entity is removed from the static cache.

We don't always access field values like this after entities are loaded, especially when render caching etc. is happening, but in cases where lots of field values are access this is very heavy, both on entity cache hits and misses.

What we might be able to do, is not set the field item back onto the entity, and live with creating it every time if the same field on the same entity is accessed multiple times. That would break the circularity of the reference, it's a bit similar to the approach used here.

If $entity->set has been used, we'd need to use whatever that value is instead, but that happens much less frequently and usually before saving. I'll try to take a look at this, if so it could at least start out as an alternative MR on this issue.

So the original approach here taken from #2345611: [pp-1] Load user entity in Cookie AuthenticationProvider instead of using manual queries adds a shortcut to field values when you explicitly don't want/need the overhead of field item/item list/typed data. For the session user loading issue that would be necessary regardless.

I have a couple/handful of branches here trying to fix the memory leak generically, so that e.g. rendering lots of comments or paragraphs or along views listing would also benefit, so far the results are middling.

The problem is that ContentEntityBase stores both $this->values - the raw field/property values loaded from the db or cache, but also $this->fields - those same items wrapped in field item list classes.

The field item list classes are typed data, typed data has a $parent property, the $parent is the $entity. This means as soon as you call ::get() on a field you have end up with references to the entity inside field inside the entity. It looks like PHP garbage collection is not able to remove these multiple circular references, which means entity LRU static caching can't get rid of them.

I am trying to break the circular references using a WeakReference, or just not creating the references at all. Tried weakreferences for TypedData::parent and also for $entity->fields[$langcode][$field_name].

It is fairly easy to get something that works when entities are loaded and fields are accessed - I need to verify whether this affects the original memory leak I found or not which I'm hoping to do later today.

The problem with typed data parent being a weakreference is that if you lose the reference to the entity, there is no way to get the entity back again. I tried keeping entity ID and entity type ID around then loading it again if it's accessed after dropping out, but loading an entity can call into typed data and create infinite loops which is not fun.

Theoretically $entity->fields is only a static cache or explicitly set, so I also tried WeakReference there, that seems to work slightly better than typed data but still running into issues where the fields disappear in parts of the entity API that assume they'll be there.

This would be a lot easier if we had the concept of ::getEditable() for content entities, because that could set a mode where $this->fields doesn't use weak references, but we don't have that and it would be a massive change.

Note this won't cause any visible change in memory usage on Umami because there aren't enough entities loaded to run into the LRU purging, it needs to be more than 500 on a page.

@geek-merlin this isn't about the entity cache as such, that's the problem. We already have #3498154: Use LRU Cache for static entity cache for entity caching in core. In the controller I'm working against (pretty much worst case), I set the cap on entities to 10, called gc_collect_cycles() etc. and despite confirming that entities are removed from the cache, the objects still aren't cleaned up. This controller loads a lot of entities, calls $entity->get($field_name) on various fields including references to other entities etc. pretty sure it's actually breaking PHP garbage collection.

The 'cache' isn't the entity cache, but the property on entities that holds field lists - an extra little static cache on the entity object itself for field items. But per experiments on this issue and inline comments in the code, it's not purely a static cache but also the way that updates to fields are stored prior to entity save.

Added a fix for entity cloning which dramatically reduces the test failures - still enough to work through but going to test the latest version against the situation I originally found this on.

The code added here is not much but can probably be simplified - e.g. if we added a setFieldReference method that handles the 'should it be weak or not' logic in one place.

Here's some before/after profiling from the page I found this on with the latest diff applied. As you can see it's taking 10mb off the memory usage, also (not in screenshots) the total memory usage for the request drops from around 220mb to 210mb. I think this is enough to demonstrate that there's a real memory leak, that the FieldItemList objects on the $entity->fields property are responsible for it, and that weak references can help.

Pretty sure it also shows that we still have a memory leak even with the MR though, just a bit less of one because there is still extremely high memory usage from ::getTranslatedFields() and this stays high even if I reduce the entity LRU memory cache slots down to 10.

Re-titling since the memory leak is confirmed now.

At one point I had some logic in ::set() to only prevent weak references for the specific call and not subsequent calls, but took it out in desperation when trying to fix ::save() / __clone() bugs.

This particular page is doing some preloading of things in hook_entity_load() and setting them on the field items to prevent individual queries, comment counts is one. So that was undoing a lot of the optimisation because we still had the memory leak for all the fields requested after that.

Adding that back leads to a 30mb reduction in memory usage overall (compared to 10mb as above), which is about 1/3 of the total memory usage of ::getTransatedField() on this page.

The custom code could be implemented in such a way as to not set the values on the field item, which would save another 1000 entity-sized memory leaks so we're getting closer to 50% of the memory saved at this point. You can see the remaining 1000 calls because there are 1000 less weak references created than field instances (76000 compared to 77000).

Still more to do but convinced myself it's worth continuing here.

I tried implementing __destruct() in ContentEntityBase and it's not getting called, including when trying to force garbage collection via gc_collect_cycles() - I think this is a result of the circular references. PHP docs say 'when the last reference is removed' and that seems to not include circular ones.

Then I wondered about the typed data objects themselves, so implemented __destruct() in there. This does get called, at least a bit, but it doesn't free up any significant memory to speak of, at least not yet.

Memory usage going down a bit more with the combination of the weak references and the destructors. This has a couple of local changes applied to the custom code base to take advantage of things here more. Roughly 50% of the original memory peak memory usage from ::getTranslatedField() now, which equates to over 25% of the memory usage for the entire request.

No yet clear to me whether the destructor changes, by themselves or in combination with the weak references, cause a decrease in the peak memory usage. xhprof doesn't show it. Could probably use a better before/after when I've got more time, or might need to add a couple more somewhere.

Wrote up 3/4 of a lost comment then lost it :( trying again.

In LruMemoryCache we start unsetting entities in set() as soon as we exceed the number of slots, so it would be possible to call a method there.

The problem is that we can't guarantee at that point that the item in the LruMemoryCache is the last reference. If I have 100 slots, and load 200 entities at once into an array, the first 100 of those won't be in the memory cache any more, but I can be foreaching over them calling methods on them.

So if we had something in LruMemorycache::set() that called a manual ::destroy() method on entities:

public function destroy() {
   $this->fields = [];
}

Or whatever more complex version of that was required to break the references, the problem with that would be that my foreach loop could still be calling $entity->get() or $entity->save() afterwards, and any values explicitly set on fields in that array ready for saving would be lost. We could apply this to individual items in that array - .e.g. don't remove items that are explicitly there due to a ::set() call, but then we're very close to the logic in the current version of the MR.

Once thing I experimented with in the other MRs, but quickly ran into problems with, would be if EntityAdapter::entity and TypedData::parent contained information to retrieve the entity but not the entity itself - e.g. ID and and entity type, and then it's always loaded on demand. This would break the circular reference in the other direction - the fields wouldn't contain a reference to the entity. I'm not sure there's a workable way to do that though. The problem with that is there can be access on ->parent within entity loading itself, which then leads to infinite recursion.

@geek-merlin I think I'd seen something about it a while ago, but never used it, this might be a good issue to try it out on.

In the meantime found two more circular references:

1. $entity->typedData is a property that references an entity typed data object that references the entity, so a self-reference one step removed.

2. TypedDataManager::prototypes is a static cache that includes the original entity used to create the prototype as well as the typed data manager, which is completely unnecessary and would definitely prevent those entities being garbage collected when not in the static entity cache.

#1 may or may not be contributing to the problem here.

#2 definitely is contributing somewhat because it's an entire static cache of entity objects that the LRU cache cannot clear.

I haven't checked the impact of this on the controller loading hundreds of entities yet but results are promising when profiling umami.

Uploading a before/after of those two changes relative to the current state of the MR.

I've opened #3573982: Circular references / memory leaks in ItemList for what I think is the 'other half' of the memory leak - essentially the same recursive/circular reference problem, but for field items in field item lists vs. field item lists in entities. Depending on how things go it might be feasible to merge that issue back into this one, but there's enough breakage here already I don't want to add any more, and so far at least both can be worked on independently.

Opened #3574198: TypedDataManager prototypes should not include the parent context for one quick fix that's included here.

edit: and #3574230: Use a weak reference for EntityBase::typedData for another one.

Opened #3574012: Add a getFieldValue() method to bypass typed data overhead for specific use cases for @berdir's original idea from #2345611: [pp-1] Load user entity in Cookie AuthenticationProvider instead of using manual queries because this issue has taken a turn...

I tried that issue with the custom controller I've been looking at, and using ::getFieldValue() instead of ::get() in all the relevant custom code I could find it reduced total memory from loading 500 * 3 entities 100 at a time with a 400 entity memory cache slot limit from 220mb to around 60mb - this very much confirms that the memory leak resides entirely in the field/typed data system. I was also able to see the memory usage go up and down when tweaking the entity lru memory cache slots, which confirms that it can work. It also goes down a little bit for each conversion of $entity->get() to $entity->getFieldValue() which shows it's not just the entities themselves contributing to the memory usage, but the field item lists + items that get created too.

The current state of this MR reduces 220mb down to around 150-160mb, I think #3573982: Circular references / memory leaks in ItemList would probably do about the same (once it works).

If we're able to do those, there is probably some memory overhead just from creating the typed data objects in the first place, so it might not be possible to get down from 220mb to 60mb without the workaround/hack method, but maybe we can get down to comfortably under 100mb and that would still be a big improvement.

A couple of the issues that were spun out from this landed, so I've rebased. @andypost's last comment was on code from one of those issues that I'm pretty sure got reviewed there and resolved before the commit. Still lots of test failures, high level review would still be useful though.

OK the MR is green.

As you can see this is nearly entirely self-contained to ContentEntityBase. There are two exceptions:

1. Excluding the new property from a method in ContentEntityCloneTest - this is purely an oddity in how the test takes some shortcuts.

2. The change to ContentEntityDeleteForm - this is because it was accessing the translated value of a field immediately after that translation was deleted, and the WeakReferences mean that the field reference is actually not there any more. And then accessing it tries to recreate it again, but correctly tells you that the translation doesn't exist, because it's just been deleted. For me this is a harmless but subtle bugfix for some fragile code, but a bit of a behaviour change.

As mentioned above, assuming we're able to fix this, it's not the only memory leak in the entity/typed data system, I also found #3573982: Circular references / memory leaks in ItemList when working on this, which may or may not be fixable via a similar approach. That's a separate memory leak though, this one already allows some to be freed that previously wasn't.

See #33 for some numbers against a quite extreme case I found in a client project.

Rebased.

Ahh the ::getTranslation() change was https://git.drupalcode.org/project/drupal/-/merge_requests/14752/diffs?c... which was quite late in the MR, looking back I can see I profiled this just before that commit but not after... good spot.

Two fixes were needed:
- in onChange(), promote its weak reference to a strong one. This covers normal edits like $entity->get('name')->value = 'x'.
- in ContentEntityStorageBase::initFieldValues(), turn weak references off while the initial values are being set. This covers the default values set with $notify = FALSE during create() and createTranslation().

Both of these changes look right to me.

@nicxvan the fail in LayoutBuilderViewModeTest is real and its due to what's described in #41. Moving this to needs work. I think unless we can find a way to formalise how field items get updated, which would be very hard to do with full bc, we're a bit stuck here.