-
Notifications
You must be signed in to change notification settings - Fork 301
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
Add C11 threads.h implementation in addition to pthread implementation for multithreaded encoding. #822
base: master
Are you sure you want to change the base?
Conversation
This whole thing is useful because MSVC provides native C11 threads support in recent versions, and has (of course) no pthread support. See https://devblogs.microsoft.com/cppblog/c11-threads-in-visual-studio-2022-version-17-8-preview-2/. Also see https://stackoverflow.com/questions/24557728/does-any-c-library-implement-c11-threads-for-gnu-linux for some (outdated) discussion about C11 threads adoption in C standard library implementations. |
@sezero As you mentioned the extra pthread dependency in #634 (comment), this might be relevant for you. I did choose C11 threads instead of native WinAPI threads because the impedance mismatch between C11 threads and pthread is really low implementation-wise. As far as I know, MinGW-w64 does not support C11 threads yet, so this is not of immediate use with that particular toolchain. I would expect MinGW-w64 to eventually support C11 threads (it's part of ISO C after all), so this might be useful in the future. For now, it adds in particular multithreading support with MSVC, for which the extra pthread dependency was even more of a hassle than it is for MinGW-w64. |
…be disabled via a new hidden setting [Export]FLACMultithreading=true. This depends on <xiph/flac#822>, and is a no-op until our libflac supports multi-threading. [Imp] FLAC Samples: Add hidden setting [Sample Editor]FLACMultithreading=false. As samples are short for the most part, thread creation overhead might dominate the encoding, so this is disabled by default. This has not been benchmarked. This depends on <xiph/flac#822>, and is a no-op until our libflac supports multi-threading. [Imp] openmpt123: Enable FLAC multithreading unconditionally. git-svn-id: https://source.openmpt.org/svn/openmpt/trunk/OpenMPT@23042 56274372-70c3-4bfc-bfc3-4c3a0b034d27
Sorry, really not interested |
Thanks! Would indeed be great to have MSVC on board. Just one comment for now (I haven't thoroughly reviewed this), can you move as much as possible into share/compat.h? That seems appropriate, even if it is only used in stream_encoder.c |
I would rather not use the global I can still move the abstraction macros of course, but I would rather use a distinct new |
…n for multithreaded encoding. The switching between both APIs is done via macros in a new file include/share/compat_threads.h, as this seemed simple enough. There are only slight differences between pthread and C11 threads, in particular the way attributes are handled for mutex and condition variable (both not used by FLAC), the signature of the thread function, the return value of trylock, and the error handling, where pthread uses errno and C11 threads use an explicit return value enum. The wrapper macros are prefixed with FLAC__ as is common in FLAC code. All functions are renamed to closely resemble the C11 names instead of the POSIX names, as this is the more forward-looking approach. The macro to enable C11 threads is HAVE_C11THREADS. When both HAVE_C11THREADS and HAVE_PTHREAD are defined, the C11 implementation is preferred. This does not add any build system support for C11 threads, so the current default behaviour remains unchanged.
If multithreading is not explicitly disabled, and if pthread is not detected, enable C11 threads support as a fallback. We still default to pthread as this is the more conservative and backwards-compatible approach for older POSIX systems. Using C11 threads might imply using pthreads under the hood, and might require linking additional pthread libraries, which a simple check for threads.h will not enable. AX_PTHREAD however does that, and it is thus the safer choice. configure output is changed to display "C11 threads" or "pthread", instead of just "yes" when multithreading is enabled.
I added I know close to nothing about CMake, and would prefer if someone else more knowledgeable touches that. |
I did have a go at that, however I ended up running into CMake quirks for which I am not sure I found the appropriate work-around. The patch is in a separate branch at https://github.com/manxorist/flac/tree/c11threads-cmake. |
The switching between both APIs is done via macros as this seemed simple enough. There are only slight differences between pthread and C11 threads, in particular the way attributes are handled for mutex and condition variable (both not used by FLAC), the signature of the thread function, the return value of trylock, and the error handling, where pthread uses errno and C11 threads use an explicit return value enum. The wrapper macros are prefixed with FLAC__ as is common in FLAC code. All functions are renamed to closely resemble the C11 names instead of the POSIX names, as this is the more forward-looking approach.
The macro to enable C11 threads is HAVE_C11THREADS. When both HAVE_C11THREADS and HAVE_PTHREAD are defined, the C11 implementation is preferred.
This does not add any build system support for C11 threads, so the current default behaviour remains unchanged.