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

facade/ignition#302: Fix memory leaks caused by log and query recorder #344

Merged
merged 1 commit into from Feb 5, 2021
Merged

facade/ignition#302: Fix memory leaks caused by log and query recorder #344

merged 1 commit into from Feb 5, 2021

Conversation

TheLevti
Copy link
Contributor

@TheLevti TheLevti commented Jan 17, 2021

  • Add optional log recorder limit to prevent memory leaks. Especially
    noticeable during long running tasks or daemons.
  • Rewrote query recorder to not tightly couple it with the framework
    container when deciding whether to include bindings or limit the
    maximum query count that is recorded.
  • Added a config flag to disable log recording all together.
  • Made sure that query and log recorders are not used when their enable
    flag is disabled.

Closes #302

Closes #301

@freekmurze
Copy link
Collaborator

On first sight, this seems ok to me.

@AlexVanderbist can you review and merge this?

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rubenvanassche rubenvanassche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor changes request, but a great PR, look forward to merge it!

src/IgnitionServiceProvider.php Show resolved Hide resolved
src/IgnitionServiceProvider.php Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@AlexVanderbist AlexVanderbist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the minor remarks by my colleagues; LGTM!

- Add optional log recorder limit to prevent memory leaks. Especially noticable during long running tasks or daemons.
- Rewrote query recorder to not tigthly couple it with the framework container when deciding whether to include bindings or limit the maximum query count that is recorded.
- Added a config flag to disable log recording all together.
- Made sure that query and log recorders are not used when their enable flag is disabled.
@rubenvanassche
Copy link
Contributor

Thanks, looks great!

@rubenvanassche rubenvanassche merged commit a92dcbe into facade:master Feb 5, 2021
@TheLevti TheLevti deleted the feature/fix-mem-leaks branch February 5, 2021 21:21
@rimace
Copy link

rimace commented Feb 13, 2021

This change is causing a memory leak for me.
I am importing ~300,000 models. First 1,000 models are read and bundled into one transaction, then the query is fired and then next 1,000 and so on.
Usually my import uses always around 25 MB RAM, but since version 2.5.11 my importer reaches 500 MB RAM after a while.
Will report a proper issue with an example later today.

@TheLevti
Copy link
Contributor Author

This change is causing a memory leak for me.
I am importing ~300,000 models. First 1,000 models are read and bundled into one transaction, then the query is fired and then next 1,000 and so on.
Usually my import uses always around 25 MB RAM, but since version 2.5.11 my importer reaches 500 MB RAM after a while.
Will report a proper issue with an example later today.

Thanks for the feedback. Please let me know the details. For my use case the memory leak that I had previously was fixed once the array is topping out at the configured limit.

Copy link

@rimace rimace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's it.
The QueryRecorder uses a new config which didn't exist in my config cache (forgot to recreate it).
Because QueryRecorder was activated but the maximum_number_of_collected_logs didn't exist in my config cache yet, it was null which deactivated the "maximum logs" of 200 (by default).
So it was my fault for not recreating the config cache. Still I think the name of the config variable is wrong there.

src/IgnitionServiceProvider.php Show resolved Hide resolved
rubenvanassche added a commit that referenced this pull request Feb 15, 2021
lisadeloach63 pushed a commit to lisadeloach63/laravel-ignition-error-page that referenced this pull request Sep 1, 2022
lisadeloach63 pushed a commit to lisadeloach63/laravel-ignition-error-page that referenced this pull request Sep 1, 2022
sadafrangian3 added a commit to sadafrangian3/laravel-ignition that referenced this pull request Apr 6, 2023
sadafrangian3 added a commit to sadafrangian3/laravel-ignition that referenced this pull request Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants