Skip to content

Processor affinity#3137

Open
fwesselm wants to merge 21 commits into
latestfrom
affinity
Open

Processor affinity#3137
fwesselm wants to merge 21 commits into
latestfrom
affinity

Conversation

@fwesselm

@fwesselm fwesselm commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

This is a first attempt to handle processor affinity. I am not an expert here, so any feedback is appreciated.

@Opt-Mucca Opt-Mucca left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I like the change! I'd defer to @mathgeekcoder for a more informed opinion though

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.19%. Comparing base (bcd7231) to head (5193b26).
⚠️ Report is 5 commits behind head on latest.

Files with missing lines Patch % Lines
highs/parallel/HighsTaskExecutor.cpp 77.77% 6 Missing ⚠️
highs/lp_data/Highs.cpp 66.66% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           latest    #3137   +/-   ##
=======================================
  Coverage   73.18%   73.19%           
=======================================
  Files         430      430           
  Lines      104132   104180   +48     
  Branches    16737    16742    +5     
=======================================
+ Hits        76212    76254   +42     
- Misses      27644    27650    +6     
  Partials      276      276           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mathgeekcoder

Copy link
Copy Markdown
Collaborator

I think this is great!

My only feedback would be to be careful about interchanging the terminology vthreads with cores. Specifically, unsigned int available_cores = highs::parallel::available_concurrency();

The CPU_COUNT, hardware_concurrency return vthreads, not cores. However, we'd like to create a thread pool with the number of cores (not vthreads)!

On that note, I believe the current (and previous) code assumes 2 vthreads per core, however this is not always true; and gets more complicated when running in the cloud.

Ideally, we'd like to get the count of physical cores that we are mapped and masked by affinity, and use this as our number of threads in the thread pool.

@fwesselm

fwesselm commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

I think this is great!

My only feedback would be to be careful about interchanging the terminology vthreads with cores. Specifically, unsigned int available_cores = highs::parallel::available_concurrency();

The CPU_COUNT, hardware_concurrency return vthreads, not cores. However, we'd like to create a thread pool with the number of cores (not vthreads)!

On that note, I believe the current (and previous) code assumes 2 vthreads per core, however this is not always true; and gets more complicated when running in the cloud.

Ideally, we'd like to get the count of physical cores that we are mapped and masked by affinity, and use this as our number of threads in the thread pool.

Thank you, @mathgeekcoder!

I tried to change the code according to your comments. I hope I did not introduce new bugs, it would be great if you could check.

@mathgeekcoder

Copy link
Copy Markdown
Collaborator

Oh wow! I said it, but didn't expect you to implement it :)

I think this is a great start. If I'm being really pedantic:

  • I believe the cpu terminology is the same as vthread (rather than core), i.e., cpu can mean physical or logical processor, whereas core is only physical processor
  • We might want to use fallback if any of the topology files don't exist (rather than if physical_cores.empty())
  • Technically CPU_SETSIZE = 1024 and larger systems exist. Can use sysconf(_SC_NPROCESSORS_CONF) and CPU_ALLOC to get and set correct size
  • I think you can use GetLogicalProcessorInformationEx when >64 CPUs on Windows

But as I said... pedantic. Honestly, it's fine as is!

Ultimately, it would be fantastic to also exploit numa nodes and shared cache (e.g., L3). Once @galabovaa gets back we should check if it's okay to use something like hwloc to get this information. hwloc also has cross-platform code to count the cores.

For example, it'd be great to configure work-stealing to prefer cores within same numa node, and perhaps use logical processors (i.e., vthread, if available) to sync/exchange information.

@fwesselm fwesselm removed the Draft PR label Jul 9, 2026
@fwesselm

fwesselm commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Oh wow! I said it, but didn't expect you to implement it :)

I think this is a great start. If I'm being really pedantic:

  • I believe the cpu terminology is the same as vthread (rather than core), i.e., cpu can mean physical or logical processor, whereas core is only physical processor
  • We might want to use fallback if any of the topology files don't exist (rather than if physical_cores.empty())
  • Technically CPU_SETSIZE = 1024 and larger systems exist. Can use sysconf(_SC_NPROCESSORS_CONF) and CPU_ALLOC to get and set correct size
  • I think you can use GetLogicalProcessorInformationEx when >64 CPUs on Windows

But as I said... pedantic. Honestly, it's fine as is!

Ultimately, it would be fantastic to also exploit numa nodes and shared cache (e.g., L3). Once @galabovaa gets back we should check if it's okay to use something like hwloc to get this information. hwloc also has cross-platform code to count the cores.

For example, it'd be great to configure work-stealing to prefer cores within same numa node, and perhaps use logical processors (i.e., vthread, if available) to sync/exchange information.

Thanks again, @mathgeekcoder! I tried to address your comments. For Windows, I am trying to follow hwloc's approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants