Do topology perms with threads#4270
Conversation
…rflow in demos Adds a new step "Configure matplotlib for CI" that creates a matplotlib configuration file with `axes.unicode_minus: False` to work around FreeType "raster overflow" crashes (error 0x62) when rendering Unicode minus signs in plot labels for negative tick labels and log-scale exponents. This resolves test failures in demo_lagrange-variants.py and demo_tnt-elements.py that were caused by FT_Render_Glyph failures in the Spack/Ubuntu environment.
| int num_threads | ||
| = std::max(1, static_cast<int>(std::thread::hardware_concurrency())); | ||
| spdlog::debug("Using {} threads for face permutation computation", | ||
| num_threads); |
There was a problem hiding this comment.
I thought we always wanted this to be specified by the user as a input argument. That is what we have done for all other threaded functionality so far.
There was a problem hiding this comment.
I know. Bear in mind this is still draft. I am not super convinced about passing num_threads around everywhere.
There was a problem hiding this comment.
Something we should discuss in a developer meeting :)
There was a problem hiding this comment.
Passing the number of threads is the right thing to do. There is no other good option.
There was a problem hiding this comment.
Had a quick chat about this with @chrisrichardson today and we do see a path forwards on this; use an explicit std::optional for num_threads and then fall back on an 'ambient' global. The ambient global could also be overridden using an RAII-scope guard.
There was a problem hiding this comment.
The low-level functions should just have a num threads arg. It's clear, simple and explicit, and forces the caller to recognise that the function supports threads.
At a higher level we can work through options.
| std::vector<std::bitset<BITSETSIZE>> edge_perm(num_cells, 0); | ||
| std::vector<std::int64_t> cell_vertices, vertices; | ||
| for (int c = 0; c < c_to_v->num_nodes(); ++c) | ||
| std::vector<std::jthread> threads( |
michalhabera
left a comment
michalhabera
left a comment
There was a problem hiding this comment.
Few comments to consider:
- Don't default use
std::thread::hardware_concurrency(). User runs with outermpirun -n 2and this oversubscribes badly. - We need to stick to the same policy of meaning for
num_threadsin the API. It could either mean: a) total number of execution lanes including caller, or b) number of worker threads to spawn, besides the caller.
For a), num_threads=0 is invalid, and num_threads=1 means serial, with no spawning. For b), num_threads=0 means serial, and num_threads=1 means the caller does its job (maybe waits?), and there is one thread spawned. I would strongly suggest to stick with a).
Your PR does neither, since with num_threads=1 the caller idles for one spawned worker. That needs fixing. It is the point @schnellerhase was making in the other PR that introduced threading. - Passing num_threads in the API is good, I agree, as long as this is mostly for few low-level interfaces, and we never allow calling a foo(num_threads=8) inside parallel region of a higher level call, i.e. a nesting. That would need to pass some richer context around.
- If num_threads pollutes the interfaces too much, for convenience, we can maybe consider it to have a non-zero, user changeable default value.
5 participants
Uses
std::jthreadto run topology permutation computation in parallel, enabling a speedup.Also, moves the
dolfinx::MPI::local_rangecomputation todolfinx::common::local_rangesince it is now being used for other things than MPI.See #1250