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

Add C11 threads.h implementation in addition to pthread implementation for multithreaded encoding. #822

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

manxorist
Copy link
Contributor

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.

@manxorist
Copy link
Contributor Author

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.

@manxorist
Copy link
Contributor Author

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

manxorist added a commit to OpenMPT/openmpt that referenced this pull request Mar 16, 2025
…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
@sezero
Copy link
Contributor

sezero commented Mar 16, 2025

@sezero As you mentioned the extra pthread dependency in #634 (comment), this might be relevant for you.

Sorry, really not interested

@ktmf01
Copy link
Collaborator

ktmf01 commented Mar 18, 2025

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

@manxorist
Copy link
Contributor Author

manxorist commented Mar 18, 2025

Just one comment for now (I haven't thoroughly reviewed this), can you move as much as possible into share/compat.h?

I would rather not use the global compat.h for that. For MSVC, using <threads.h> is documented to induce an additional runtime dependency on vcruntime140_threads.dll (which is redistributable and part of the compiler runtime) (see https://devblogs.microsoft.com/cppblog/c11-threads-in-visual-studio-2022-version-17-8-preview-2/). As far as I can see, this does (currently) not make use of #pragma comment(lib, [...]) (which would add the dependency from a sole #include <threads.h>), but it looks like to be implemented in the C runtime import library instead (I did not verify that). However, this fact is not documented and it would thus be relying on a current MSVC implementation detail. IFF this behaviour should ever change, everything that includes share/compat.h could inherit the dependency on the additional runtime DLL, which would not be desirable.

I can still move the abstraction macros of course, but I would rather use a distinct new compat_threads.h. Feels also a bit cleaner to not do totally separate things in a single kitchen sink abstraction header.

…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.
@manxorist
Copy link
Contributor Author

I added include/share/compat_threads.h, and also added Autotools support in a separate commit (see its commit message for the logic).

I know close to nothing about CMake, and would prefer if someone else more knowledgeable touches that.

@manxorist manxorist marked this pull request as ready for review March 20, 2025 12:35
@manxorist
Copy link
Contributor Author

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.

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.

3 participants