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

Make plotting/replotting thread pools distinct for farms to avoid stack overflow, still protected by semaphores #2167

Merged
merged 1 commit into from Oct 27, 2023

Conversation

nazar-pc
Copy link
Member

Having multiple ThreadPool::install() apears to be problematic as described in rayon-rs/rayon#751

Since #2095 we have separate concurrency parameters for sector downloading and encoding, so we will not have two sectors plotting at the same time unless user wants to anyway. BTW it would open the option to have thread pool size smaller per plot and allow multiple sectors to be encoded at the same time on larger machines to not cross NUMA boundaries, but farmer would need to become NUMA-aware for that to make sense (and at that point it can do such things automatically when NUMA is detected).

Code contributor checklist:

@shamil-gadelshin
Copy link
Member

Do I understand correctly that with the new changes and having set --sector-encoding-concurrency > 1 it's possible to undo the thread pool separation between tasks we achieved previously?

@nazar-pc
Copy link
Member Author

Do I understand correctly that with the new changes and having set --sector-encoding-concurrency > 1 it's possible to undo the thread pool separation between tasks we achieved previously?

Before this PR thread pools for plotting/replotting were global, now they are deparate. Not sure what you mean by "undo" in this case.

@shamil-gadelshin
Copy link
Member

Before this PR thread pools for plotting/replotting were global, now they are deparate. Not sure what you mean by "undo" in this case.

We can have several "single_plot_farms" simultaneously and each will create its own copies of the thread pools for both plotting and replotting, correct? This means that plotting and replotting threads could steal CPU cores from each other similar to what we had before. Does it make sense?

@nazar-pc
Copy link
Member Author

This means that plotting and replotting threads could steal CPU cores from each other similar to what we had before. Does it make sense?

Not in the same way. These will be separate threads, while previously they'd compete for a shared thread pool.

@nazar-pc nazar-pc added this pull request to the merge queue Oct 27, 2023
Merged via the queue into main with commit e864961 Oct 27, 2023
10 checks passed
@nazar-pc nazar-pc deleted the fix-plotting-stack-overflow branch October 27, 2023 10:40
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.

None yet

2 participants