libxkbcommon icon indicating copy to clipboard operation
libxkbcommon copied to clipboard

context: Make buffer thread-safe

Open wismill opened this issue 1 year ago • 1 comments

Try to do minimal changes.

Partially fixes #300.

wismill avatar Nov 25 '24 16:11 wismill

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?

bluetech avatar Jan 18 '25 21:01 bluetech