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?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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 !
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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?
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
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
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.
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.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Someone which knows autotools please check configure.ac changes
Similar with https://sourceforge.net/p/fuse-emulator/libspectrum/merge-requests/1/ but with less changes
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:
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.
Where is the libspectrum documentation?
made [un]lock_XXXX functions static
The libspectrum documentation is in libspectrum/doc/libspectrum.txt
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 !
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.
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?
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
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
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
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.
I updated my patch with some of your changes.
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.
No bug, you can ship it!
Is it any reason why you choose not to use atomic_char for locked/unlocked vars ?
Thanks, will do.
I chose that just because my compiler was telling me that expected and value (desired) are not atomic :) :
Lot of context is lost when the original repository is deleted. See for example
https://sourceforge.net/p/fuse-emulator/libspectrum/merge-requests/1/
The pushed commits have been [9c2f9c] and [bbbcee].
Related
Commit: [9c2f9c]
Commit: [bbbcee]