-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the library threadsafe #109
Conversation
6c14c9b
to
6ca3c15
Compare
result = (float*)malloc(n*sizeof(float)); | ||
for (i=0; i<n; i++) | ||
result[i] = a + (b-a) * rand() / ((double)RAND_MAX); | ||
|
||
return result; | ||
} | ||
|
||
double gaussrand() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I also removed this because while looking for static
s in the codebase that need to be updated for thread-safety, I've noticed that those functions are unused altogether, so just removed them.
@@ -33,7 +33,7 @@ | |||
#define UNKNOWN_NOISE_TYPE 10 | |||
|
|||
#define BIG 1e+30 /* a huge number (< biggest value a float can store) */ | |||
#define PI 3.1415926535898 | |||
#define PI M_PI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that clang-tidy
also noticed during its runs and thought it's worth the minor cleanup too.
I can split out |
Also I still think it's a good idea to make some refactorings to isolate data further, but I think it's worth implementing the minimal / least invasive change first, that already guarantees thread-safety at least for independent inputs and outputs isolated to their own threads, and then it's easier to do further improvements on top of it. |
@kbarbary Hey! I suspect you're busy, but just in case it got miseed - does this approach make sense in general? |
Sorry for the delay, this is really impressive work! It looks good to me. It might be worth adding a note to the unreleased version in CHANGES.md explaining the implications. I think you'd be more qualified to write it than me. Want to add that and I'll merge it? |
Ah I see you used |
6ea79e9
to
3c957b4
Compare
As mentioned in kbarbary#81, currently the library is not thread-safe, that is, it can't even be used on multiple separate images in parallel due to the shared global mutable variables. There are a couple of ways to fix this. I've mostly done refactoring in a separate branch to pass everything around via params, but that kind of a change is rather huge and harder to review for correctness. So I took a different approach I mentioned in kbarbary#81 (comment), and instead went through all the statics and changed them depending on usage: 1. Into local function variables where they are initialized every time anyway - those clearly don't have to be `static`. 2. Into `const static` if it was a static that's been always initialised to the same constant value - there was just `initinfo` that fell into this category. 3. Into `_Atomic` if it's a global config - in particular `sub_object_limit` and `extract_pixstack` params. I believe there is no point in configuring those per-thread, so just left them as globals, but `_Atomic` ensures safe complete initialization in case of a race. 4. Finally, made all the other mutable statics `_Thread_local`, which ensures that none of the threads will override each others' data while storing temporary information about the image being processed. Then, I used `clang-tidy` to: 1. Find and auto-mark all places where pointers can be `const` - this allows consumer to more easily reason about which data can be shared across images and/or threads, and which will be mutated internally by SEP and so needs to have separate copies per run. 2. Detect all non-thread-safe functions (clang-tidy can do this starting from LLVM 13). Such function was only `rand`, so I added a `_Thread_local` seed variable `randseed`, and switched those callsites to `rand_r(&randseed)`, making those thread-safe too. Note that due to trimming of unnecessary spacing by clang-tidy and editors the diffs can look a bit more bloated, so I suggest to review the diff using "ignore whitespace changes" mode here: https://github.com/kbarbary/sep/compare/master...RReverser:simple-threadsafe?expand=1&w=1 Overall I think this is the least invasive change that should be relatively easy to review for correctness (and, of course, I checked that all the tests are passing), while at the same time ensures thread-safety throughout the codebase. Fixes kbarbary#81.
5807e09
to
5545ddc
Compare
5545ddc
to
6b9f5f8
Compare
There is a lot more in terms of pointers that could be constified, but they all need manual review, and I'll probably leave them for a separate PR. |
@kbarbary Thanks! |
Thank you! Again, great work. |
Should I do a version release? |
@kbarbary I think tagging 1.2.0 and updating |
As mentioned in #81, currently the library is not thread-safe, that is, it can't even be used on multiple separate images in parallel due to the shared global mutable variables.
There are a couple of ways to fix this. I've mostly done refactoring in a separate branch to pass everything around via params, but that kind of a change is rather huge and harder to review for correctness.
So I took a different approach I mentioned in #81 (comment), and instead went through all the statics and changed them depending on usage:
static
.const static
if it was a static that's been always initialised to the same constant value - there was justinitinfo
that fell into this category._Atomic
if it's a global config - in particularsub_object_limit
andextract_pixstack
params. I believe there is no point in configuring those per-thread, so just left them as globals, but_Atomic
ensures safe complete initialization in case of a race._Thread_local
, which ensures that none of the threads will override each others' data while storing temporary information about the image being processed.Then, I used
clang-tidy
to:const
- this allows consumer to more easily reason about which data can be shared across images and/or threads, and which will be mutated internally by SEP and so needs to have separate copies per run.rand
, so I added a_Thread_local
seed variablerandseed
, and switched those callsites torand_r(&randseed)
, making those thread-safe too.Note that due to trimming of unnecessary spacing by clang-tidy and editors the diffs can look a bit more bloated, so I suggest to review the diff using "ignore whitespace changes" mode here: https://github.com/kbarbary/sep/compare/master...RReverser:simple-threadsafe?expand=1&w=1
Overall I think this is the least invasive change that should be relatively easy to review for correctness (and, of course, I checked that all the tests are passing), while at the same time ensures thread-safety throughout the codebase.
Fixes #81.