Thread safety (tests still missing)

[emoon]

Hi,

First of all thanks for a great library!

I wonder if there are any plans on making the code thread-safe? My use case is that I want to support play more than one song at a time (examples being cross fading, multi decode of many files at the same time, etc)

The current problem is that esp gbhw.c has lots of global variables. On way to solve this would be to setup a "state object" as the first parameter to all gbhw_ functions could take that instead of using the global variables. That way several threads can have their own "hardware" without conflicting with each-other

[mmitch]

That is a nice idea! I have started to do this in the multi-gbs branch which does not compile yet. I'll just extract everything into a struct as you suggested and see where it gets us :-) Full thread-safety might still be a long way to go because then all functions would need to be fully reentrant - I don't know if we can guarantee this. But running everything over a gbhw handle like you suggested should work – it's just a bit of refactoring work.

[mmitch]

Checklist for things that have come up:

• [x] check if libimpulse supports multiple buffers
  • we allocate a buffer internally, so no cross-talk between gbs instances
• [x] check if libimpulse is thread-safe (should not be required for just using global handles)
  • does not matter as we only do everything by handle, not real thread-safeness
  • actually, we do the impulse handling by ourself in gbhw.c:gb_change_level() - the impulse itself is constant (from compile-time generated impulse.h) and the impulse buffer is part of struct gbhw which is part of struct gbs
• [x] expose all internals via struct gbhw* & co in the libgbs interface? will this stay compatible if we have internal changes in the structs? if not, how do we tackle this instead? ==> forward declare structs, see below
  • forward declared structs are umplemented
• [x] turn all struct gbhw *gbhw into struct gbhw *const gbhw? same for gbs and gbcpu
  • unneeded because internally we need to work with those structs. externally we use a forward declared struct, so the internals are inaccessible
• [x] move or rename xxx_handle_init() functions
  • xxx_init_struct() it is

[emoon]

you don't need to expose the internals really, you can just forward declare struct gbhw; in the gbs.h and it will just take the pointer in each function so that pointer will just be "passed down" to specific gbhw_<func> that includes the real header with the internal info.

[emoon]

An alternative to bump to 1.0 is to do something like this

inline void gbhw_setrate(int rate) { gbhw_state_setrate(&g_global_state, rate); }

Now the old API can still work and it makes it possible for people to transition to the gbhw_state_* API instead, but up to you :) Using the old API of course isn't thread safe, but it would still work with compiling the code and run as before.

[mmitch]

I foresee some problems with API stability and forward declaration of structs (currently code outside of gbhw accesses the channel data directly, this will probably need some new functions). So for now I created the new issue #42 to address the API stability. This issue will just focus on the changes to use multiple gbhw instances at the same time.

[mmitch]

The new interface that basically encapsulates everything inside the struct gbs works. As keep piling up more and more refactorings in the multi-lib branch, I'll just merge that branch to master if there are no objections against it in the near future ;-) As of now the libgbs interface becomes incompatible, but want to tackle that separately in issue #42. Perhaps it is not bad that I rename all the external gbs_* functions – that way we can easily build a backwards compatible layer to the old interface ;-)

[mmitch]

The branch has been merged to master. You can try it, but as already discussed the API has changed and will change again, it is neither stabilized nor finalized yet.

[emoon]

Cool. Thanks!

[mmitch]

btw:

• We only change data from our own struct gbs* handle.
• Variables local to the functions are never static and thus should be on the stack.
• Everything static is const.

This would mean that we are thread-safe and re-entrant now, right? (unless I missed a variable)

[emoon]

yeah, as long as the code only works with it's own owned data and doesn't modify global state it's all fine.

To validate this is true the best way would be to write a test for it. The test could look something like this:

  // singe threaded test
  songs = ["song1.gbs", "song2.gbs", "song3.gbs"];
  hashes_single = [];
  hashes_multi = [];

  for (song in songs) hashes_single[song] = calc_hash(render_song(song));

 // multi test
 for (song in songs) {
   spawn_thread( hashes_multi[song] = calc_hash(render_song(song)) );
 }
 // wait all threads
 ...
 // compare hashes between single and multi  
 ... 

[mmitch]

I had something in mind along these lines. We currently only have one demo song – so either we would have to play it multiple times (which might hide some errors if we're unlucky) or we perhaps should write three very simple demo songs. Perhaps playing some simple scales would be enough. This would be a nice side project, I never wrote a GBS before :) Having a demo song in source form (assembly or C) in here would be a nice form of additional documentation of the GBS format I think.