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

Index traits using multiple child processes. #95

Merged
merged 8 commits into from
Mar 12, 2024

Conversation

jongardiner
Copy link
Contributor

Greatly speeds up trait importing on high core count machines. Also, cleans up the IPC code some.

@jongardiner jongardiner requested a review from a team as a code owner February 28, 2024 01:33
* @param array $toProcess
* @return array
*/
public function partition(Config $config, array $toProcess): array {
Copy link
Member

Choose a reason for hiding this comment

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

pulling out these methods makes this much easier to understand.

use BambooHR\Guardrail\Socket;
use BambooHR\Guardrail\SocketBuffer;

abstract class ChildProcess {
Copy link
Member

Choose a reason for hiding this comment

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

I like this organization of classes, with this one handling the actual i/o blocking.

if ($status != 0) {
call_user_func($serverReadCallBack, $this->connections[$childPid], "Child died with non-zero status");
echo "Child died with non-zero status!\n";
exit($status);
Copy link
Member

Choose a reason for hiding this comment

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

If any child crashes or fails, make sure the parent fails. should prevent cihub reeporting success when something goes wrong, I think? Might solve some of the previous issues we've had.

echo "Error: Unexpected error reading from socket\n";
exit(1);
}
if ($msg === false || (trim($msg) !== "" && self::CLOSE_CONNECTION == $this->dispatchMessage($socket,$msg))) {
Copy link
Member

Choose a reason for hiding this comment

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

$msg === false is unreachable here.

function handleClientMessage(\Socket $socket, string $message, string ...$params): int {
assert($message == "TRAITED");
$this->output->outputExtraVerbose("Updating class ".$params[0]. " with ".strlen($params[1])." bytes of data\n");
$class=unserialize(base64_decode($params[1]));
Copy link
Member

Choose a reason for hiding this comment

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

this area is the crux of being able to handle traits in forked processes. The children do the work, but do not update the symbol table (which would be futile, since they don't share the memory space of the parent), but instead re-emit the entire structure of the class after the trait has been applied, and the parent replaces the structure of the class in the symbol table. Keeps the children stateless, only accepting input, and emitting output. The parent is the one that keeps track of state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I love that you spend so much time understanding the code you're asked to review. Thank you.

@jongardiner jongardiner merged commit 765c190 into master Mar 12, 2024
2 checks passed
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