WordPress / wordpress-playground

Run WordPress in the browser via WebAssembly PHP
https://w.org/playground/
GNU General Public License v2.0
1.63k stars 249 forks source link

PHP memory leak #1128

Closed adamziel closed 6 months ago

adamziel commented 6 months ago

The issue below was reported by @sejas. It negatively impacts user experience of Playground and wp-now. Let's make sure:

A few memory leaks were already patched in this repo, find old PRs for more context.

pm.max_requests int The number of requests each child process should execute before respawning. This can be useful to work around memory leaks in 3rd party libraries. For endless request processing specify '0'. Equivalent to PHP_FCGI_MAX_REQUESTS. Default value: 0.


What @sejas reported:

I've created a PHP function to make it much easier to benchmark the memory usage: I'm currently adding it to the index.php, but you can also try the plugin in Studio or wp-now.

function useAllMemory() {
    echo "Initial memory usage: " . (memory_get_usage()/(1024*1024)) . ' MB <br>';
    // ini_set('memory_limit', '1024MB'); // The php limit seems to be 128MB but it doesn't affect the results.
    $data = '';

    while (true) {
        $data .= str_repeat('a', 1024 * 1024); // Increase string size by 1MB in each iteration
        echo "* " . (memory_get_usage()/(1024*1024)) . ' MB <br>';
    }
}
useAllMemory();
die();

Here are my results:

Observe that the site screenshot displays almost 60MB of maximum memory. The next (2nd) page load displays 29MB The third page load 4.4MB and then 2.4 MB and 1.3 MB

Screenshot:

315332005-ef2eabc9-ac83-4762-b460-18f3cbd763cd

cc @brandonpayton

brandonpayton commented 6 months ago

@adamziel I haven't finished testing yet, but updating wasm_memory_storage to zero allocated memory looks like it might solve the issue. Apparently many mmap() implementations zero memory for anonymous mmap. And PHP may depend on this behavior.

I'm currently rebuilding all php-wasm versions for a real PR, but you can check out https://github.com/WordPress/wordpress-playground/pull/1220/commits/c304fecfdda29149dd489c1afa7a8978c8486b16 under #1220 for the potential fix. The unit tests are passing there, but the e2e tests are not. I'm not yet sure if the failures are related to the memory leak fix.

brandonpayton commented 6 months ago

We merged a potential fix in #1229. Let's see how it goes and close this if all is indeed well.

adamziel commented 6 months ago

@brandonpayton with #1229 merged, the original reproduction scenario still triggers the out of memory error in a browser. I can see the node.js test is passing, that's weird!

brandonpayton commented 6 months ago

@adamziel the original reproduction scenario always triggers an out of memory error because it uses an infinite loop that concatenates strings. Is this what you are testing with?

    while (true) {
        $data .= str_repeat('a', 1024 * 1024); // Increase string size by 1MB in each iteration
        echo "* " . (memory_get_usage()/(1024*1024)) . ' MB <br>';
    }

I wish we could see the output prior to the fatal, but now, the error is just printed to a console with no output shown. I will see if we can change that back so we actually see output prior to error.

brandonpayton commented 6 months ago

I wish we could see the output prior to the fatal, but now, the error is just printed to a console with no output shown. I will see if we can change that back so we actually see output prior to error.

It looks like that behavior probably has to do with this PR: https://github.com/WordPress/wordpress-playground/pull/1137

The main PHP script output used to print in the browser, but now, when the script exits with an error, the error is printed to the console with no partial response shown in the content pane. For at least the web interface, it seems like showing a partial response to the user is more helpful than showing nothing. Planning to file a bug for this. cc @bgrgicak

brandonpayton commented 6 months ago

@adamziel, it turns out I am able to reproduce this failure as well with the original repo and the web version. It fails after making a 50MB string, even though there should be much more memory available. Digging into this...

brandonpayton commented 6 months ago

@adamziel, I think what we are now seeing is expected. When I test in the browser, the PHP memory limit is 128M, and the "Allowed memory size exhausted" happens when the attempted allocation would surpass the allowed memory size.

From the end of the test:

* memory_get_usage(true): 74.0625 MB

Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 57671704 bytes) in /wordpress/index.php on line 14

The limit is 128M, and the new allocation would add ~55MB to about 75MB making about 130MB, which exceeds the configured limit and triggers the error.

Full log: https://gist.github.com/brandonpayton/d4239a0da6828647ed73770da95db043


Related to the memory limit, do we expect it to be 128M, and if so, would it make sense to choose a higher default?

adamziel commented 6 months ago

What, whaaaat, you're right! I increased the memory_limit and I was able to get that loop to allocate 200M – yay! Exemplary work on this one @brandonpayton! 🎉 Let's close and discuss the larger memory limit separately – we might want to also consider lowering the overall HEAP size, I think it's ~2GB now?

adamziel commented 6 months ago

Let's follow up in these:

sejas commented 6 months ago

@brandonpayton You are awesome!! I think this was one of the hardest problems to solve. ❤️

I tested it on NodeJS and I can see each request allocates always the maximum memory.

    $data = '';
    $i = 0;
    while ($i++ < 50) {
        $data .= str_repeat('a', 1024 * 1024); // Increase string size by 1MB in each iteration until a maximum of 50MB
        echo "* " . (memory_get_usage()/(1024*1024)) . ' MB <br>';
    }

Thank you!!

brandonpayton commented 5 months ago

Thank you, @sejas!