-
Notifications
You must be signed in to change notification settings - Fork 246
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
NUMA support #2374
Merged
Merged
NUMA support #2374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s for different farms + semaphore
…ersion of `tokio_rayon_spawn_handler`
nazar-pc
requested review from
ParthDesai,
vedhavyas,
shamil-gadelshin and
NingLin-P
December 27, 2023 10:56
1 task
vedhavyas
previously approved these changes
Jan 3, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense to me!
# Conflicts: # .github/workflows/snapshot-build.yml
vedhavyas
approved these changes
Jan 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Still waiting for user feedback in https://forum.subspace.network/t/numa-support-is-coming/2299?u=nazar-pc, but otherwise this should be ready for review.
The first change here is thread pool manager. Instead of having per-farm thread pools for plotting and replotting, we now have global set of thread pools.
With thread pool manager, we can associate each thread pool with a set of logical CPU cores. Initially this PR tried to pin threads to cores, but performance degraded massively from that. So in later commit I changed that to pin all threads in thread pool to the same set of CPU cores, such that OS kernel is free to move threads between those cores if it sees benefit from doing so.
NUMA support is implemented with
hwlocality
library that I had to fix in a few places (all upstream already) and cross-compilation still doesn't work on macOS unfortunately (but it is also not immediately beneficial there). Farmer will check which CPU cores belong to which NUMA node and will create number of plotting and replotting thread pools that correspond to number of NUMA nodes present in the system.NUMA support is introduced using enabled by default
numa
feature that has additional requirements to installed dependencies, see updated documentation and CI changes.Lastly,
mimalloc
crate apparently has NUMA-aware memory allocation support that should further improve plotting performance on NUMA systems, but no one tested yet whether it helps or not, so it is currently behindNUMA_ALLOCATOR
environment variable and will either be removed or enabled unconditionally in the future.Overall this was a somewhat painful process in terms of debugging, but I hope it is worth it.
Along the way I fixed some documentation and CI bugs that affected builds in forks ever since we switched to self-hosted runners.
Code contributor checklist: