Memory Comparison Broken / WordPress Memory Limit

[hakre]

WordPress falsely assume that PHP memory limit values can be normalized as an integer value and then compared to each other:

	// set memory limits.
	if ( function_exists('memory_get_usage') && ( (int) @ini_get('memory_limit') < abs(intval(WP_MEMORY_LIMIT)) ) )
		@ini_set('memory_limit', WP_MEMORY_LIMIT);

/wp-includes/default-constants.php line 37

With a memory-limit setting of one gigabyte ('1G') to give an example, wordpress will reduce this to 32 Megabyte (Single Site) or 64 Megabyte (Multisite).

PHP Memory Values are often used with the shorthand notation ​as documented.

[hakre]

Patch is based on a code example how to convert the values to numeric values (numeric values that can be compared to each other) from the PHP Manual:

function return_bytes($val) {
    $val = trim($val);
    $last = strtolower($val[strlen($val)-1]);
    switch($last) {
        // The 'G' modifier is available since PHP 5.1.0
        case 'g':
            $val *= 1024;
        case 'm':
            $val *= 1024;
        case 'k':
            $val *= 1024;
    }

    return $val;
}

Source: ​http://www.php.net/manual/en/function.ini-get.php

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 )

[hakre]

Note: it probably could be used wp_convert_hr_to_bytes() /wp-admin/includes/template.php if it would have been already loaded.

[hakre]

I ran over that while in support I needed to deal with some of the ​Mysteries about the WordPress Memory Limit. I finally could report it.

The other example where this fails is if you setup your blog with the K notation. Like for eight megabytes: 8192k. Wordpress would not raise the limit to 32 Megabytes then.

[hakre]

Memory Related: #13847

[dd32]

For the purposes of memory calculation, i don't think converting to kilobytes / taking into account a kilobyte value is a required process.

An anonymous function such as that seems overkill as well.

$in = '1G';
$gigabytes = ('g' == strtolower( substr($in, -1) ) );
$out = (int)$in;
if ( $gigabytes )
  $out *= 1024;

echo $out; //in MB

To make our life simpler, we can also require that WP_MEMORY_LIMIT be specified in M, This'll reduce the ammount of operations required on each page load.

[hakre]

dd32 I think you have not understood the problem. The value read out from the PHP configuration is not normalized properly to compare against anything else.

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.

If you don't like a anonymous function you can create a global one which is dealing with that as well, the anononymous function was just a suggestion.

I suggest to commit and it can be improved later on. So at least the bug is fixed.

[dd32]

dd32 I think you have not understood the problem.

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.

[hakre]

+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.

I already updated my ​WordPress Technical Installation Checklist as it looks like that this bug (in terms of strict data handling) is never been fixed.

---

I my eyes it's even more important right now to remove the 256M limitation by making that magic number configureable (at least), see #13847.

[chrisbliss18]

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.

[hakre]

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.

[chrisbliss18]

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.

[hakre]

Replying to chrisbliss18:

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.

[chrisbliss18]

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:

wp_function
==========================
Trials:           250247
Total Time:       39.26492857933
Average Time:     0.00015690469248115
Runs per Sec:     6373.2956878911
Success Rate:     0.5
Speed Difference: 0

patch_function_original
==========================
Trials:           249581
Total Time:       95.880811691284
Average Time:     0.00038416711084291
Runs per Sec:     2603.0338667094
Success Rate:     1
Speed Difference: 1.4418944630847

patch_function_new
==========================
Trials:           250765
Total Time:       53.182673454285
Average Time:     0.00021208172374249
Runs per Sec:     4715.1634867614
Success Rate:     1
Speed Difference: 0.35445740966611

patch_function_new_complete
==========================
Trials:           249407
Total Time:       52.663097858429
Average Time:     0.00021115324693545
Runs per Sec:     4735.896864071
Success Rate:     1
Speed Difference: 0.34122484781881

[chrisbliss18]

Improved patch with complete conversion support

[chrisbliss18]

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.

[hakre]

