-
Notifications
You must be signed in to change notification settings - Fork 247
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
Thread priority #2566
Thread priority #2566
Conversation
@EmilFattakhov @jim-counter something for you to be aware of |
Thanks for the ping, noted! |
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. Question on plotting thread priority
/// Default priority | ||
Default, | ||
/// Max priority (not recommended) | ||
Max, |
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.
any specific reason why reference implementation wants to use this plotting when its clearly not recommended? Maybe this should not be an option at all ?
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.
There is actually a much more fine-grained control available over priority. But for now I decided to give extreme option for users to try and experiment with. It will likely evolve over time, we may end up removing it if not helpful. This might be desirable to accelerate plotting at all costs, but it will likely make farming while plotting not possible at all (though not guaranteed).
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.
Are you sure that you will deprioritize the plotting thread only compared to the regular thread of this process? At least on Windows 2000, thread priority was calculated by thread priority and process priority, and thus your change can deprioritize threads globally and I'm not sure it is the intended behaviour.
/// Plotting thread priority, by default de-prioritizes plotting threads in order to make sure | ||
/// farming is successful and computer can be used comfortably for other things | ||
#[arg(long, default_value_t = PlottingThreadPriority::Min)] | ||
plotting_thread_priority: PlottingThreadPriority, |
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.
Why do we need to change this feature if we consider it useful?
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.
Because there different circumstances and min
will likely slightly decrease plotting performance. Users should be able to change behavior to the old one without downgrading while we're figuring out the best defaults. Otherwise they'll run outdated software and will not be able to benefit from other optimizations.
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.
Are you sure that you will deprioritize the plotting thread only compared to the regular thread of this process? At least on Windows 2000, thread priority was calculated by thread priority and process priority, and thus your change can deprioritize threads globally and I'm not sure it is the intended behaviour.
I'm not 100% and a lot of things has changed since Windows 2000 anyway. I do expect threads to be de-prioritized globally though and not impact node sync and farming as much, which should be a net positive for users. We might even be able to enable farming while plotting for everyone with this change, even on small CPUs.
/// Default priority | ||
Default, | ||
/// Max priority (not recommended) | ||
Max, |
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.
There is actually a much more fine-grained control available over priority. But for now I decided to give extreme option for users to try and experiment with. It will likely evolve over time, we may end up removing it if not helpful. This might be desirable to accelerate plotting at all costs, but it will likely make farming while plotting not possible at all (though not guaranteed).
/// Plotting thread priority, by default de-prioritizes plotting threads in order to make sure | ||
/// farming is successful and computer can be used comfortably for other things | ||
#[arg(long, default_value_t = PlottingThreadPriority::Min)] | ||
plotting_thread_priority: PlottingThreadPriority, |
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.
Because there different circumstances and min
will likely slightly decrease plotting performance. Users should be able to change behavior to the old one without downgrading while we're figuring out the best defaults. Otherwise they'll run outdated software and will not be able to benefit from other optimizations.
My point was that in some cases the change will degrade the overall performance of the plotting when it competes for resources with other apps or services. But you seem to expect that. |
Yes, that was the goal. Some users complained that latest versions are very heavy and makes computer unusable if it is not dedicated to Subspace exclusively. This should greatly help with that, but behavior can still be returned to the previous default if desired. |
That makes much more sense now. Thanks for the explanation. |
Do you approve the changes then? |
Yesterday during interview with a candidate I learned that it is possible to not only set process priority, but also thread priority.
This PR takes advantage of that for maximizing timekeeper thread priority and minimizing plotting thread priority (by default, can be changed with
--plotting-thread-priority
todefault
ormax
).Code contributor checklist: