Avoid partial munmap memory leak

[brandonpayton]

What is this PR doing?

This PR works around a PHP memory leak that can quickly lead to an OOM condition.

What problem is it solving?

This PR fixes #1128 -- an OOM condition due to memory leak.

How is the problem addressed?

This PR adds custom PHP memory storage handlers to stop PHP from attempting to partial unmap memory via the munmap() function. Emscripten allocators do not support using munmap() to partially unmap memory. They fail for partial unmaps, and when attempts fail, memory is leaked because PHP thinks the memory is free while Emscripten libs consider it allocated.

The custom memory storage handlers take over responsibility for allocating and freeing aligned memory chunks. The handlers use posix_memalign() and free() instead of mmap() and munmap(), and this appears to resolve the problem.

The handlers are added as part of a PHP extension, which is not perfect because allocation can happen before the extension is initialized. But the amount of allocation before the workaround is applied should be quite small.

Testing Instructions

Before applying this patch:
Run this test script and verify that it OOMs.

After applying this patch:
Rerun the test script and verify that it completes after 1000 interations.

Remaining work

• Compare performance with php-wasm builds prior to this patch and reconsider if 20% slower.
• Find out whether it is sufficient to add the workaround during the PHP module init phase rather than during each request init
• Address minor TODO comments in wasm_memory_storage.c
• See if we can run the test script as part of tests under packages/php-wasm/node/src/test

[brandonpayton]

I rebased locally and am rebuilding php-wasm for node and web.

[brandonpayton]

Updated the PR description to note that we should add the test script to the php-wasm node tests.

[brandonpayton]

Please hold off testing this. Something is off here with the latest builds. The wasm_memory_storage extension is not present.

I verified this earlier before rebuilding for all PHP versions, but perhaps I did not test what I thought I tested. This should be a straightforward fix, but it's unfortunate others cannot yet try the fix for themselves. Planning to look into this first-thing in the morning.

[brandonpayton]

Found the issue:
Somehow the Dockerfile updates in this PR were missing the COPY instruction that I added earlier today for copying in the wasm_memory_storage extension into php-src.

I'm not yet sure what might have happened for that to be omitted. Instead of building all PHP versions again tonight, I'll go ahead and build just light PHP 7.0 and PHP 8.3 for the web.

One odd thing I noticed when testing after rebasing:
The OOM test script output is no longer directed to the browser when there is an OOM. Instead there is just an error in the dev tools console with the OOM message. This seems undesirable as a user may prefer some output indicating breakage than hidden output in the case of breakage. Will make a note to confirm that behavior tomorrow.

[brandonpayton]

If others are interested, the build is fixed, and this is now ready for exploratory testing.

I've included just the light php-wasm builds for the web for PHP 7.0 and PHP 8.3 for now.

[dmsnell]

all very nice. it would be great to see some links to the PHP documentation on custom allocators, so that someone could come into this file, review the spec, and see if the code is right.

e.g. a docblock above each function saying "This is the XYZ [link to custom allocator definition of XYZ"

[adamziel]

I've played with performance measurements.

Methodology:

• Close all other Playground tabs and open a single new tab
• Open devtools before opening the page
• Go directly to a local Playground URL that loads the respective PHP version and WP 6.5
• Paste the code snippet below into devtools
• Don't touch the computer until the test is finished. In particular, don't interact with the page and don't switch to another browser tab.

These may seem like insignificant details, but Chrome does very different things under the hood with and without devtools (different WASM runtimes) and also if the existing tab was used before (Playwright developers struggled with intermittent failures when reusing tabs).

async function measureTime() {
    console.time("ExecutionTime"); // Start the timer
    for(let i = 0; i < 100; i++) {
        await playground.request({ url: '/' });
        if ((i+1) % 10 === 0) { // Log progress every 10 requests
            console.log(`Progress: Completed ${i+1} out of 100 requests`);
        }
    }
    console.timeEnd("ExecutionTime"); // End the timer and print the total time taken
}
measureTime();

Here are the results of 4 test runs on each version:

