Menu

libspectrum Merge Request #2: Thread safe myglib using C11 atomics (merged)

Merging...

Merged

Something went wrong. Please, merge manually

Checking if merge is possible...

Something went wrong. Please, merge manually

Original repository by BogDan Vatra is deleted

Discussion

  • BogDan Vatra

    BogDan Vatra - 2016-11-12

    Someone which knows autotools please check configure.ac changes

     
  • Fredrick Meunier

    Hi BogDan,
    I like this a lot more than the earlier versions :) It is very clean and self-contained and while it uses C11 when available, it remains compatible (without locking) with C89 as used in Fuse.

    Any thoughts about using pthreads instead for an option that works for C89 too?

    I think we should take the following additional steps before merging:

    • Update the libspectrum documentation to describe the standard thread safety features
      • I guess we are basically saying that with this change the libspectrum myglib replacement is like GLib?:
        • "GLib itself is internally completely thread-safe (all global data is automatically locked), but individual data structure instances are not automatically locked for performance reasons. For example, you must coordinate accesses to the same GHashTable from multiple threads."
    • glib structures are used in libspectrum_tape and libspectrum_ide_channel and they therefore should follow these restrictions
    • lock_hash(), unlock_hash(), lock_list() and unlock_list() should be static functions?
    • Any reason that we don't share implementations of the lock and unlock functions between the GSList and GHashTable implementations?
     
  • BogDan Vatra

    BogDan Vatra - 2016-11-14

    Well, we can use pthread too, but considering that C11 is pertty old (+5 years) all modern compilers have it. I'm afraid that pthread is not available on all OSes out there.

    • yep, I think we can ditch glib with this change.
    • I don't know anything about these structures that are used in libspectrum_tape and libspectrum_ide_channel :(
    • yep, these function should be static :)
    • for performance reason, I don't want to block a thread if it tries to add an item to a list while another one is adding something to a hash.
     
  • BogDan Vatra

    BogDan Vatra - 2016-11-14

    Where is the libspectrum documentation?

     
  • BogDan Vatra

    BogDan Vatra - 2016-11-14

    made [un]lock_XXXX functions static

     
  • Fredrick Meunier

    The libspectrum documentation is in libspectrum/doc/libspectrum.txt

    for performance reason, I don't want to block a thread if it tries to add an item to a list while another one is adding something to a hash.

    I just mean that there would be lock() and unlock() functions in a shared file with a parameter for the list or hash lock atomic to save on duplication, what do you think?

     
    • BogDan Vatra

      BogDan Vatra - 2016-11-15

      The libspectrum documentation is in libspectrum/doc/libspectrum.txt
      there is no mentioning about [my]glib in that file ...

      I just mean that there would be lock() and unlock() functions in a shared file with a parameter for the list or hash lock atomic to save on duplication, what do you think?

      Done !

       
  • BogDan Vatra

    BogDan Vatra - 2016-11-15

    Can we automatically switch to myglib if "stdatomic.h" is found? If yes, can somebody help me with configure.ac changes, as I said I have no experience with autoconf/tools.

     
    • Fredrick Meunier

      I expect that most Linux desktops will have glib installed and be using the GTK+ UI and it is better/required to use the standard glib library in those cases.

      I've refactored the patch a little to reduce duplication further in the attached patch, do you see any problems with these changes?

       
      • BogDan Vatra

        BogDan Vatra - 2016-11-17

        Yep there are two problems :)
        - I think you should use

        -# define lock_hash() atomic_lock(&atomic_locker)
        -# define unlock_hash() atomic_unlock(&atomic_locker)
        -#else
        -# define lock_hash()
        -# define unlock_hash()

        because
        static atomic_char atomic_locker = ATOMIC_VAR_INIT(0);
        is available only when #ifdef HAVE_STDATOMIC_H

        • lock and unlock functions are wrong now, because
          atomic_char unlocked = ATOMIC_VAR_INIT(0);
          atomic_char locked = ATOMIC_VAR_INIT(1);
          must be initialized everyt time before atomic_compare_exchange_strong called because if it fails it will replace it's expected paramater with the desired value (check http://en.cppreference.com/w/c/atomic/atomic_compare_exchange for more info).
         

        Last edit: BogDan Vatra 2016-11-17
        • Fredrick Meunier

          Thanks BogDan, I think I've fixed the second issue based on your feedback (and corrected the types of the variables in the functions) in the attached extra patch that applies on top of the last.

          I don't think the first point is an issue as when there is no stdatomic.h the preprocessor consumes the argment to the lock() function so no error is raised from atomic_locker being missing. Stylistically, I'd probably prefer to have an initialisation function that returns an opaque type variable we use in either case but I'm not sure there will be a payoff in this scenario.

           

          Last edit: Fredrick Meunier 2016-11-18
  • BogDan Vatra

    BogDan Vatra - 2016-11-18

    The patch is still not ok, the second paramater is changed not the third one, so the second paramater must be reset every time.

    Regarding the first point,IMHO even there is no error, it's still confusing to developers ...

    I'll update my patch with (some) of your changes.

     
  • BogDan Vatra

    BogDan Vatra - 2016-11-18

    I updated my patch with some of your changes.

     
  • Fredrick Meunier

    I just pushed another middle ground version to the merge-request-2-threadsafe branch on sf, let me know if you see any remaining bugs or have any other comments. If not, I'll merge over the weekend.

     
  • BogDan Vatra

    BogDan Vatra - 2016-11-18

    No bug, you can ship it!
    Is it any reason why you choose not to use atomic_char for locked/unlocked vars ?

     
  • Fredrick Meunier

    Thanks, will do.

    I chose that just because my compiler was telling me that expected and value (desired) are not atomic :) :

    template <typename _Tp>
    static inline bool __c11_atomic_compare_exchange_strong(
        volatile _Atomic(_Tp)* __a, _Tp* __expected, _Tp __value,
        memory_order __success, memory_order __failure) {
    
     
  • Fredrick Meunier

    • Status: open --> merged
     

Log in to post a comment.

Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.