Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

context: Make buffer thread-safe #544

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wismill
Copy link
Member

@wismill wismill commented Nov 25, 2024

Try to do minimal changes.

Partially fixes #300.

@wismill wismill added the compile-keymap Indicates a need for improvements or additions to keymap compilation label Nov 25, 2024
@wismill wismill mentioned this pull request Nov 25, 2024
@wismill wismill force-pushed the context/thread-safe-buffer branch from 51802b8 to ec44f84 Compare November 25, 2024 16:39
@bluetech
Copy link
Member

After reading the patch, I realize it's not very helpful for our purposes.

The context buffer is a "scratch buffer" that is only allocated once and reused for all scratch purposes for the context lifetime.
The problem we were facing is that it is not thread safe.
My idea was to make it thread local, so each thread gets its own scratch buffer and everything is happy.
But I missed that you can't create a tss, bind it to the context lifetime, and have the context destructor free the buffers for all threads (i.e. call xkb_context_destroy_buffer only once in xkb_context_unref).
This means we have to match an xkb_context_destroy_buffer to each xkb_context_create_buffer. Which means the buffer isn't reused...
Obviously I haven't really used the tss_ API before making my suggestion, sorry about that.

So my new suggestion is, let's scratch the scratch buffer? That is, replace the xkb_context_create_buffer/xkb_context_destroy_buffer with malloc/free. This also doesn't reuse the buffer, but it's straightforward and no threading stuff :)

Just to be clear, I do not mean to remove the idea of the buffer entirely, i.e. use malloc/free for each xkb_context_get_buffer. That would indeed introduce too many allocations. I mean allocate a buffer per text_v1_keymap_get_as_string/text_v1_keymap_new_from_names call. That seems fine to me. The hassle with this is that we'd need to pass this buffer to all of the *Text functions, siText in particular inside xkbcomp/...

Other alternatives:

  • Make the buffer thread-global not bound to context lifetime. I don't like globals...
  • Allocate it on the stack. The buffer size is 2048 bytes (it needs to be able to fit whatever is thrown into it), maybe it's fine for today's stack sizes?

WDYT?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
compile-keymap Indicates a need for improvements or additions to keymap compilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Thread safety
2 participants