STIR
STIR copied to clipboard
UCL
ProjDataFromStream get/set_bin_value need to be made threadsafe
In C++, stream access is not thread-safe. get/set_viewgram etc are now "guarded" in OpenMP critical sections, but we forgot to put this in for get/set_bin_value.
Hmm, this is worrying. Have you observed any problems ? I have not observed seg faults so far.
I don't know who is using get_bin_value anyway. Maybe in one of your PR. @danieldeidda will be using set_bin_value in some code for Mediso support.
I have tried to call set_bin_value() with openMP and get seg fault. You could make that critical but then is as slow as without OMP.
It makes sense that in @danieldeidda 's current application, the thread-safe version is slow. Multi-threading only works if there's more calculations/operations than waiting.
Initially, I didn't make get_viewgram etc openMP safe, and leave it do the caller to make sure of that. However, that quickly became difficult as it implies that more of STIR needs to change, and more people need to know about openMP. On the other hand, putting critical sections into low-level code prevents people to be smarter. (I suppose that's why the C++ stream library isn't thread-safe).
Given the target audience of STIR, I think that we should make get/set_bin_value etc thread-safe. If we want (and have the time!), it would be easy to later on provide 2 versions: the thread-safe and the non-thread-safe version for the smart people among us.
Final note: an omp critical section is a blunt tool. As read-access to streams is thread-safe between different streams, it would be possible to make the "waiting" stream-specific. This would be good to do, as someone could then go and open multiple read-only streams for one file and write a smart application. you then need mutexes etc to implement this. Volunteers welcome! (but it wouldn't resolve the write issue of course).
is the following included in the STIR 4.0? Author: Daniel Deidda [email protected] Date: Thu Apr 2 11:51:03 2020 +0100
ProjDataFromStream, adding set_bin_value, fixing #462: (#469)
* ProjDataFromStream: added set_bin_value(Bin)
* ProjDataInMemory: removed get_bin_value such that we use the default implementation
* add test for get/set_bin_value() in test/test_proj_data_in_memory
I am getting an error looking for set_bin_value()
ah. sorry. this was merged on master.
I think best thing to do is to merge release_4 onto master, and you can work from there. is that ok?
the merge might be a bit difficult of course. Possibly @rijobro could initiate it?
ah. sorry. this was merged on
master.I think best thing to do is to merge release_4 onto master, and you can work from there. is that ok?
yes sure
This issue remains open. ProjDataFromStream::get_bin_value is not thread-safe as stream operations are not. ProjDataInMemory::get_bin_value() is thread-safe for read-only cases (it wouldn't be when using read/write access in a multi-threaded context, but I guess we don't do that).
It is probably best to leave this behaviour, and just documented it. Making it thread-safe will probably make it almost useless (i.e. a bit slower than unthreaded as @danieldeidda wrote above). Code that uses get_bin_value has to think about thread-safe access therefore. That's unfortunate, but I don't see a better solution.
Doing a search throws up very few uses for this function (a lot are Bin::get_bin_value() but that is thread-safe). At the moment, outside of our tests, I could only find two occurrences:
https://github.com/UCL/STIR/blob/5f5fd2d745ec0f49b0fe1bc70ff57b7914519d42/src/recon_buildblock/BinNormalisationFromGEHDF5.cxx#L581
but this uses ProjDataInMemory.
https://github.com/UCL/STIR/blob/4142d5969e4320e32ed9dbe463b0fd50f36fb3f6/src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndListModeDataWithProjMatrixByBin.cxx#L885 but this is unthreaded.
This sounds sensible to me. People that read and write the same file in a multi-threaded context can be expected to be knowledgeable enough to deal with this risk.