• PHP 8.3 from this PR: 29 765 ms, 29 112ms, 29 126 ms, 29 180ms
• PHP 8.3 from trunk: 29 701 ms, 29 334ms, 29 383ms, 29 317ms

My testing methodology requires some manual work and I won't spend the time to collect 30 or 500 samples to plot a distribution and calculate p-values. We could automate that with a Node.js build, but at this point I'm quite happy to assume we're not looking at a shattering performance decline. I've captured this comment in the project wiki for any future performance discussions.

For now – let's assume the performance is alright here and proceed with code review as usual 👍

Even if there's a much slower code branch somewhere, I'd rather fix five more crashes than obsess about all the performance implications – we'll focus on the speed more if and when that matters.

Thank you so much for all your work on this @brandonpayton!

[adamziel]

Let's also link to this PR and the related issue to give future readers more context

[brandonpayton]

I've played with performance measurements.

Methodology:
[snip]
These may seem like insignificant details, but Chrome does very different things under the hood with and without devtools (different WASM runtimes) and also if the existing tab was used before (Playwright developers struggled with intermittent failures when reusing tabs).

@adamziel Thanks for testing and for sharing the clear methodology.

I ran some tests but with less care about creating a new tab and opening dev tools first. I didn't remember about console.time() and didn't know about playground.request(). Here's the script I used (with the current scope URL substituted):

async function* test100() {
    for (let i = 0; i < 100; ++i) {
        yield await fetch( '/scope:0.7238152673090998/' ).then( r => r.text() );
    }
}

let start = performance.now();
console.log( 'Start:', start );
for await ( let t of test100() ) {}
let end = performance.now();
console.log( 'End:', end );
console.log( 'Total:', end - start );

There isn't a reason that the response text was needed, but originally, I was thinking about logging part of it.

Without this PR, the total time was about 8.97 seconds.
With this PR, the total time was about 7.87 seconds.

I prefer your more careful tests but wanted to share this sample.

[brandonpayton]

@dmsnell, I added more documentation, including links to inline examples of custom storage functions, but so far, I have not been able to find related specs or external documentation to link to. Please feel free to ask for more if you think something is missing.

@adamziel, I was able to move the workaround assignment into module init. Zend memory manager does do some cleanup during request shutdown, but it only creates its heap during module startup.

It is possible other modules may have allocated memory before the workaround is added, and we should give those a chance to free the memory in the same way it was allocated. Fortunately, when PHP chooses to run module shutdown, it appears to do so in reverse order which gives us a chance to remove the workaround before those initial allocations are freed. So I added a module shutdown function to cleanup after ourselves.

With the exception of adding a test for the OOM case, this PR is ready for another review.

[adamziel]

This looks great @brandonpayton! I'm happy to merge as soon as all the PHP versions are rebuilt.

Should we now be strict about using emalloc and efree in php_wasm.c? Or is malloc and free still fine?

[brandonpayton]

I added a test for this fix that runs for all supported PHP versions:
2e37037

All these tests fail with an OOM error before rebuilding php-wasm with the fix. After the fix, the tests pass.

This is ready for another review.

[brandonpayton]

This looks great @brandonpayton! I'm happy to merge as soon as all the PHP versions are rebuilt.

@adamziel, sounds good! I will rebuild the PHP versions but leave for merging tomorrow, in case there is some auto-deploy to playground.wordpress.net and there are unexpected problems during the night.

Should we now be strict about using emalloc and efree in php_wasm.c? Or is malloc and free still fine?

At a high level, I guess emalloc and efree should be used if you want PHP's memory manager involved. But based on my current understanding, it should be safe to use either pair, as long as we don't mix them and attempt to efree() something allocated with malloc().

[brandonpayton]

Note: I am stepping away for the night but will leave a build command running that should commit and push when complete:

npm run "recompile:php:node" && npm run "recompile:php:web" && git add -u && git commit -m "Add php-wasm builds" && git push --force-with-lease

[dmsnell]

Looks great, and thanks for the comments. Too bad there's not more clarity in PHP around creating these custom internals, but the links to a versioned source code file are about as perfect as I suppose we can get.

Looking forward to how this goes. It will make many people happy.