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

Thread priority #2566

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Thread priority #2566

merged 2 commits into from
Feb 29, 2024

Conversation

nazar-pc
Copy link
Member

@nazar-pc nazar-pc commented Feb 28, 2024

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 to default or max).

Code contributor checklist:

@nazar-pc
Copy link
Member Author

@EmilFattakhov @jim-counter something for you to be aware of

@EmilFattakhov
Copy link
Member

Thanks for the ping, noted!

Copy link
Member

@vedhavyas vedhavyas left a 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,
Copy link
Member

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 ?

Copy link
Member Author

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).

Copy link
Contributor

@shamil-gadelshin shamil-gadelshin left a 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,
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

@nazar-pc nazar-pc left a 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,
Copy link
Member Author

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,
Copy link
Member Author

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.

@shamil-gadelshin
Copy link
Contributor

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.

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.

@nazar-pc
Copy link
Member Author

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.

@shamil-gadelshin
Copy link
Contributor

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.

@nazar-pc
Copy link
Member Author

That makes much more sense now. Thanks for the explanation.

Do you approve the changes then?

@nazar-pc nazar-pc added this pull request to the merge queue Feb 29, 2024
Merged via the queue into main with commit 4b690d9 Feb 29, 2024
11 checks passed
@nazar-pc nazar-pc deleted the thread-priority branch February 29, 2024 13:30
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