Nice you catched -1, I knew it exsited, but must admit, I never put it into thought for the code but indeed it's needed.

Quickly scanned your last patch, chrisbliss18-patch.2.diff​.

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):

$map = array('g' => 1073741824, 'm' => 1048576, 'k'=> 1024);
$memory_limit *= isset($map[$unit]) ? $map[$unit] : 1;
$wp_memory_limit *= isset($map[$wp_unit]) ? $map[$wp_unit] : 1;

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.

My 2 cents for the latest patch.

[aaroncampbell]

Just thought I'd throw my 2c in on how I usually translate memory numbers like this.

$symbol = array('B', 'K', 'M', 'G');
$memory_limit *= pow(1024, array_search(strtoupper($unit), $symbol));

The advantage obviously being that its easily scalable just by adding (in order) new memory labels to the $symbol array. I've used this method for quite some time in my efficient related posts plugin. You can see the actual code here: ​http://plugins.trac.wordpress.org/browser/efficient-related-posts/trunk/efficient-related-posts.php#L624

[chrisbliss18]

I ran some benchmarks with the code provided by Hakre and Aaron. The updated benchmark script is attached.

With a total of 1,000,000 runs spread between the seven different function versions that have cropped up in this ticket, I get the following results:

wp_function
==========================
Trials:           143735
Total Time:       28.980546236038
Average Time:     0.00020162483901651
Runs per Sec:     4959.7063778343
Success Rate:     0.5
Speed Difference: 0

patch_function_original
==========================
Trials:           142930
Total Time:       70.426205396652
Average Time:     0.00049273214438293
Runs per Sec:     2029.5002292825
Success Rate:     0.75
Speed Difference: 1.4301200130271

patch_function_new
==========================
Trials:           142482
Total Time:       36.730138063431
Average Time:     0.00025778791751541
Runs per Sec:     3879.1577574237
Success Rate:     1
Speed Difference: 0.26740668599806

patch_function_new_complete
==========================
Trials:           142732
Total Time:       34.809070825577
Average Time:     0.0002438771321468
Runs per Sec:     4100.4254527565
Success Rate:     1
Speed Difference: 0.20111852075068

wp_convert_hr_to_bytes
==========================
Trials:           142653
Total Time:       46.752933263779
Average Time:     0.0003277388716941
Runs per Sec:     3051.2096256112
Success Rate:     0.625
Speed Difference: 0.61325231356889

map
==========================
Trials:           142673
Total Time:       40.819175481796
Average Time:     0.00028610301515911
Runs per Sec:     3495.2445343642
Success Rate:     1
Speed Difference: 0.40850262618709

aaron
==========================
Trials:           142795
Total Time:       56.10250377655
Average Time:     0.00039288843290417
Runs per Sec:     2545.2518227838
Success Rate:     1
Speed Difference: 0.93586771345204

Note: My current patch is represented by patch_function_new_complete.

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.

[hakre]

wow, much news ;)

@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).

[chrisbliss18]

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.

[aaroncampbell]

@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:

function bytes_from_memory_string( $mem ) {
	$unit = strtolower( substr( $mem, -1 ) );
	if ( 'm' == $unit )
		return $mem * 1048576;
	else if ( 'g' == $unit )
		return $mem * 1073741824;
	else if ( 'k' == $unit )
		return $mem * 1024;
	return $mem;
}

$memory_limit = @ini_get( 'memory_limit' );

if ( $memory_limit > -1 ) {
	if ( (float) bytes_from_memory_string( $memory_limit ) < (float) bytes_from_memory_string( WP_MEMORY_LIMIT ) )
		@ini_set( 'memory_limit', WP_MEMORY_LIMIT );
}

Also, casting to int caused the 7.5G test to fail on several of my test environments. As you can see above, casting to float fixed that issue for me.

[hakre]

@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.

[hakre]

FYI: I've been running over an integer overflow in wp_convert_hr_to_bytes(), see #17725

[hakre]

I had the idea of using a string as a map to calculate the values but didn't know if it would work out. I could test now the following routine while I was testing for #17725:

