Problem/Motivation

Split from #3572625: Calling $entity->getTranslatedField() results in an entity-sized memory leak because it's a self-contained issue.

$entity->typedData is a static cache of a typed data object containing the entity. This creates a circular reference which is likely contributing to the memory leaks in #3572625: Calling $entity->getTranslatedField() results in an entity-sized memory leak. Fixing this in itself won't improve memory usage (as far as I know), but in combination with the other changes in that issue it should. However if we do it as an isolated change it reduces the scope of that issue.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3574230

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:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !14814

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

Not sure about bc here. There's a getter for the object, and it's required to use that because the typedData property is only created when that's called, so you can't assume it's set up correctly if accessing the property directly. However someone theoretically could access it directly in an entity subclass and it might have worked by accident.

Apart from using property hooks or magic methods, don't have a solution to providing 100% bc for that.

andypost made their first commit to this issue’s fork.

andypost’s picture

andypost
he/him
Primary language Russian
commented

changed structure a bit

smustgrave’s picture

smustgrave commented

Based on the 1 no discard issue I reviewed this looks in line, question will this need test coverage at all?

andypost’s picture

andypost
he/him
Primary language Russian
commented

I bet it needs test for memory leaks

catch’s picture

catch
he/him
Primary language English
commented

It's don't think it's possible to test for memory leaks here.

This will only completely allow entity references to be freed once other changes in #3572625: Calling $entity->getTranslatedField() results in an entity-sized memory leak are done.

Apart from that, I'm not sure there's even a way to test for memory leaks reliably.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

In that case I'll go on a limb and mark it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Looking at https://search.tresbien.tech/search?q=%22-%3EtypedData%3B%22%20-f%3Acore... and https://search.tresbien.tech/search?q=%22-%3EtypedData-%3E%22%20-f%3Acor... I think we're okay here. It's looking like no one is accessing this directly... and there is plenty of usage of the method https://search.tresbien.tech/search?q=%22-%3EgetTypedData()%22%20-f%3Acore&num=50&ctx=0

So I think we're good to merge this to main and 11.x but I'm just going to let it sit my head for an half a day in case something pops up.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

I pondered whether even the weak reference was worth it and whether we should just be creating a new object each time this is called but I run the standard install and could see the weak reference returning a value when we saved an entity so this does help during entity saves so I think we're good to go here.

Committed and pushed 410370cb86f to main and 5f9de462bed 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 5f9de462 on 11.x 
    perf: #3574230 Use a weak reference for EntityBase::typedData...

  • alexpott committed 410370cb on main 
    perf: #3574230 Use a weak reference for EntityBase::typedData...
catch’s picture

catch
he/him
Primary language English
commented

weak reference was worth it and whether we should just be creating a new object each time

I did profile with this but yes it gets accessed quite a lot.

andypost’s picture

andypost
he/him
Primary language Russian
commented

It will consume less memory (-3k every entity) as new method will be introduced or creation will be delayed via yield

Status: Fixed » Closed (fixed)

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