search

MRtrix3 / mrtrix3

MRtrix3 provides a set of tools to perform various advanced diffusion MRI analyses, including constrained spherical deconvolution (CSD), probabilistic tractography, track-density imaging, and apparent fibre density
http://www.mrtrix.org
Mozilla Public License 2.0
295 stars 182 forks source link

Document what resources should be protected by mutexes #2795

Open daljit46 opened 10 months ago

daljit46 commented 10 months ago

Doing a quick grep -r "std::mutex " . shows that we use 32 mutexes in the codebase. However, all of them are named "mutex". This is not very informative and it's unclear from the name what resources should be protected by each mutex. As a result, it's difficult for a developer when changing/adding code to understand what resource should be guarded. I think we should really rename those to include the object they protect in their name or alternatively at least provide a helpful comment alongside their declaration. Implementation of #2778 may also be helpful to make this better.

jdtournier commented 10 months ago

If you feel that's a real issue, my preference would be to implement #2778.

Lestropie commented 10 months ago

This is not very informative and it's unclear from the name what resources should be protected by each mutex.

In some contexts at least, from my comment in #2778, my class Slave::Writeback class would intrinsically protect only the members of that class, rather than the entirety of Slave. So maybe it's possible to provide selective protection via encapsulation rather than variable names. It's also the case that in a few areas, it's quite deliberate to utilise a single mutex at different points during processing, and to protect different things, hence the generic name.

Though as per @jdtournier's comment, possible that some of those mutexes might not actually be necessary.

daljit46 commented 10 months ago

I think this issue is important because anyone who touches code in a class which contains a mutex, needs to be aware of what that mutex is actually doing. Otherwise, the risk of introducing data races is likely. Of course, this is mostly relevant to our core library, which is probably the least likely part of our codebase being the subject of modifications.

Encapsulation and something like MutexProtected may not always be possible due to either performance considerations (e.g. need of guarding two member variables under different conditions by the same mutex) or requiring significant modifications to existing code. Naming is one of the hardest problem in Computer Science, but I think in this case we can do better without any significant effort.