Skip to content

feat(runtime): implement thread affinity for runtime and dispatcher #445

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

Merged

Conversation

numinnex
Copy link
Contributor

@numinnex numinnex commented Jul 3, 2025

This PR adds an extra field to both Runtime and Dispatcher builder that allows user to set thread affinity.

Copy link
Member

@Berrysoft Berrysoft left a comment

Choose a reason for hiding this comment

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

I suggest using core_affinity (or other equivalents) rather than nix, to support more platforms.

@George-Miao Do you think we need a feature gate for this PR?

@numinnex
Copy link
Contributor Author

numinnex commented Jul 3, 2025

I think given that core_affinity is an extra dependency, feature gate wouldn't hurt. I just used nix because I am not big of a fan of slapping extra deps.

Will take a look into core_affinity guarantees about other platforms support.

@numinnex
Copy link
Contributor Author

numinnex commented Jul 3, 2025

One more question tho, what behavior should we expose given an input that exceeds number of available CPUs ?

@numinnex numinnex force-pushed the runtime_dispatcher_thread_affinity branch from c90cb74 to ae48b26 Compare July 3, 2025 18:41
@numinnex
Copy link
Contributor Author

numinnex commented Jul 3, 2025

I have pushed changes that replace nix with core_affinity, but the question from above still stands.

@numinnex numinnex marked this pull request as draft July 4, 2025 04:39
@George-Miao
Copy link
Member

One more question tho, what behavior should we expose given an input that exceeds number of available CPUs ?

I think an error won't hurt.

@numinnex numinnex force-pushed the runtime_dispatcher_thread_affinity branch 2 times, most recently from 06f1389 to 593fae3 Compare July 5, 2025 11:22
@numinnex
Copy link
Contributor Author

numinnex commented Jul 5, 2025

I have changed the implementation from using Vec to use HashSet, the vec implementation was wrong and convoluted.

@numinnex numinnex force-pushed the runtime_dispatcher_thread_affinity branch from b201309 to 85f2aaf Compare July 5, 2025 12:17
@numinnex numinnex force-pushed the runtime_dispatcher_thread_affinity branch 3 times, most recently from c3ea248 to a390c77 Compare July 5, 2025 16:47
@numinnex
Copy link
Contributor Author

numinnex commented Jul 5, 2025

I don't understand why this test fails on MacOS and sadly I don't own one to test it locally :( but running those on my Linux setup works fine.

@Berrysoft
Copy link
Member

It's not successful to get the error with last_os_error because the macOS method thread_policy_set doesn't set errno.

I think bind_to_cpu_set is OK to fail. We may just log the error (through compio_log::error!) and return silently.

@numinnex numinnex force-pushed the runtime_dispatcher_thread_affinity branch from a390c77 to c6584c4 Compare July 5, 2025 18:40
@numinnex numinnex marked this pull request as ready for review July 5, 2025 18:58
Copy link
Member

@Berrysoft Berrysoft left a comment

Choose a reason for hiding this comment

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

LGTM

@Berrysoft
Copy link
Member

@AsakuraMizu help wanted: test_cmsg on Windows failed, but it should not be related to this PR.

@numinnex
Copy link
Contributor Author

numinnex commented Jul 6, 2025

Maybe it's just a flaky test. Let me re-run those.

@numinnex
Copy link
Contributor Author

numinnex commented Jul 6, 2025

Doesn't seem like that was the issue...

windows-sys got recently updated, maybe this is what causes this test to fail ?
image

But it was failing already, before merging the latest commit from master... So I doubt it.

@Berrysoft
Copy link
Member

The test failure has been fixed in #449 and merged just now. Could you rebase this PR?

@Berrysoft Berrysoft force-pushed the runtime_dispatcher_thread_affinity branch from 24c21ad to 2efe07b Compare July 6, 2025 10:13
@Berrysoft
Copy link
Member

I rebased it for you :)

@Berrysoft
Copy link
Member

Oh no seems I didn't update the branch before rebase

@numinnex
Copy link
Contributor Author

numinnex commented Jul 6, 2025

Thanks, I am away from my PC right now

@Berrysoft Berrysoft merged commit 3d5ec97 into compio-rs:master Jul 6, 2025
48 checks passed
@Berrysoft
Copy link
Member

Thank you very much!

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