Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP memory leak #1128

Closed
adamziel opened this issue Mar 21, 2024 · 61 comments · Fixed by #1189
Closed

PHP memory leak #1128

adamziel opened this issue Mar 21, 2024 · 61 comments · Fixed by #1189
Assignees
Labels
[Feature] PHP.wasm [Type] Bug An existing feature does not function as intended

Comments

@adamziel
Copy link
Collaborator

adamziel commented Mar 21, 2024

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

  • PHP has ample memory limit available. Confirm php.ini defaults to, say, 1GB memory.
  • php_wasm.c doesn't contribute to the memory leak problem. PHP itself suffers from one, see pm.max_requests. Is php_wasm.c making it worse?
  • PHP runtime is automatically rotated when an out of memory error is detected AND more than 10 requests were handled to avoid an "infinite rotate trap". This could rely on the error logging API implemented by @bgrgicak.

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

@adamziel adamziel added [Type] Bug An existing feature does not function as intended [Feature] PHP.wasm labels Mar 21, 2024
@adamziel adamziel added this to the Zero Crashes milestone Mar 21, 2024
@adamziel adamziel changed the title Memory leak PHP memory leak Mar 21, 2024
@brandonpayton brandonpayton self-assigned this Mar 22, 2024
@adamziel
Copy link
Collaborator Author

adamziel commented Mar 22, 2024

Relevant unit test:

it('Frees up the heap memory after handling a request body with a size of ~512MB', async () => {

A historical PR that added that test:

#993

@adamziel
Copy link
Collaborator Author

adamziel commented Mar 22, 2024

I checked a few quick ideas to give the next person some entrypoints and potentially save a few hours of looking for these:

I just quickly tested whether increasing the memory_limit helps at all and it doesn't seem to:

http://localhost:5400/website-server/?url=/crash.php

diff --git a/packages/playground/remote/src/lib/worker-thread.ts b/packages/playground/remote/src/lib/worker-thread.ts
index cfc77fea2..06b78152d 100644
--- a/packages/playground/remote/src/lib/worker-thread.ts
+++ b/packages/playground/remote/src/lib/worker-thread.ts
@@ -241,6 +241,7 @@ const [setApiReady, setAPIError] = exposeAPI(
 
 try {
 	php.initializeRuntime(await recreateRuntime());
+	php.setPhpIniEntry('memory_limit', '256M');
 
 	if (startupOptions.sapiName) {
 		await php.setSapiName(startupOptions.sapiName);
@@ -286,6 +287,20 @@ try {
 		'playground-includes/wp_http_dummy.php': transportDummy,
 		'playground-includes/wp_http_fetch.php': transportFetch,
 	});
+	php.writeFile('/wordpress/crash.php', `<?php
+	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();`)
+
 
 	if (virtualOpfsDir) {
 		await bindOpfs({

Which is weird considering how:

var getHeapMax = () => 2147483648;

and

-s INITIAL_MEMORY=1024MB \
-s ALLOW_MEMORY_GROWTH=1 \

I was, however, able to use a bit more memory before crashing by not extracting WordPress files into MEMFS – I think HEAP is used to store their content:

@adamziel
Copy link
Collaborator Author

I tried two more things:

php[__private__dont__use].malloc(1024 * 1024 * 512);

and

Neither seemed to have a significant effect on the size of the string PHP is able to allocate:

* 49.415100097656 MB
* 50.415100097656 MB
* 51.415100097656 MB
* 52.415100097656 MB
(crash)

HEAP size is not an issue here, then. It must be that PHP thinks it runs out of memory when it actually doesn't.

@brandonpayton
Copy link
Member

@adamziel Thanks for doing all this work to help narrow the possibilities.

@brandonpayton brandonpayton moved this to In progress in Playground Board Mar 22, 2024
@brandonpayton
Copy link
Member

I am looking at whether we can just build PHP with debug info and step through it in the browser. This page suggests it is possible at least sometimes:
https://developer.chrome.com/docs/devtools/wasm

So far, I've asked emscripten to build with debug info via the -g flag, and php_8_0.wasm is now 21MB instead of ~5MB. So there is something more there. The OOM is still happening with this build which is good.

Now I'm looking at whether we can step through it in Chrome. The original source files may be required, and that might be a challenge since the build is done within Docker. We'll see.

@brandonpayton
Copy link
Member

The original source files may be required, and that might be a challenge since the build is done within Docker. We'll see.

With Chrome Canary, after tweaking the Dockerfile to add the -g flag and disable optimization with -O0, I've gotten the dev tools listing PHP's source files, but it is indeed missing actual file content since it is referring to the php-src path within the Docker build.

Screenshot 2024-03-23 at 12 01 21 PM

With some tweaks we should be able to fix this. Then we can hopefully step through the debugger to observe where and why PHP thinks it is out of memory.

@adamziel
Copy link
Collaborator Author

adamziel commented Mar 23, 2024

@brandonpayton check -g3 and the SOURCE_MAPS option in the Dockerfile. Entry to the rabbit hole: #639

@brandonpayton
Copy link
Member

brandonpayton commented Mar 23, 2024

@adamziel thanks for the pointer. I only ran into source map things in the Dockerfile after beginning to add DWARF info. Then I saw something that suggested the DWARF route would offer richer info while debugging. That promise hasn't panned out yet, but I'm working on it.

Thankfully, to address the missing source files, the debugging extension has a setting to map paths, designed for cases like ours (They even mentioned Docker-based builds : ).
https://developer.chrome.com/blog/wasm-debugging-2020/#path-mapping

Here's what I'm seeing so far with the crash.
Screenshot 2024-03-23 at 1 31 46 PM

We get the call stack which may not be super useful. But it does help us identify where the "Allowed memory size" error is occurring (there are 3-4 places in PHP 8.0 where that error is reported). Unfortunately, access to variable values is infrequent and rare, so I'm working on that. It's impressive that the reflected call stack includes the JS functions that invoked the Wasm export.

@brandonpayton
Copy link
Member

brandonpayton commented Mar 23, 2024

I tried using the latest emscripten version to see whether using a newer version might lead to more complete debug info, but the variable values are still largely unavailable. One interesting thing is that, after building PHP 8.0 with latest emcc, the error changed to simply "Out of memory", which comes a bit later than the error seen above. It seems to indicate that the limit checking said everything was OK but the actual allocation failed. This might just be due to changing emscripten options/behavior leading to different preprocessor behavior during build (i.e., maybe the memory limit checking didn't run at all).

Either way, we don't have to have var values in the debugger right now. As a next step, I plan to patch the PHP 8.0 to augment the error message, including whatever info we want from within zend_mm_alloc_huge.

@adamziel
Copy link
Collaborator Author

adamziel commented Mar 23, 2024

I know it probably doesn't matter here, but I was curious and found this page suggesting that variable names in the inspector are possible: https://developer.chrome.com/docs/devtools/wasm

Also: https://www.perplexity.ai/search/Debug-wasm-variable-0L7fbRR3T0iM2LqSMnt39g

@brandonpayton
Copy link
Member

brandonpayton commented Mar 24, 2024

The first page you mentioned is the one that made me think variable names and values were possible. And I was seeing variable names in the Scope panel, but all said "undefined".

But oddly enough, the values seem to be present now too. I'd quit Chrome Canary and was restarting to take a screenshot of the undefined vars, but now values are present!
Screenshot 2024-03-23 at 9 13 59 PM

I'll see what I can learn from the values and then play with the configuration to see whether var values are possible with our current emscripten version.

@brandonpayton
Copy link
Member

I switched back to emscripten 3.1.43 and am thankfully still seeing var values in the debugger. It works best when I let the page load Playground and open the dev tools after. If dev tools are opened before loading Playground for the first time, no php-src source files are listed.

Setting memory limit to 1024MB via ini_set('memory_limit', '1024M') results in the heap structure having a limit that reflects 1024MB, but regardless, the test script still hits an OOM condition:

Fatal error: Out of memory (allocated 87097344) at /root/php-src/Zend/zend_string.h:241 (tried to allocate 62914584 bytes) in /wordpress/index.php on line 10

Planning to dig into this more on Monday.

@brandonpayton
Copy link
Member

Even without repeated str_repeat() calls, there is an OOM. It seems like there is some mis-management involving string concat.

The following version of the test script:

  • sets the PHP memory limit to 64MB.
  • Only uses str_repeat() once to create a 64KB string.
  • Appends the 64KB string to $data once for 1000 iterations.
<?php

ini_set('memory_limit', '64M');
function useAllMemory() {
    echo "Initial memory limit: " . ini_get( 'memory_limit' ) . '<br>';
    echo "Initial memory usage: " . (memory_get_usage()/(1024*1024)) . ' MB <br>';
    echo "<br>";

    $data = '';
    $counter = 0;
    $total_strlen = 0;
    $tail = str_repeat('a', 64 * 1024); // Increase string size by 64KB in each iteration
    for ($counter = 0; $counter < 1000; $counter++) {
      $data .= $tail;

      $usage = memory_get_usage();
      $strlen = strlen($data);
      $total_strlen += $strlen;
      echo "* iteration: $counter <br>";
      echo "* strlen(): " . ($strlen/(1024*1024)) . " MB <br>";
      echo "* memory_get_usage(): " . ($usage/(1024*1024)) . " MB <br>";
      echo "* aggregate strlen()'s: " . ($total_strlen/(1024*1024)) . " MB <br>";
      echo "<br>";
    }
}
useAllMemory();
echo "Completed without OOM<br>";
die();

This script printed the following results:

* iteration: 0
* strlen(): 0.0625 MB
* memory_get_usage(): 0.43998718261719 MB
* aggregate strlen()'s: 0.0625 MB

* iteration: 1
* strlen(): 0.125 MB
* memory_get_usage(): 0.56889343261719 MB
* aggregate strlen()'s: 0.1875 MB

* iteration: 2
* strlen(): 0.1875 MB
* memory_get_usage(): 0.63139343261719 MB
* aggregate strlen()'s: 0.375 MB

... iterations 3 - 220 ...

* iteration: 221
* strlen(): 13.875 MB
* memory_get_usage(): 14.377540588379 MB
* aggregate strlen()'s: 1547.0625 MB

* iteration: 222
* strlen(): 13.9375 MB
* memory_get_usage(): 14.440040588379 MB
* aggregate strlen()'s: 1561 MB

* iteration: 223
* strlen(): 14 MB
* memory_get_usage(): 14.502540588379 MB
* aggregate strlen()'s: 1575 MB

Fatal error: Out of memory (allocated 39911424) at /root/php-src/Zend/zend_string.h:241 (tried to allocate 14745624 bytes) in /wordpress/index.php on line 14

PHP reports just 14.5MB of memory used, yet somehow it is unable (or believes it is unable) to allocate more memory.

For next steps, I'm planning to:

  • Read how PHP string concat works within php-src
  • See how emscripten handles the system-related memory allocation calls
  • Look for any other way to see what the WebAssembly memory reflects. One possibility is having Playground keep a reference to a WebAssembly.Memory instance it creates rather than allowing Wasm to create its own on instantiation.

Also, planning to read this article for ideas:
Debugging memory leaks in WebAssembly using Emscripten

@brandonpayton
Copy link
Member

Read how PHP string concat works within php-src

This appears to be the function PHP calls to make room for the concatenation. I've highlighted the line requesting allocation. It is part of the call stack where the OOM occurs, and it is the furthest up the stack I could get before getting into PHP opcode processing. The dev tools aren't showing which opcode, or I would share that here.

https://github.com/php/php-src/blob/ff2359b62e999c38598db16e60c3cc6818477224/Zend/zend_string.h#L271

@brandonpayton
Copy link
Member

And this is where PHP asks the OS for memory and detects a failure:
https://github.com/php/php-src/blob/ff2359b62e999c38598db16e60c3cc6818477224/Zend/zend_alloc.c#L530-L536

Now to see what Emscripten is doing with the mmap() call...

@brandonpayton
Copy link
Member

And this is where PHP asks the OS for memory

NOTE: When OOM occurs, the debugger shows that the requested memory size is about 14MB.

@brandonpayton
Copy link
Member

Memory allocated with mmap() is freed with munmap(). PHP mostly ignores failed munmap() calls, except that it logs to STDERR when ZEND_MM_ERROR is defined:
https://github.com/php/php-src/blob/ff2359b62e999c38598db16e60c3cc6818477224/Zend/zend_alloc.c#L448-L450

I went to define ZEND_MM_ERROR so we could see munmap() failures in the debugger and found this, which implies there were such failures before we silenced them:

# Silence the errors "munmap() failed: [28] Invalid argument"
# @TODO: Identify the root cause behind these errors and fix them properly
RUN echo '#define ZEND_MM_ERROR 0' >> /root/php-src/main/php_config.h;

from https://github.com/WordPress/wordpress-playground/blob/trunk/packages/php-wasm/compile/php/Dockerfile#L260-L262

Next: Confirm that these failures are occurring for this OOM and, if so, try to find out why.

@brandonpayton
Copy link
Member

Confirm that these failures are occurring for this OOM

Yep. This is what I see in the console after setting ZEND_MM_ERROR=1.
Screenshot 2024-03-25 at 9 29 34 PM

Some progress on getting visibility into Emscripten library behavior:
It turns out that mmap() and munmap() are implemented in C in the Emscripten codebase, and when building with debug info, you can step into those calls as well after setting path mappings in the extension options:
Screenshot 2024-03-25 at 10 42 43 PM

Stepping into munmap():
Screenshot 2024-03-25 at 10 41 54 PM

I haven't read or understood the implementation yet, but at least we can take a look. Planning to continue with this tomorrow.

Other notes:

  1. I briefly looked at whether we could skip using mmap() within PHP. So far, it doesn't look like there is a preprocessor flag for it, but if we really wanted to, PHP does appear to support providing your own custom memory storage API.

  2. This comment on another emscripten issue about failed munmap() says:

Is the code in question trying to partial unmapping of anonymous regions?

We don't support that, and I don't think there is any easy way to fake it. We should really be asserting in that case. We also don't support mprotect at all. We should assert there too.

PHP does appear to use anonymous regions, but I'm not yet sure whether PHP attempts partial unmapping. Something to check.

For tomorrow:

  1. Check whether PHP attempt partial unmapping of anonymous regions. If so, see if there's anything we can do to avoid it.
  2. Step through emscripten mmap() and munmap() implementations in the debugger, building understanding and looking for clues.

@adamziel
Copy link
Collaborator Author

This is top-notch work @brandonpayton!

@brandonpayton
Copy link
Member

brandonpayton commented Mar 27, 2024

❤️ thanks, @adamziel!

Check whether PHP attempt partial unmapping of anonymous regions.

I instrumented PHP 8.3 to print the address and size of each mmap() and munmap() attempt, and PHP does appear to attempt partial unmapping.

From one of the test runs (full log here), we can see the first munmap() failure occurs when attempting to unmap with a smaller size than previously mapped memory at the same address.

* iteration: 31
mmap, 0x6320000, 2162688
munmap, 0x6320000, 2162688
mmap, 0x6320000, 4194304
munmap, 0x6320000, 917504
munmap() failed: [28] Invalid argument
munmap, 0x6610000, 1114112
munmap() failed: [28] Invalid argument
* strlen(): 2 MB
* memory_get_usage(): 2.5128479003906 MB
* aggregate strlen()'s: 33 MB

Specifically, this is the first munmap() failure():

mmap, 0x6320000, 4194304
munmap, 0x6320000, 917504
munmap() failed: [28] Invalid argument

And the second munmap() failure is an attempt to partially unmap within the same memory space allocated earlier at address 0x6320000 and continuing until address 0x6720000.

munmap, 0x6610000, 1114112
munmap() failed: [28] Invalid argument

Note that 0x6610000 + 1114112 = 0x6720000.

PHP is requesting a partial unmapping, something the emscripten libs apparently don't support. And when it fails, it does nothing (but does log to stderr if ZEND_MM_ERROR is truthy).

I don't yet know what we can do about this. Here are some questions we can pursue. Do any others come to mind?

Tomorrow, I plan to start by walking through more of php-src to see if I missed other memory allocation options and to see whether providing custom memory storage is a possible solution.

@adamziel
Copy link
Collaborator Author

Is it that support cannot be reasonably implemented or that it simply hasn't been added yet?

I’ve had a similar experience with a bunch of socket reading options and they were relatively easy to add with a patch. If we’re lucky, this would also be the case here.

@brandonpayton
Copy link
Member

I’ve had a similar experience with a bunch of socket reading options and they were relatively easy to add with a patch.

That's good to hear! OK, I'll take a look at the Emscripten libs first. It would be ideal not to have to customize PHP behavior but rather have the underlying platform behave as expected.

@brandonpayton
Copy link
Member

brandonpayton commented Mar 28, 2024

Emscripten mmap() and mmap() rely upon a generic API supported by multiple allocators:

// Direct access to the system allocator.  Use these to access that underlying
// allocator when intercepting/wrapping the allocator API.  Works with with both
// dlmalloc and emmalloc.
void *emscripten_builtin_memalign(size_t alignment, size_t size);
void *emscripten_builtin_malloc(size_t size);
void emscripten_builtin_free(void *ptr);

And emscripten simulates anonymous mmap() and munmap() using emscripten_builtin_memalign() and emscripten_builtin_free() respectively. This works because Emscripten does not currently support partial munmap() of anonymous mapped regions.

But it seems like unmapping support could be added to an allocator. An allocator tracks allocated and free regions, and AFAICT, all a partial unmapping should do is take an allocated region and break it into a free region and one or more allocated regions.

Conceptually partial unmapping can do one of four things:

  • unmap the beginning of a mapped region, creating a free region followed by the remainder of the mapped region
  • unmap the end of a mapped region, leaving the remainder of the mapped region followed by a free region
  • unmap the middle of a mapped region, leaving the beginning of the mapped region followed by a free region which is followed by the remaining end of the mapped region
  • fail because the region described by the address/length is not fully contained by a single mapped region

At a high-level, this seems doable.

Doing it sounds like a lot of fun, but a simpler solution would be better.

I've spent a fair amount of time thinking about this issue, re-reading php-src code, and testing various ideas (e.g., playing with declared alignments in an attempt to avoid reasons PHP munmaps). So far, adding partial unmapping support is my only idea.

If we go ahead with this...

Our current allocator is dlmalloc, a generic malloc implementation from elsewhere that was adopted by Emscripten, and it tracks chunks using a combination of circular doubly-linked lists and a form of trees (specifically, tries). In our current version of Emscripten, its source file is ~6400 lines.

But Emscripten also has it's own minimalistic allocator called "emmalloc". It's billed as a "Simple minimalistic but efficient sbrk()-based malloc/free that works in singlethreaded and multithreaded builds.". It "subdivides regions of free space into distinct circular doubly linked lists, where each linked list
represents a range of free space blocks"
, and its source file is around 1400 lines.

If we're going to add support for partial unmapping of anonymous regions, it is probably best to try first with the simpler allocator. I'm not sure which is the better allocator for PHP/WP performance, but for a PoC, it seems better to pick the simpler starting point. In very brief testing, Emmalloc seems fine when rendering a WP home page, and if the performance is comparable for the average case, it's also the smaller implementation.

@github-project-automation github-project-automation bot moved this from In progress to Done in Playground Board Apr 4, 2024
@adamziel adamziel reopened this Apr 4, 2024
@adamziel
Copy link
Collaborator Author

adamziel commented Apr 4, 2024

I had to revert #1189 unfortunately (in #1195) because of the memory access out of bounds, see
#1194 for more details.

@brandonpayton
Copy link
Member

There are some updates on our continued investigation here and here.

The state of this issue is:

  • we know the source of the memory leak -- partial munmap() calls are used by PHP and unsupported by emscripten memory allocation
  • we are working to find a sound solution that does not cause additional errors

@brandonpayton
Copy link
Member

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

adamziel pushed a commit that referenced this issue Apr 11, 2024
This PR attempts to fix the memory leak reported in #1128 and is an
iteration on PR #1189 which had problems with "memory out of bounds"
errors.

## What problem is it solving?

It stops PHP from leaking memory throughout the runtime of a script and
hopefully stops memory out of bounds errors by zeroing all memory given
to PHP.

## How is the problem addressed?

- By avoiding mmap()/munmap() which have incomplete implementations in
Emscripten
- By using posix_memalign() to allocate memory instead and manually
zeroing the memory before handing it to PHP

## Testing Instructions

- Observe CI test results
- Use `npm run dev` and exercise Playground locally in the browser
@brandonpayton
Copy link
Member

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

@adamziel
Copy link
Collaborator Author

adamziel commented Apr 11, 2024

@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
Copy link
Member

@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
Copy link
Member

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:
#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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Collaborator Author

adamziel commented Apr 12, 2024

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 added a commit that referenced this issue Apr 12, 2024
## What is this PR doing?

Increases PHP memory limit to 256M in web browsers to unblock more
memory-hungry use-cases.

Related:

* #1128

## Testing instructions

Go to http://127.0.0.1:5400/website-server/?url=/phpinfo.php and confirm
the memory limit says `256 M`
@adamziel
Copy link
Collaborator Author

adamziel commented Apr 12, 2024

adamziel added a commit that referenced this issue Apr 12, 2024
 ## What does this PR do?

This PRrestores displaying the PHP output when a PHP error is
encountered. It does that by attaching `response` to the error thrown by
BasePHP and augmenting the Comlink transfer handler to pass that
response between workers through postMessage.

 ## What problem does it solve?

Prior to be0e783, script output was still shown for Fatal Errors. For example, when reproducing the memory-related errors under #1128, we used to see something like the following where page output was shown along with the Fatal Error that ended execution.

But now that we are no longer returning a PHPResponse when there is a non-zero exit code, the content is not updated when running the same script. The error message is printed to the console, but the partial content is not visible to the user. Instead the previous content is left in place as if the script had not run at all.

Closes #1231

 ## Testing instructions

Ensure the E2E tests pass
adamziel added a commit that referenced this issue Apr 12, 2024
## What does this PR do?

This PR restores displaying the PHP output when a PHP error is
encountered. It does that by attaching `response` to the error thrown by
BasePHP and augmenting the Comlink transfer handler to pass that
response between workers through postMessage.

 ## What problem does it solve?

Prior to be0e783, script output was still shown for Fatal Errors. For
example, when reproducing the memory-related errors under #1128, we used
to see something like the following where page output was shown along
with the Fatal Error that ended execution.

But now that we are no longer returning a PHPResponse when there is a
non-zero exit code, the content is not updated when running the same
script. The error message is printed to the console, but the partial
content is not visible to the user. Instead the previous content is left
in place as if the script had not run at all.

Closes #1231

 ## Testing instructions

Ensure the E2E tests pass
@sejas
Copy link
Collaborator

sejas commented Apr 12, 2024

@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
Copy link
Member

Thank you, @sejas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] PHP.wasm [Type] Bug An existing feature does not function as intended
Projects
Archived in project
5 participants