Skip to content
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

Document what resources should be protected by mutexes #2795

Open
daljit46 opened this issue Feb 1, 2024 · 3 comments
Open

Document what resources should be protected by mutexes #2795

daljit46 opened this issue Feb 1, 2024 · 3 comments

Comments

@daljit46
Copy link
Member

daljit46 commented Feb 1, 2024

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.

@daljit46 daljit46 self-assigned this Feb 1, 2024
@jdtournier
Copy link
Member

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

@Lestropie
Copy link
Member

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
Copy link
Member Author

daljit46 commented Feb 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants