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

Fixed incorrect memory_limits according to DevDocs #24533

Closed
wants to merge 1 commit into from
Closed

Fixed incorrect memory_limits according to DevDocs #24533

wants to merge 1 commit into from

Conversation

DavidLambauer
Copy link
Contributor

This DevDocs recommends a PHP memory_limit setting of at least 2G.

This DevDocs also say, that there should be at least 2G for running M2 properly.

There has been an issue (#11322) which was closed a while ago without solving the issue.

Therefore, this pull request raises the limit of the default memory_limit.

@m2-assistant
Copy link

m2-assistant bot commented Sep 9, 2019

Hi @DavidLambauer. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@ghost ghost assigned torhoehn Sep 9, 2019
@magento-engcom-team
Copy link
Contributor

Hi @torhoehn, thank you for the review.
ENGCOM-5819 has been created to process this Pull Request
✳️ @torhoehn, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @DavidLambauer,
AFAIK 2G is recommended value because of CLI scripts, crons, etc, but lower 756 mb for web requests.
@akaplya could you confirm that?

@ihor-sviziev ihor-sviziev self-assigned this Sep 9, 2019
@hostep
Copy link
Contributor

hostep commented Sep 9, 2019

This should not get approved please, also see: #24157 & magento/devdocs#739 :)

@DavidLambauer
Copy link
Contributor Author

@hostep can't follow your arguments. Currently, the code and the docs distinguish. If it is recommended to have separate values for either cli and fpm, then this should be configured correctly within the nginx.sample/.htaccess and the .user.ini files. Giving hardware recommendations might be a topic that can be discussed a lot. Nevertheless, the current values don't align with the docs.

IMHO: M2 needs to have 2G at least.

@hostep
Copy link
Contributor

hostep commented Sep 9, 2019

Please show me a feature in a non-modified Magento which uses 2 GB of memory?

As already mentioned in the linked tickets, there is currently no suggestion for a memory_limit value for running Magento in a webserver context in the docs, people should choose their own limit on a project per project basis.

That memory_limit value exists for a reason, otherwise everybody would just set it to -1.

See php docs:

This sets the maximum amount of memory in bytes that a script is allowed to allocate. This helps prevent poorly written scripts for eating up all available memory on a server. Note that to have no memory limit, set this directive to -1.

Trust me, there are a lot of poorly written php in the magento universe (not saying Magento core, but things I've seen in 3rd party code for example, or my own for that matter).
You don't want visitors of your shop being able to pull down your entire server just because they triggered some memory intensive script (most likely due to a bug).

@DavidLambauer
Copy link
Contributor Author

Hi @ihor-sviziev, if you're correct, we would need to update the .user.ini to use 2G at least. .user.ini and .htaccess/nginx.conf can have different impacts.

@hostep
Copy link
Contributor

hostep commented Sep 10, 2019

@DavidLambauer: does the .user.ini file has impact on the php cli? That's the first time I'm hearing this? According to the docs it shouldn't:

Since PHP 5.3.0, PHP includes support for configuration INI files on a per-directory basis. These files are processed only by the CGI/FastCGI SAPI.

As far as I'm aware, the value for the cli is taken from the php.ini files or the additional .ini files in your php installation, which you can find by executing php --ini

@DavidLambauer
Copy link
Contributor Author

We're talking about 2 different ini-loading mechanisms. The default php-cli stuff evaluates the global php.ini. Furthermore php's default settings say, that a .user.ini will be evaluated as well. For example: if you're running php bin/magento, the .user.ini in the Magento root will have an effect. The .user.ini in pub shouldn't have an effect.

For PHP-FPM usage, the global php-fpm.ini will be used (Often the same as the cli ini). Farther, additional configurations can be set either via nginx.config or via .htaccess (depending on your webserver).

Summarized:

The .user.ini in the Magento root directory should be necessary to set configurations for the use of php bin/magento.

The .htaccess under pub and the nginx.conf.sample in the Magento root directory should be necessary to set configurations for Web Requests.

As far as I know, Magento recommends to point the document root to pub/ instead of root. Not sure, when you want to make use of the .htaccess in the root dir. Apart from that, the .user.ini under pub/ shouldn't have any effect at all?

@hostep
Copy link
Contributor

hostep commented Sep 10, 2019

Thanks for the elaborate feedback!

if you're running php bin/magento, the .user.ini in the Magento root will have an effect

Can you show how or why this happens? Does this depend on a certain setup? Because I'm not seeing this?

I've tested by putting some var_dump(ini_get('memory_limit')); lines in bin/magento on various places, it always returns -1 (which is what is setup in my php-cli.ini file). The .user.ini file in the Magento root does define 756M, but that value isn't being used from what I see.

@DavidLambauer
Copy link
Contributor Author

Alrighty, so I did some tests 😜:

I have a local setup (php-fpm) with the default php.ini and a nginx as my webserver. My global php.ini defines the memory_limit as -1, so unlimited. The following files define a memory_limit of 756M:

  • ./root/.user.ini
  • ./root/nginx.conf.sample
  • ./root/.htaccess
  • ./root/.htaccess.sample
  • ./root/pub/.htaccess
  • ./root/pub/.user.ini

My document root is pointed to ./root/pub/. I added echo ini_get('memory_limit') to the index.php.

As I am using nginx, any .htaccess files won't have any effect.

After pointing my browser to my test instance, I receive 756M as expected. Global php.ini settings don't effect the site.

Changing the params in the ./root/nginx.conf.sample to use a memory_limit of 2G has no effect at all. I also restarted the services for nginx and php-fpm to make sure no caching or stuff is in place.

Changing the params in ./root/.user.ini has no effect.

Changing the params in ./root/pub/.user.ini works.

Deleting the ./root/pub/.user.ini... Now the values from the ./root/nginx.conf.sample are in place.

This leads me to the following statements:

Magento has two types of document roots:

  1. The first one is under ./root/ and enables user to use the Web Installer
  2. Then there is the actual production root under ./root/pub/. This root is recommended. See DevDocs.

Having two document roots explains the necessity for having duplicate .htaccess and .user.ini files.

The specified values in the nginx.conf.sample seem to be a fallback for situations where the .user.ini is missing? These settings seem to be useless for me.

In the ./root/pub/ dir, we have the .htaccess and the .user.ini. Both are used by the webserver, either nginx or apache. So these are the files that are important.

@hostep You're absolute right that the ./root/.user.ini has no effect on cli php usage at all. Sorry for that!

The whole research would still end up in this PR. As @hostep and I just researched: There is nothing that effects the compilation of static assets via cli. Therefore, we only talk about the use of the memory_limit settings for webserver requests. And those should be 2G IMHO :).

@hostep
Copy link
Contributor

hostep commented Sep 10, 2019

Awesome research!

Therefore, we only talk about the use of the memory_limit settings for webserver requests. And those should be 2G IMHO :).

Sorry to keep nagging about this, but why should it be 2 GB for webserver requests?
What functionality in Magento which runs in context of the webserver needs this much memory?

@DavidLambauer
Copy link
Contributor Author

Basically, I am not able to share a profiler output or so. Getting valid results for memory use in PHP is quite hard I guess. Also, memory usage depends on a specific request. This PR is based upon two assumptions:

#1: I tend to support bigger M2 installations with a couple of stores, lots of products, customers and data. Increasing the memory_limit was always necessary in order to have a performant site.

#2: DevDocs recommendation

@hostep
Copy link
Contributor

hostep commented Sep 10, 2019

Increasing the memory_limit was always necessary in order to have a performant site.

That's super weird, it should not have any influence on the performance of a shop? It's just an upper limit. Unless there is some code in Magento explicitly checking this value and then behaving differently based on the value? Can't seem to find that at a first glance?

I tend to support bigger M2 installations with a couple of stores, lots of products, customers and data.

Most of our shops are running with only 256M of memory_limit without any issue.
Albeit that these aren't huge, mostly 2 or 3 storviews, up to 20.000 products sometimes, so not super big.

Hence my hesitation to get this merged, because 2GB is kind of ridiculous in my opinion 😉
If you actually need it, then please go ahead and change it for the particular project which needs it, but to make it a default is not ok in my opinion.

@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Sep 11, 2019

Hi @DavidLambauer and @hostep ,

Thank you both for contribution and productive discussion!

I just double checked and see that in both links links in DevDocs you can see that memory_limit should be defined 2G in php.ini file:

https://devdocs.magento.com/guides/v2.3/install-gde/trouble/php/tshoot_php-set.html
image

https://devdocs.magento.com/guides/v2.3/install-gde/prereq/php-settings.html
image

As @hostep already said - current limit is already high enough and should not cause any issues. If you have any steps to reproduce where higher limit is required - please report separate issue with exact steps to reproduce.

Also if you think it's not good enough documented - feel free to create issue or PR in devdocs. You can suggest any change to https://github.com/magento/devdocs/ repo.

I'm closing this PR.

@m2-assistant
Copy link

m2-assistant bot commented Sep 11, 2019

Hi @DavidLambauer, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants