Moodle

Assigning to Dongsheng...

IMO all those ini_set('memory_limit', XX) directives should be replaced by the raise_memory_limit(XX) function that handles the same but respecting php.ini settings.

Note that the function is in lib/setuplib.php and I'm not sure if that lib is included always, so must be checked.

Can your review that, plz? TIA! Ciao

Eloy, raise_memory_limit is used to raise memory, but sometime we need to turn it down
In search/cron_php5.php, the script raise the memory limit then change it to the normal limit, but we cannot do this by raise_memory_limit.

What do you think?

As I'm usually setting my own memory limits for various functions, I think it would be nice to have three memory limits that can be specified in the config.php - low, medium, and high where high would fit the maximum memory limit that can be consistently used across all of Moodle where memory limits are applied. This way, large sites don't have to go around the Moodle source to increase memory limits or fiddle with PHP ini files.

Well,

while having multiple settings (min, normal, max) as commented by Elvedin could be a solution... I think that, in this case, it's enough to guarantee the php.ini setting is respected (i.e. it's never lowered by code).

So I think that it's ok to change those calls in search/cron_php5.php to the raise_memory_limit() function.

Also note that the call near the end of the script:

ini_set('memory_limit', $maxmemoryamount);

used to "restore" original value is really useless, because the script ends there, so nothing else is executed. So I think we can delete it safely (or leave it as is). NP with that.

Ciao

This patch fixed hard-coded memory limit, and allow admin setting memory_limit in admin panel, server/performance

Hi Dong,

there are two bits in the patch I think need some decision:

1) In admin/health.php you use:

raise_memory_limit($oldmemlimit);

and that won't work, because that function is only able to raise mem, never lower it. So, perhaps, it will need the original ini_set() or so.

2) About the new configuration setting, I think it's a good idea, but perhaps it would be interesting to have it only with big values: 64MB, 128MB, 256MB, 512MB, 1024MB explaining in what exact parts of Moodle it's going to be used (search indexing, backup & restore, admin/health.php), perhaps also in Moodle Docs page. But in any case, always specifying that any setting in php.ini will have precedence. I'd make it to default to 128MB and apply it to those 3 places: search indexing, backup & restore, admin/health.php for now. Let's see if we need to apply it to more places along the time. And note install.php isn't it the list, 40M is ok there IMO.

Feel free to comment with MD if you want. Ciao

+1 for what Eloy just said

Fixed, please review,TIA

Hi Dongsheng, thanks fo the fix. Reopening since in 2.0 and 1.9 the default for memory_limit is not set nor is asked to set by admins during upgrades.

This yields to a "invalid current value: 0" under "PHP memory limit" in Administration Performance page.

Fixed, please review.

change admin setting

Do we know what really happens when reducing memory limits? What is there is more memory allocated already? What is somebody intentionally sets very large value in php.ini?

Alse the inline docs seem incorrect:

• Function to reduce the memory limit to a new value.
• Will respect the memory limit if it is lower, thus allowing
• settings in php.ini, apache conf or command line switches
• to override it

the function does not detect anything and just lowers the memory.

My +1 to have this new feature only in HEAD.

Fixed, I will check what is the difference between admin_setting_configselect and admin_setting_special_selectsetup

PHP memory limit
memorylimit
Default: 128M
This sets the maximum amount of memory that a script is allowed to allocate. This option is applied to search indexing, backup/restore and admin/health scripts.

The setting name and also description does not look ok to me at all.
1/ it is NOT memory limit for all scripts
2/ if it is for special pages it should not be called $CFG->memorylimiot which would be very misleading

proposing change immediately $CFG->memorylimit to something else because it is not "Memory limit", maybe $CFG->extramemorylimit

Agree with Petr... changing it to:

$CFG->extramemorylimit
(and also changing explanation, pointing it only affects some special scripts backup/health...)

+1

PS: Also, of course needs to discover why the default isn't being detected in install/upgrade (and undo the "0" hack introduced by last commit).

all those if/else can be moved into a new function raise_memory_to_extra_limit() (needs better name)

hmm, maybe it is not a good idea

Yes, agree (it's not a good idea). I also proposed that to Dong but finally we discarded it.

I have committed the "extra memory" clarification - just changing name of variable was not exactly what I wanted to suggest, sorry

just a note, the option 512Mo is missing

Please review, feel free to reopen if there are any problems

512Mo added, can I resolve this issue, Petr?

Closing this. Thanks Dongsheng and everybody else. Ciao