function wp_convert_hr_to_bytes( $size ) {
	$bytes = (float) $size;
	if ( $bytes ) {
		$last = strtolower( substr( $size, -1 ) );
		$pos = strpos( ' kmg', $last , 1 );
		if ( $pos ) $bytes *= pow( 1024, $pos );
	}
	return round( $bytes );
}

It uses the proposed lookup with substr(-1) and the pow() function based on the mapping. strpos will return false if 'k', 'm' or 'g' is not found in $last. Otherwise $pos is the exponent of 1024.

I could not run a speed comparison so far, however it reduces the number of ifs and I think strpos is quite fast, probably even faster than looking up key hashes in the array structure.

I added a round() at the end as there are no half bytes. The function will always return a float.

See ticket:17725:17725.2.diff​

Update: substr(-1) issues a warning on empty string, see ticket:17725:17725.3.diff​

[hakre]

If someone is in for a speed test, I'm just curious how this expression perfoms:

function wp_convert_hr_to_bytes( $size ) {
	( $bytes = (float) $size )
		&& ( $last = strtolower( substr( $size, -1 ) ) )
		&& ( $pos = strpos( ' kmg', $last , 1 ) )
		&& $bytes *= pow( 1024, $pos )
		;
	return round( $bytes );
}

[hakre]

I was looking into int size based on system:

32bit: 2 147 483 647
64bit: 9 223 372 036 854 775 807

2g: 2 147 483 648
1g: 1 073 741 824
1m: 1 048 576
1k: 1 024
1b: 1

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)

See ​intval(), ​PHP integers.

[hakre]

Updated map(), added error check, added boundary integer values

[hakre]

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.

These results are on a 32bit windows system, 1000000 iterations:

wp_function   => 32M (32M)  (no errors) Success
wp_function  2147483648 => 2147483648 (2147483648)  (no errors) Success
wp_function  4294967296 => 4294967296 (4294967296)  (no errors) Success
wp_function  -1 => -1 (32M)  (no errors) Fail
wp_function  100 => 32M (100)  (no errors) Fail
wp_function  1m => 32M (32M)  (no errors) Success
wp_function  7.5G => 7.5G (32M)  (no errors) Fail
wp_function  234k => 32M (234k)  (no errors) Fail
wp_function  94556M => 94556M (94556M)  (no errors) Success
wp_function  128 M => 128 M (128 M)  (no errors) Success
wp_function  52428800 => 52428800 (52428800)  (no errors) Success

patch_function_original   => 32M (32M) (1 errors!) Success
 #0: 8 Uninitialized string offset: -1 [...]chrisbliss18-patch-benchmark.2.php(25)
 : runtime-created function
patch_function_original  2147483648 => 2147483648 (32M)  (no errors) Fail
patch_function_original  4294967296 => 4294967296 (32M)  (no errors) Fail
patch_function_original  -1 => -1 (32M)  (no errors) Fail
patch_function_original  100 => 32M (32M)  (no errors) Success
patch_function_original  1m => 32M (32M)  (no errors) Success
patch_function_original  7.5G => 7.5G (32M)  (no errors) Fail
patch_function_original  234k => 32M (32M)  (no errors) Success
patch_function_original  94556M => 94556M (94556M)  (no errors) Success
patch_function_original  128 M => 128 M (128 M)  (no errors) Success
patch_function_original  52428800 => 52428800 (32M)  (no errors) Fail

patch_function_new   => 32M (32M)  (no errors) Success
patch_function_new  2147483648 => 2147483648 (2147483648)  (no errors) Success
patch_function_new  4294967296 => 4294967296 (4294967296)  (no errors) Success
patch_function_new  -1 => -1 (-1)  (no errors) Success
patch_function_new  100 => 32M (32M)  (no errors) Success
patch_function_new  1m => 32M (32M)  (no errors) Success
patch_function_new  7.5G => 7.5G (7.5G)  (no errors) Success
patch_function_new  234k => 32M (32M)  (no errors) Success
patch_function_new  94556M => 94556M (94556M)  (no errors) Success
patch_function_new  128 M => 128 M (128 M)  (no errors) Success
patch_function_new  52428800 => 52428800 (52428800)  (no errors) Success

patch_function_new_complete   => 32M (32M)  (no errors) Success
patch_function_new_complete  2147483648 => 2147483648 (2147483648)  (no errors) Success
patch_function_new_complete  4294967296 => 4294967296 (4294967296)  (no errors) Success
patch_function_new_complete  -1 => -1 (-1)  (no errors) Success
patch_function_new_complete  100 => 32M (32M)  (no errors) Success
patch_function_new_complete  1m => 32M (32M)  (no errors) Success
patch_function_new_complete  7.5G => 7.5G (32M)  (no errors) Fail
patch_function_new_complete  234k => 32M (32M)  (no errors) Success
patch_function_new_complete  94556M => 94556M (94556M)  (no errors) Success
patch_function_new_complete  128 M => 128 M (128 M)  (no errors) Success
patch_function_new_complete  52428800 => 52428800 (52428800)  (no errors) Success

wp_convert_hr_to_bytes   => 32M ()  (no errors) Fail
wp_convert_hr_to_bytes  2147483648 => 2147483648 (2147483648)  (no errors) Success
wp_convert_hr_to_bytes  4294967296 => 4294967296 (4294967296)  (no errors) Success
wp_convert_hr_to_bytes  -1 => -1 (-1)  (no errors) Success
wp_convert_hr_to_bytes  100 => 32M (100)  (no errors) Fail
wp_convert_hr_to_bytes  1m => 32M (1m)  (no errors) Fail
wp_convert_hr_to_bytes  7.5G => 7.5G (7.5G)  (no errors) Success
wp_convert_hr_to_bytes  234k => 32M (234k)  (no errors) Fail
wp_convert_hr_to_bytes  94556M => 94556M (94556M)  (no errors) Success
wp_convert_hr_to_bytes  128 M => 128 M (128 M)  (no errors) Success
wp_convert_hr_to_bytes  52428800 => 52428800 (52428800)  (no errors) Success

map   => 32M (32M)  (no errors) Success
map  2147483648 => 2147483648 (2147483648)  (no errors) Success
map  4294967296 => 4294967296 (4294967296)  (no errors) Success
map  -1 => -1 (-1)  (no errors) Success
map  100 => 32M (32M)  (no errors) Success
map  1m => 32M (32M)  (no errors) Success
map  7.5G => 7.5G (7.5G)  (no errors) Success
map  234k => 32M (32M)  (no errors) Success
map  94556M => 94556M (94556M)  (no errors) Success
map  128 M => 128 M (128 M)  (no errors) Success
map  52428800 => 52428800 (52428800)  (no errors) Success

aaron   => 32M (32M)  (no errors) Success
aaron  2147483648 => 2147483648 (32M)  (no errors) Fail
aaron  4294967296 => 4294967296 (32M)  (no errors) Fail
aaron  -1 => -1 (-1)  (no errors) Success
aaron  100 => 32M (32M)  (no errors) Success
aaron  1m => 32M (32M)  (no errors) Success
aaron  7.5G => 7.5G (32M)  (no errors) Fail
aaron  234k => 32M (32M)  (no errors) Success
aaron  94556M => 94556M (94556M)  (no errors) Success
aaron  128 M => 128 M (128 M)  (no errors) Success
aaron  52428800 => 52428800 (52428800)  (no errors) Success

str   => 32M (32M)  (no errors) Success
str  2147483648 => 2147483648 (2147483648)  (no errors) Success
str  4294967296 => 4294967296 (4294967296)  (no errors) Success
str  -1 => -1 (-1)  (no errors) Success
str  100 => 32M (32M)  (no errors) Success
str  1m => 32M (32M)  (no errors) Success
str  7.5G => 7.5G (7.5G)  (no errors) Success
str  234k => 32M (32M)  (no errors) Success
str  94556M => 94556M (94556M)  (no errors) Success
str  128 M => 128 M (128 M)  (no errors) Success
str  52428800 => 52428800 (52428800)  (no errors) Success

