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

Memory leak: add rotatedPHP to kill and recreate PHP instances after a certain number of requests #990

Merged
merged 19 commits into from
Feb 2, 2024

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Jan 31, 2024

Supersedes WordPress/playground-tools#110

Swaps the internal PHP runtime for a fresh one after a certain number of requests are handled.

Rationale

PHP and PHP extension have a memory leak. Each request leaves the memory a bit more fragmented and with a bit less available space than before. Eventually, new allocations start failing.

Rotating the PHP instance may seem like a workaround, but it's actually what PHP-FPM does natively:

Implementing this solution in core enables all downstream consumers of the PHP WASM package, like wp-now, to benefit.

How it works

Adds a rotatePHP function that keeps track of the BasePHP.run() calls and replaces the Emscripten runtime with a new one after a certain number of calls.

Example:

import { rotatePHPRuntime } from '@php-wasm/universal';

const loadRuntime = async () => await NodePHP.loadRuntime("8.0");

const php = new NodePHP(await loadRuntime(), {
	documentRoot: '/test-root',
});

await rotatePHPRuntime({
	php,
	recreateRuntime: loadRuntime,
	maxRequests: 50
});

// After 50 request() calls, the PHP runtime will be discarded and a new one will be created.
// It all happens internally. The caller doesn't know anything about this.

I started by porting the "Process Pool" idea (see the branch) from WordPress/playground-tools#110, but I realized that:

  • The logic was complicated
  • We only need to manage a single PHP instance, not a pool of them
  • Worker thread is built to a single PHP instance and not architected to handle a pool

So I decided to simplify the logic and just manage a single PHP instance.

Other changes

  • Implements BasePHP.hotSwapPHPRuntime() that opens the door to switching PHP versions dynamically and without a full page reload.
  • Makes the BasePHP.semaphore property public to enable conflict-free runtime swaps
  • Adds the runtime.initialized and runtime.beforedestroy events for OPFS handlers to re-bind on runtime swap.
  • Tiny unrelated change: Migrates the OPFS handler from monkey-patching the php.run method to relying on event listeners. The rotatePHP function went through a similar journey and I updated that other code path while I was on it.

Testing instructions

confirm the CI checks pass

CC @dmsnell

…a certain number of requests

Supersedes WordPress/playground-tools#110

Prevents PHP's memory leaks from causing the system to stop working once memory has been exhausted. It follows in the footsteps of systems like PHP-FPM by limiting the number of requests a PHP "thread" can handle before it is discarded and restarted.

Implementing a solution in core enables all downstream consumers of the PHP WASM package to benefit.

 ## How it works

Adds a rotatedPHP function that takes a PHP factory function and returns a Proxy that manages the PHP instance internally. The Proxy will discard the PHP instance after a certain number of requests, and will create a new instance when the next request comes in.

Example:

```ts
const php = rotatedPHP({
    createPhp: () => WebPHP.load('8.0'),
    maxRequests: 50,
});

// After 50 request() calls, the PHP instance will be discarded and a new one will be created.
// It all happens internally. The caller doesn't know anything about this.
```

I started by porting the "Process Pool" idea from WordPress/playground-tools#110, but I realized that:

* The logic was complicated
* We only need to manage a single PHP instance, not a pool of them
* Worker thread is built to a single PHP instance and not architected to handle a pool

So I decided to simplify the logic and just manage a single PHP instance.

 ## Remaining work

* Unit tests
* Comments
* Better names perhaps, `rotatedPHP` doesn't seem ideal

CC @dmsnell
@adamziel adamziel added [Feature] PHP.wasm [Type] Reliability Playground uptime, reliability, not crashing labels Jan 31, 2024
adamziel added a commit that referenced this pull request Jan 31, 2024
…requests

This is an alternative to #990 that was
not pursued
@adamziel
Copy link
Collaborator Author

adamziel commented Feb 1, 2024

This approach unlocks hotswapping PHP versions without forcing a full page reload – just rotate to PHP 8.2, then 8.1 etc.

@adamziel adamziel requested a review from bgrgicak February 1, 2024 13:42
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks more robust than the previous approach, and I like how this swaps out a single runtime vs. pre-reserving a pool of workers.

how are we deciding on the number of max requests? would it be helpful to shorten that list? would it be helpful to analyze total memory use and make it depend on that?

});
await php.run({
code: `<?php echo 'hi';`,
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the point of these lines?

Copy link
Collaborator Author

@adamziel adamziel Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These run() calls are meant to trigger the internal PHP runtime rotation. I just replaced the rotate() function with something more readable:

		// Rotate the PHP runtime
		await php.run({ code: `` });

With no "echo" involved, the intention is hopefully less obscured

@adamziel
Copy link
Collaborator Author

adamziel commented Feb 2, 2024

how are we deciding on the number of max requests? would it be helpful to shorten that list? would it be helpful to analyze total memory use and make it depend on that?

I just added a comment to answer these questions in the code. 400 is an arbitrary number I pulled out of a hat. It seems to work well in practice on my setup in that:

  • The memory-related crash is not happening anymore
  • The small slowdown required to initialize the new runtime and copy the MEMFS files is rare enough that it goes mostly unnoticed.

I realize there might be ways of using Playground that will leave the memory extremely fragmented after less than 400 requests. I don't know whether they exist or how to identify them so you're right – ideally the rotation logic would be based on the amount of free, unfragmented memory. I'm yet to find a fast method of keeping track of that – even a naive approach of calculating the null bytes takes a few blocking seconds. I think I'll leave that hardcoded 400 in there for now with a mental note to keep exploring should the memory issue be reported again.

Also CC @bgrgicak

@adamziel adamziel merged commit 07fd316 into trunk Feb 2, 2024
5 checks passed
@adamziel adamziel deleted the memory-pool branch February 2, 2024 09:43
* Copies the MEMFS directory structure from one FS in another FS.
* Non-MEMFS nodes are ignored.
*/
function recreateMemFS(newFS: EmscriptenFS, oldFS: EmscriptenFS, path: string) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity – it would be so, so lovely to directly reuse the oldFS object without going through the copying here.

Copy link
Collaborator

@bgrgicak bgrgicak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's mindblowing that we can swap php versions on the fly 🚀

await php.run({ code: `<?php echo "abc";` });
const freeAfterRotation = freeMemory(php);
expect(freeAfterRotation).toBeGreaterThan(freeAfter1000Requests);
}, 30_000);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL 30_000 === 30000

@dmsnell
Copy link
Member

dmsnell commented Feb 2, 2024

we can swap php versions on the fly

idea: randomly pick the PHP versions on swap just to mess with people 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] PHP.wasm [Type] Reliability Playground uptime, reliability, not crashing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants