xkbcommon
Thread safety
libxkbcommon can be awkward to use in a multi-threaded context given the use of reference counting in a non-thread-safe way. In particular it's proven inconvenient with how Smithay (a Rust library for building compositors) wants to use types like xkb_keymap.
Would there be any interest in making libxkbcommon APIs thread safe? Making reference counting thread-safe should be relatively simple with atomic operations. That would be enough to make xkb_keymap thread safe since it is immutable... except it contains a reference to xkb_context, which is reference counted and mutable. That is a bit more complicated to deal with.
(I'm also not sure if libxkbcommon can be used in systems where atomics and mutexes wouldn't be available, or how that would best be addressed.)
In particular it's proven inconvenient with how Smithay (a Rust library for building compositors) wants to use types like xkb_keymap
Can you expand on this a bit? I mean, why does Smithay need to share (send?) xkb_keymap between threads.
Overall not opposed to making xkb_keymap thread safe if it makes sense and is not too hard (I think all we need is atomics, we can use stdatomic.h for it, but it's not supported everywhere so would need some polyfills). But I'm curious to hear the use case.
Generally it can be awkward in Rust to deal with types that don't implement Send (i.e. they can't be safely moved between threads). Normally this trait applies, even for types that can't be used in multiple threads at once, but becomes a problem with reference counting like this.
smithay and smithay-client-toolkit have an unsafe impl Send for the types they use wrapping an xdg_state, I think mainly so they can provide a convention API without !Send types, but this means the Keymap can't be safely exposed in the public API in any way. Which has proven awkward for something I'm trying to do with these libraries (https://github.com/Smithay/smithay/pull/750, https://github.com/Smithay/client-toolkit/pull/299), though there may be a better way to deal with that.
It shouldn't be too hard to just create an independent xkb_keymap by converting it to a string and back, anyway. But it would be nice if xkb_keymap were thread-safe. Things like the log_fn in an xkb_context might complicate that; if the issue was just the refcounting in xkb_keymap this would be quite simple if support for atomics can be assumed.
Edit: So this probably isn't essential for anything in Smithay, but it may be convenient if it could be improved.
I drafted #540 for experimenting.
The only value that can be modified, once the keymap is compiled, is the reference counter. Using atomics makes it thread-safe.
WARNING: this alone does not make the keymap API thread-safe:
- It depends on the atom table in the
xkb_contextfor its strings values; this table is not thread-safe. However it is “safe” only if no other keymap is compiled using the same context. - Functions retrieving text may use the
xkb_contextinternal string buffer, which is not thread safe. API not using this buffer should be “safe”. - Functions use the
xkb_contextlog function, but modifying it is not thread-safe. - State API is not thread-safe.
I see 2) to be the main blocker: if the context is not modified, no other keymap is compiled while the previous one is in use and a brand new state is created for each thread, then it should be “safe” if not using functions working with strings.
It depends on the atom table in the xkb_context for its strings values; this table is not thread-safe. However it is “safe” only if no other keymap is compiled using the same context.
Do you know if MSVC also supports C11 mutexes? If so, I guess we can add one to atom_table and see how it affects the (uncontended single threaded) keymap compilation bench.
Functions retrieving text may use the xkb_context internal string buffer, which is not thread safe. API not using this buffer should be “safe”.
TBH I always felt bad about this one, especially since it imposes a length limit on whatever is using it. I think we can consider thread safe alternatives for this.
Functions use the xkb_context log function, but modifying it is not thread-safe.
I think it's OK to document the log set functions as being thread-unsafe, like must be done at init time or such.
State API is not thread-safe.
State should not be thread safe IMO. Trying to use it from multiple threads is probably wrong. My suggestion would be to only look into if there's a valid use case.
https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance lists C11 threads <threads.h> as added in VS 2022 17.8. So I guess it's a relatively recent addition (compared to when C11 was standardized), but should be available.
https://learn.microsoft.com/en-us/cpp/overview/visual-cpp-language-conformance lists C11 threads <threads.h> as added in VS 2022 17.8. So I guess it's a relatively recent addition (compared to when C11 was standardized), but should be available.
Good to know, thanks.
If threads.h is fully supported, then another, easy option for the xkb_context text buffer is to make it thread local (tss_create & friends). With current usage, I believe the buffer is only used temp buffers for formatting stuff with a limited lifetime, never returned directly to the user. So making it thread local is not a problem.