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

Swoole fix setAffinity call on Mac #327

Merged
merged 1 commit into from
May 6, 2024

Conversation

mcharytoniuk
Copy link
Member

No description provided.

Copy link
Member

@andrewdalpino andrewdalpino left a comment

Choose a reason for hiding this comment

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

Looks good, just wanted to confirm a couple things before we merge. See comment

@@ -98,7 +98,10 @@ function (Process $worker) use ($maxMessageLength, $queueItem) {
true,
);

$workerProcess->setAffinity([$currentCpu]);
if (method_exists($workerProcess, 'setAffinity')) {
$workerProcess->setAffinity([$currentCpu]);
Copy link
Member

Choose a reason for hiding this comment

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

So this looks like it's telling the worker process to bind to a particular unit. Is that right? Does this provide some performance enhancement? Does it still work as intended without setting the CPU affinity?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does work as intended; yeah it will be slower on mac because I think it will cause more context switching.

I added that on top of the other changes, so it wasn't the primary driver behind the performance gains.

@andrewdalpino andrewdalpino merged commit 274076c into RubixML:2.5 May 6, 2024
6 of 13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants