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

Rqd reserve all cores #1296

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

KernAttila
Copy link
Contributor

Link the Issue(s) this Pull Request is related to.
#1295

Summarize your change.

  • Reserve only the requested amount of threads, taking into account hybrid system with P-cores and E-cores.

  • Pick cores in order, still with priority on less idle cpu.

  • I had to change a test to match the updated HT reservation algo.

  • Added a test for hybrid cpu (i9-12900)

Warning
Should bump minor version as it may impact some existing implementations (hacks) relying on wrong thread counts.

@DiegoTavares
Copy link
Collaborator

This is an interesting change that never really got attention. I'm taking some time to study the architecture of hybrid system with P-cores and E-cores to understand what this PR is trying to accomplish. At a first glance, I like the direction it is taking, but I don't think it is documented properly.

@DiegoTavares
Copy link
Collaborator

DiegoTavares commented Aug 28, 2024

I'm finishing up a collection of changes that were pending on SPI's end and will keep this PR right at the top of my review list for when I finish. Sorry for the wait.

rqd/rqd/rqmachine.py Outdated Show resolved Hide resolved
@DiegoTavares
Copy link
Collaborator

DiegoTavares commented Sep 25, 2024

I think this logic was never well documented and now is the perfect opportunity to do so, as most of the content is already on this PR. Can you add a blog post to opencue.io's repo?

@DiegoTavares
Copy link
Collaborator

Aside from my question, I think this PR is ready to be merged. Did you have the change to tests it on a production environment to confirm:

  1. It behaves similarly to what it used to for non hybrid systems
  2. It behaves as expected on hybrid systems

@KernAttila
Copy link
Contributor Author

I think this logic was never well documented and now is the perfect opportunity to do so, as most of the content is already on this PR. Can you add a blog post to opencue.io's repo?

Sure thing, I will do it soon :)

@KernAttila
Copy link
Contributor Author

Aside from my question, I think this PR is ready to be merged. Did you have the change to tests it on a production environment to confirm:

  1. It behaves similarly to what it used to for non hybrid systems
  2. It behaves as expected on hybrid systems

Yes we did use it in production last year in my previous studio on our artists workstations, it was on RockyLinux with hybrid CPUs.
Both negative and normal booking worked as expected.
But no, I did not have the chance to test it on Linux with non-hybrid CPUs.
Booking on Windows is in progress with Anton's #1468, we discussed it and we'll make some adjustment on that part. For now, Windows behaves as before.
The only difference I can point is that CPUs are now forced to be booked in ascending order, otherwise it should behave similarly.
I'll make a VM to test on Ubuntu and confirm.

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.

4 participants