Running 1000000 iterations now... - done.

Results:

wp_function
==========================
Trials:           124892
Total Time:       154.32998085022
Average Time:     0.0012357074980801
Runs per Sec:     809.25300004547
Success Rate:     0.63636363636364
Speed Difference: 0

patch_function_original
==========================
Trials:           125286
Total Time:       390.26043176651
Average Time:     0.0031149564338115
Runs per Sec:     321.03177724909
Success Rate:     0.45454545454545
Speed Difference: 1.5287402332102

patch_function_new
==========================
Trials:           124484
Total Time:       194.98491120338
Average Time:     0.0015663451624577
Runs per Sec:     638.42888781355
Success Rate:     1
Speed Difference: 0.26342859714744

patch_function_new_complete
==========================
Trials:           124900
Total Time:       195.32537388802
Average Time:     0.0015638540743636
Runs per Sec:     639.44585137007
Success Rate:     0.90909090909091
Speed Difference: 0.26563466678314

wp_convert_hr_to_bytes
==========================
Trials:           124857
Total Time:       284.27002906799
Average Time:     0.0022767648515341
Runs per Sec:     439.21971095355
Success Rate:     0.63636363636364
Speed Difference: 0.84196244632391

map
==========================
Trials:           125201
Total Time:       194.24108624458
Average Time:     0.0015514339841102
Runs per Sec:     644.56497037064
Success Rate:     1
Speed Difference: 0.25860889228709

aaron
==========================
Trials:           125004
Total Time:       319.43641614914
Average Time:     0.0025554095560873
Runs per Sec:     391.32670440942
Success Rate:     0.72727272727273
Speed Difference: 1.0698273555749

str
==========================
Trials:           125376
Total Time:       253.95498728752
Average Time:     0.0020255470527655
Runs per Sec:     493.69378935666
Success Rate:     1
Speed Difference: 0.64553242272472

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.

[nacin]

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.

[hakre]

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.

[nacin]

Please pitch a patch that allows WP_MEMORY_LIMIT to be set to -1. That seems fine.

The comparisons and what not are not a bug. We can handle those in a future release.

[aaroncampbell]

Replying to nacin:

Please pitch a patch that allows WP_MEMORY_LIMIT to be set to -1. That seems fine.

I did very minimal testing, but it adds support for -1 in WP_MEMORY_LIMIT

[nacin]

At a glance, looks good.

[hakre]

Jup, -1 is taken care of now, if check for null is added on front, it's dealt with the error case of @ini_get as well:

    if ( null !== $current_limit && -1 != $current_limit && [...]

My 2 cents.

[hakre]

Added check for null / hidden warning on @ini_get

[hakre]

FYI: ​memory-get-usage():

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

[hakre]

minus memory_get_usage() check

[nacin]

Replying to hakre:

FYI: ​memory-get-usage():

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.

[hakre]

Replying to nacin:

Replying to hakre:

FYI: ​memory-get-usage():

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.

Excerpt from the original ticket:

ruckus:

Why does the code in [6681] check for the memory_get_usage() function to exist? That function is not used.

ryan:

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.

[aaroncampbell]

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.

[hakre]

Replying to aaroncampbell:

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.

Okay, then I think this information is helpful then:

​memory_limit - Description of core php.ini directives:

memory_limit integer

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][...]

[scribu]

Could we please fix the -1 limit issue already?

Either 14889.2.diff​ or 14889.4.diff​ will do.

[nacin]

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.

Going with 14889.2.diff​.

[nacin]

In [21165]:

Respect -1 as a memory limit setting.

Don't override memory_limit = -1 with a fixed value.
Know that WP_MEMORY_LIMIT = -1 can override a fixed value.

props aaroncampbell
fixes #14889

[nacin]

In [21168]:

s/inval/intval/. props PeteMall. see #14889.

[chrisbliss18]

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.