Skip to content

Fix #81585: cached_chunks are not counted to real_size on shutdown - #7745

Closed
cmb69 wants to merge 1 commit into
php:PHP-8.0from
cmb69:cmb/81585
Closed

cmb69 wants to merge 1 commit into
php:PHP-8.0from
cmb69:cmb/81585

Conversation

@cmb69

@cmb69 cmb69 commented Dec 9, 2021

Copy link
Copy Markdown
Member

The amount of allocated system memory is kept in real_size, including
the allocated cached_chunks. Thus, we need to keep the proper count
at the end of the shutdown.

The amount of allocated system memory is kept in `real_size`, including
the allocated `cached_chunks`.  Thus, we need to keep the proper count
at the end of the shutdown.
@dstogov

dstogov commented Dec 9, 2021

Copy link
Copy Markdown
Member

I think, it's right now.

@cmb69 cmb69 closed this in 5675ebe Dec 10, 2021
@cmb69
cmb69 deleted the cmb/81585 branch December 10, 2021 11:30
@pvandommelen

Copy link
Copy Markdown
Contributor

@cmb69 @dstogov I believe this change has had an (unforeseen?) side effect which can be relevant in some scenarios. Within a single php worker process, a previous request will now more directly impact the amount of memory available in the following request.

I believe the new behaviour is more correct, as the memory reported now more accurately reports the actual consumed memory from the OS. Our code can also be adapted to handle this new behaviour, probably by taking the currently available memory into account. So there is probably nothing actionable (except maybe more general improvements to the memory manager), but this reply can maybe be useful as additional documentation.

In our case we call ini_set('memory_limit', ...) on every request, where we expect each request's memory usage to start below the default memory limit. Changing the memory limit makes a check against the currently allocated memory (since #7040). This wasn't a problem before the change of this MR, but with this change it will randomly fail when a previous request happened to consume a lot of memory which wasn't entirely collected.

The following script illustrates the issue. Works with apache's mod_php + mpm_prefork (making sure only 1 worker runs), but is more easily tested with php's internal webserver (php -S).

header('Content-Type:text/plain');
ini_set('display_errors', 1);

echo sprintf("\nmemory usage: %.3f MB", memory_get_usage(true) / 1024 / 1024);
ini_set('memory_limit', '64M');

ini_set('memory_limit', '1G');
for ($i = 0; $i < 100; ++$i) {
	// create an object which isn't immediately garbage collected by making it reference itself
	$a = new \StdClass();
	$a->a = $a;
	// have it consume memory. Behaviour does not appear to trigger when this string is 2 MB or larger,
	// possibly because it can be more easily be collected when it exceeds ZEND_MM_CHUNK_SIZE?
	$a->b = str_repeat('a', 1024*1024*1);
}
echo sprintf("\nmemory usage: %.3f MB", memory_get_usage(true) / 1024 / 1024);

gc_collect_cycles();
echo sprintf("\nmemory usage: %.3f MB", memory_get_usage(true) / 1024 / 1024);

PHP 8.0.14, the memory reported goes down for each executed request (which is incorrect):

memory usage: 2.000 MB
memory usage: 200.000 MB
memory usage: 4.000 MB

memory usage: 2.000 MB
memory usage: 198.000 MB
memory usage: 100.000 MB

memory usage: 2.000 MB
memory usage: 100.000 MB
memory usage: 52.000 MB

memory usage: 2.000 MB
memory usage: 50.000 MB
memory usage: 26.000 MB

memory usage: 2.000 MB
memory usage: 14.000 MB
memory usage: 10.000 MB

...

memory usage: 2.000 MB
memory usage: 2.000 MB
memory usage: 2.000 MB

PHP 8.0.15. Memory reset at start will fail when the memory of the previous request is not collected.

memory usage: 2.000 MB
memory usage: 200.000 MB
memory usage: 4.000 MB

memory usage: 4.000 MB
memory usage: 200.000 MB
memory usage: 102.000 MB

memory usage: 102.000 MB<br />
<b>Warning</b>:  Failed to set memory limit to 67108864 bytes (Current memory usage is 106954752 bytes) in <b>/home/peter/projects/objectenregister/memtest.php</b> on line <b>6</b><br />
memory usage: 200.000 MB
memory usage: 152.000 MB

memory usage: 152.000 MB<br />
<b>Warning</b>:  Failed to set memory limit to 67108864 bytes (Current memory usage is 159383552 bytes) in <b>/home/peter/projects/objectenregister/memtest.php</b> on line <b>6</b><br />
memory usage: 200.000 MB
memory usage: 176.000 MB

...

memory usage: 200.000 MB<br />
<b>Warning</b>:  Failed to set memory limit to 67108864 bytes (Current memory usage is 209715200 bytes) in <b>/home/peter/projects/objectenregister/memtest.php</b> on line <b>6</b><br />
memory usage: 200.000 MB
memory usage: 200.000 MB

@dstogov

dstogov commented Feb 7, 2022

Copy link
Copy Markdown
Member

I hope #8053 should fix the problem

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants