Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.x] Optimize eager loading memory handling #30248

Merged
merged 1 commit into from
Oct 14, 2019

Conversation

netpok
Copy link
Contributor

@netpok netpok commented Oct 11, 2019

Fixes #30114

This pull request fixes a memory leak issue reported in the mentioned issue, I think this may be a bug in PHP itself so I will report to them too.

The problem is that the Eloquent builder is creating closures for the relations, these relations are stored in the eagerLoads property of the class because these closures are containing a reference to the builder the GC thinks the class is still referenced. By refactoring these closures to static ones this reference does not get stored so the GC can correctly clean up the builder earlier.

edit: Turns out this just delays the GC, it gets detected later when memory usage hits ~24MB. Updated the title to optimize.

These methods does not use the $this reference so there is no other change needed.

@netpok netpok changed the title [6.x] Fix eager loading memory leak [6.x] Optimize eager loading memory handling Oct 11, 2019
@taylorotwell taylorotwell merged commit 02d0226 into laravel:6.x Oct 14, 2019
@taai
Copy link
Contributor

taai commented Oct 17, 2019

Hey, @netpok, from your PR I learned something new today – about static closures. Thank you! So now all closures should get checked and converted to static closures, if they don't use $this. For example, at the same file, in the hydrate method is a closure that does not use $this and can be converted to a static closure. Am I right?

@netpok
Copy link
Contributor Author

netpok commented Oct 17, 2019

Yes that can be replaced too.

Although that's not that big of a problem, here the main issue was that these calbacks were stored in the same class, so the gc viewed the class as it still had a reference. There are $this cycles in the gc to check this but that is not executed that often because of the performance penalty it causes.

So I'm not sure the performance boost worth the effort. But other classes may have this kind of issue too.

@netpok netpok deleted the bugfix/eager-load-memory-leak branch October 22, 2019 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[6.x] Memory leak on Redis queue when using relation
3 participants