I modified it to reflect to return an integervalue larger then -1. MAX_INT might conflict with memory settings (e.g. on 32 bit systems, needs verififcation), so (int) might not be appropriate and need to be replaced with (float). ( Related: #10332 )

I have understood it quite fine. The memory settings are assumed to be in Megabytes, (int) of such PHP Memory strings will result in the Integer fraction of the string to be returned. PHP 5.1+ allows for Gigabyte memory strings, As such, WordPress cannot assume it to be in M.

PHP Memory limits being expressed in kilobytes however, Is pure madness, Including that check here seems like it'll be a waste of time.. Remember, this is code that is run on every pageload as part of the bootstrap, The less operations needed the better.

So with your life making easier suggestion you must add as well, that server admins must configure the memory limit with the M shorthand notation.

No, My mention that was the PHP memory limits should be checked for G, however, WP_MEMORY_LIMIT (Which 'we' (as WordPress) have have the _choice_ to decide how it's defined) Presiously it has mearly been taken to be the same as the php memory limit, However has only ever worked as being expressed in Megabytes, We can choose to continue that by explicitly requesting it be defined as such, If someone defines it as G we'll silently pass on it as (int)64MB(PHP) > (int)1G (WP_MEMORY_LIMIT)

Its purely a thought for optimization and keeping the 2-line code as simple as possible.

For various reasons, We've also removed anonymous functions elsewhere in code, This is another thing which I feel is effectively part of the changes that occur to the standards over time.. It may not be written as such, but it's something that has been moved towards.

We also already have a function for converting PHP Size notations for use in the Uploads: wp_convert_hr_to_bytes() - wp-admin/includes/template.php - But still, I feel the use of that is overkill for a bootstrap function.. You can over-engineer anything, but sometimes, a simple check is better.

+1 for using a named and not an anonymous function (as this is not PHP 5.3) - if at all.

If you don't like the drawback of having additional code in the bootstrap (albeit the bootstrap's complexity can be reduced but it's normally argumented against it - probably it depends who brings things up) in _this_case_ this could be done if administrators are told that they MUST configure their memory limit in the M notation in both PHP.INI and wordpress configuration.

From a real life standpoint this can work, some might wonder, but if it's documented, why not? Most will have the M notation anyway. And even if you uses gigabytes, you can pretty easily write them in megabytes 1G = 1024M.

Here's a new patch that addresses the ideas brought up in discussion about the original patch, namely:

Does not use an anonymous function.

Assumes that WP_MEMORY_LIMIT is in megs.

The wp_convert_hr_to_bytes() function was not used as it creates a chicken and egg scenario due to the wp_initial_constants() function being called well before the wp-admin/includes/template.php file can possibly be loaded. Restructuring is possible, but this could possibly cause new issues while attempting to address this one. So, adding the code directly to the check was the route I went with.

While the patch does assume the WP_MEMORY_LIMIT define to be in megs, it makes no such assumption about the existing memory_limit configuration and normalizes the size for bytes, kilobytes, megabytes, and gigabytes.

The patch is added as chrisbliss18-patch.diff.

Since performance is a concern, I benchmarked the existing code against the first patch and my patch. I found that the original patch increased processing time 100-120% on my test machines while my patch increased processing time by 25-35%. I should note that the processing time for the first patch would be higher if I could test it completely (I had to move the anonymous function creation to outside the testing loop to prevent memory exhaustion, it is not possible to reclaim memory consumed by an anonymous function while in execution).

The benchmark code is attached as chrisbliss18-patch-benchmark.php and works as a a good testing platform. The benchmark code uses six different test cases. The existing WordPress code has a success rate of 50% while both patches have a success rate of 100%.

Note that my test cases does include "7.5G". I found that while this doesn't cause any issues, PHP treats such values as ints by removing the decimal, resulting in "7G" for this test case.

Thanks for providing an additional patch, the test code and your efforts in benchmarking it.

I could quickly review your patch. Even I did work on this ticket and reviewed the case multiple times I do not fully remember whether ini_get does return the shorthand notation or the expanded value in bytes and if so that one or other would be the case on every time, version and system configuration.

Next to that, I think both cases (WP:"1G" as INT 1 < PHP:"1024M" as INT 1024) and (WP:"2048M" as (int) 2048 > PHP:"4G" as (int) 4) need to be dealt with.

The way how to deal with those two cases is not specified in documentation so far. The current behavior is, that the memory_limit setting is only updated/set to when it is _assumed_ (I call it assumption as that part of the process is broken) that the PHP configuration is lower then the WordPress configuration value.

If I read your patch right, you assume that the configuration value is returned as an integer number of bytes in any case. I can't say so.

The ini_get function returns the value as it was set. So if I set in my php.ini "7.5G" (which is invalid and treated as 7G by PHP), it still returns "7.5G".

I tested to confirm that 7.5G does indeed translate into 7G in PHP, and it does. Since this is the case, I force the value to an int since that is how PHP works with it.

As for the test cases you propose, the first is invalid as it was already determined earlier in the conversation that WP_MEMORY_LIMIT will be restricted to only supplying a meg format. The second one works as expected with the patch I supplied.

I've tested the code supplied and have provided my test code for review. Can I please get someone that is willing to try the code to sign off on it so we can get this in 3.1? I'm helping at least 3 people a week fix this problem.

The ini_get function returns the value as it was set. So if I set in my php.ini "7.5G" (which is invalid and treated as 7G by PHP), it still returns "7.5G".

Thanks for the insight.

I tested to confirm that 7.5G does indeed translate into 7G in PHP, and it does. Since this is the case, I force the value to an int since that is how PHP works with it.

Can you test what happens if you write 7.5G 64M into the ini? Into what is this translated then?

As for the test cases you propose, the first is invalid as it was already determined earlier in the conversation that WP_MEMORY_LIMIT will be restricted to only supplying a meg format. The second one works as expected with the patch I supplied.

That restriction is a virtual one only. I did it here so to have a de-facto specification to go around any issue in the process of assuming for this ticket. So to say: "Fixing the issues by not changing any code." (a.k.a as "wontfix")

I've tested the code supplied and have provided my test code for review. Can I please get someone that is willing to try the code to sign off on it so we can get this in 3.1? I'm helping at least 3 people a week fix this problem.

Yeah would be great to see something at least in 3.1 but I don't want to sound rude to you, FWIK about WordPress core developement, this or anything related will make it into 3.1. Not my fault thought. If my opinion would count at least a patch that would bring administrators into control of memory configuration again would make it into 3.0.5 even.

I decided to revisit this problem and find a way to change the code to be just as fast but while adding the ability to allow the WP_MEMORY_LIMIT define to use units other than M. I believe that I have succeeded.

After many benchmarks and tweaks, I got the code to average out to about the same speed as my last patch (in my trials, the speed difference is around +-5%) while only adding four lines of code. I'm a bit of a proud code parent. The new patch is chrisbliss18-patch.2.diff.

Note that one of the big speed improvements came from removing the function_exists check for memory_get_usage. PHP 5.2.1 removed the need to compile PHP with the --enable-memory-limit compile-time directive in order to manipulate the memory_limit. Since 3.2 has a minimum PHP version of 5.2.4, there isn't a need for this expensive check.

For those that are interested in the benchmarks, I've updated my benchmarking script to also include this new algorithm. Here are the stats while running it on my current dev system:

Updated my chrisbliss18-patch.2.diff file. The update now checks for values of memory_limit equal to or less than -1, which allows unlimited memory consumption.

I believe that this is the reason that the original code failed for me. I just so happened to change my php.ini file while trying to figure out the issue and forgot about the original setting. I verified this by resetting memory_limit to "-1" in my php.ini file and activating enough code to consume 32M of memory. This would fail while setting to "128M" would not fail.

My code tests for a memory_limit of -1 or less because my testing shows that any value of -1 or less (including values with fractional parts) will always be unlimited even though the documentation only says that this applies to values of -1. Interestingly, a value of -.5 is treated as 512K.

What I ran over was the order of m,g,k. First impression was, why not g,m,k or k,m,g. However that's detail.

Another Idea I ran over is because of the duplicate code. I'm aware that it's not wanted to introduce a new function (or if, it would made the patch stale), so I was thinking about something else than a function - a shared map (rough code, exec'ed):

Next to that it should be even possible to iterate over both variables after init, so to reduce the duplicated code full but that costs an iteration (runtime).

However with the rest I wonder if it can be properly said that if the system setting is -1 that it should not be degraded to the setting explicity specified. I'm unsure about that, but just wanted to leave the note.

Creating that array seems to have a high penalty on the code execution time. I also believe that the pow function has a high overhead that accounts for the more than doubling of processing time when compared to the other function using a map.

As for the reason that I decided to test in order of M, G, and then K is because most systems will have configurations that use a unit of M. Both G and K are unlikely (I've never seen them), but I decided to put G second as it is more likely to be used with more frequency than K as the years go on. I would have loved to put a test for no unit second since that is the second most-likely case, but since that has to be the fallback, it had to be made last.

@aaroncampbell (et al) array_search is pretty costly (that's known, just saying), I was looking into pow as well, probably searching in a string ("BKMG") is faster (and/or with array_search to set $strict param to true).

Sparing the pow by using precalculated values is somehow reasonable as we're lucky to have a function with local scope already that get's dropped after being used (for the local vars especially like $map).

@chrisbliss thanks a lot for the quick support and directing the tests. I really appreceate your input and the way you're taking care. I really have missed the -1 case and I think that's more important here in this issue. Otherwise we might have solved the "m" vs. "g" while still not parsing the ini setting to the full range of options (for which this ticket was opened!: the _full_ range of options to deal with properly as ini settings are user input we need to deal with properly within widespread applications).

Don't thank me too much. It's thanks to Nacin that this was caught. He wanted to know what the real cause for my issue was. Going back for a revisit, I was able to recreate the original conditions... that of the -1 setting in php.ini.

@chrisbliss: Yep, looks like your way is faster. However, looking at your test script we're really starting to split hairs here. I hate duplicate code, so I'd prefer to see the string->bytes conversion pushed to a function rather than duplicated:

@aaroncampbell: If it's now fine to have a function for that, I would be confident with it as well. But IIRC earlier in this or a related ticket it was criticized to have an additional function for it. So a decision whether or not would be useful.

Added: The function you propose can return (float) already so you can spare the casts when used. (int) can not be used because of integer overflows, so probably a round() of the bytes calculated.

Added2: @see wp_convert_hr_to_bytes

What about making -1 available for the WP setting as well so to have it in ligne with the PHP setting? From the patch code now, I think we don't support -1 for a WP setting for unlimited memory. This can be useful for the new WP_MAX_MEMORY_LIMIT ([17749], #13847) setting if the admin decides to give mem a push to top.

A memory value of 2g will trigger the ​PHP_INT_MAX for signed integers on 32bit systems. So if you can add that value (2147483648) for tests with memory settings, I think that would be good to prevent triggering this issue.

The comparison function should successfully work on 2147483648 as well as on 4294967296 on 32 bit systems (2g and 4g in bytes). With integer overflow, 2147483648 in error can be 2147483647 or -2147483648, 4g can be 2147483647 or 0 bytes if the error occurs (cap @ PHP_MAX_INT)

I updated the benchmark script. I added some side-case values (the benchmark script does only compares output, so it can not completely check if the algos properly handle values in full), updated the map() function to use a static map and to increase it's success rate to 1. Added as well an str() function for testing.

What I see is that not all functions have the admired success rate. I tweaked map() to be conform, to be fair other functions might need some love as well.

Using pow() looks expensive to me as well.

But what looks even more expensive to me (not shown in stats, but I tested for it) is the @ operator. If it is removed from the code speed will go significantly up. I wonder if it is really needed for ini_get() / ini_set(). Perhaps warnings will scare a certain type of users away, because it will do warnings if those are disabled in disable_functions. If so, return will be NULL.Probably it's worth to cut short on NULL.

Functions with error/notices get a slight penalty as the error callback gets called now. So if a function is giving notice or warning, it will not only fail but take longer.

At this point this seems like entirely unnecessary micro-optimization. We're nearing 3.2 RC. Let's add in a check for -1 and be done with it. Fixes the bug, and the enhancement to support more values can wait.

The suggested patch does not fix the issue with comparing the values. You can not compare integer values because of the shorthand notation. Just noting not that this ticket gets closed w/o fixing the issue.

Additionally setting WP_MEMORY_LIMIT to -1 (unlimited) will not work with 14889.diff​.

However regarding the 32bit integer overflow I've found out that it actually looks not to be a problem as even the core ini values break with that limit. So it's actually somewhat okay to cast to int. ​See note here. But I have not tested for that so it remains a bit ambiguous.

5.2.1 Compiling with --enable-memory-limit is no longer required for this function to exist.

No -- hosts may still disable the function. We need to continue to account for it.

Account what? A function that always exists unless put into disabled_functions (which was not why it had been checked for prior PHP 5.x)? And if so part of disabled functions? The code that was dependent on it does not make use of it or am I missing something? Can you explain for what it needs to stay in there for 3.2? I must admit I'm a bit puzzled ... ;)

Update: According to the documentation when the function was introduced, it has been put in to determine "if PHP was compiled with --enable-memory-limit." As since PHP 5.2.1 it obviously does not fullfil this check any longer.

It's used to determine if PHP was compiled with --enable-memory-limit.

As the check for the function is not of use any longer (to check for compilation with --enable-memory-limit) and the function itself is not used at all, the check of it's existence is superfluous. I see no reason to keep it. But probably I'm missing something here, I'm interested about your thoughts Nacin.

Replying to hakre:
Thanks for digging that up (#3141). Three years ago in WordPress years is basically forever, so we probably just didn't remember WHY it was there and thus we were keeping it just to be safe.

I'll check and see whether it should be removed or whether we actually need a new way to tell if PHP was compiled with --enable-memory-limit. Maybe Ryan will remember more about it.

Replying to hakre:
Thanks for digging that up (#3141). Three years ago in WordPress years is basically forever, so we probably just didn't remember WHY it was there and thus we were keeping it just to be safe.

I'll check and see whether it should be removed or whether we actually need a new way to tell if PHP was compiled with --enable-memory-limit. Maybe Ryan will remember more about it.

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.

Prior to PHP 5.2.1 in order to use this directive it had to be enabled at compile time by using --enable-memory-limit in the configure line. This compile-time flag was also required to define the functions memory_get_usage() and memory_get_peak_usage() prior to 5.2.1. [emphasis added][...]

I don't see a NULL case as a possible return value at ​http://php.net/ini_get, so I don't really understand 14889.3.diff​. 14889.4.diff​ removes the function_exists() checks, which I'd rather not do. A host currently disabling that function would be compatible with WordPress, and changing this would make them incompatible, causing fatal errors on sites on upgrade. Lame.

You are correct that the null value isn't possible. I must have misread the docs. (Correction: Not my patch; pay attention Chris)

I was about to submit a patch for the inval issue which I just noticed, but it looks like it was already caught.

Regarding the function_exists check for memory_get_usage, this was removed because it doesn't do anything. I disabled memory_get_usage on my local system, and it caused no problems with the code supplied by the patch.