Add inline shared lock #785
Conversation
|
I addressed most comments in the latest commit. I'll look into converting this namespace to a class and aligning the predicate checks with their descriptive comments tomorrow. |
|
I think I've addressed all pending suggestions except refactoring into a class. I think I'll need to address that after some higher-priority work. |
|
Changed this to a class. I don't think it's actually an improvement, but I need to get this merged... |
| { | ||
| namespace common | ||
| { | ||
| class inline_shared_lock |
There was a problem hiding this comment.
Is the inline for performance purposes? If so it may not be relevant, https://stackoverflow.com/a/1759575/1214125.
Now I don;t know enough about these compiler things to detect whether it is worthwhile here or not. Keep in mind that by not having a cpp, you increase compilation time.
I think I already had this discussion in the past.
There was a problem hiding this comment.
No, the name of the class indicates that it's designed to be "inlined" into a shared-memory data structure like our chunk header. There's probably a better name to convey this notion though. (The name made more sense originally when the whole lock was just a single atomic<uint64_t>, not a class.)
There was a problem hiding this comment.
I meant the inline methods within the class (I know that class names do not affect the compilation process)
There was a problem hiding this comment.
I would only mark as inline and keep in the header the shorter helper methods like is_free. But the actual lock acquiring/releasing methods could be moved to a cpp file.
There was a problem hiding this comment.
I don't agree with that, since methods cannot be inlined if I move them into a .cpp file (except within that translation unit itself).
There was a problem hiding this comment.
I thought that we discussed this before and there was a way for compilers to do this.
See more discussion and examples of compilers that already do such things here, for example:
https://stackoverflow.com/questions/8450030/can-modern-c-compilers-inline-functions-that-are-defined-in-a-cpp-file
I think you're overthinking this anyway. On one hand you've argued that we shouldn't optimize this code because it's not used in a critical path (yet). OTOH you're now concerned about the questionable loss of performance optimization due to the compiler not being able to inline this code.
There was a problem hiding this comment.
Using LTO can massively slow compile times, so it would have to be a carefully considered decision (clang thinLTO is supposed to be faster but I haven't tried it). By keeping potentially hot paths in inline functions, I'm giving the compiler maximum flexibility, without assuming that LTO is enabled.
| { | ||
| namespace common | ||
| { | ||
| class inline_shared_lock |
There was a problem hiding this comment.
Please add a line between the namespace declarations and what is defined inside them (both at beginning and end)?
I would also move the comment just before the class declaration, because in the future, this file may get some additional supporting declarations.
|
|
||
| // NB: This cannot be used safely in program logic without synchronization, | ||
| // and is intended only for logging/debugging. | ||
| size_t get_reader_count(); |
There was a problem hiding this comment.
If this is for debugging, it should definitely be private.
There was a problem hiding this comment.
No, it's not for use inside the class.
There was a problem hiding this comment.
Why don't you actually make this safe then? You only need to read the value from the atomic once and then do all the checks on that value, so the answer is consistent. You already have the helpers required for this approach.
There was a problem hiding this comment.
I don't think it can really be used safely in a lock-free context (even double-checking after use would require safe retry and could entail an ABA hazard). But you're right that I could at least make it atomic, which I've done in the pending commit.
There was a problem hiding this comment.
Last implementation looks good to me. I'd remove this comment because the same thing could be said about is_free and any other check whose result may already be obsolete by the time it is processed.
|
|
||
| std::atomic<uint64_t> m_lock_word{c_free_lock}; | ||
|
|
||
| inline bool is_valid(); |
There was a problem hiding this comment.
I would add another private label before this section of methods and before the field declaration. I like to separate the fields from the constants/methods so one can quickly figure out what a class instance contains as data.
| ASSERT_INVARIANT(is_valid(), "Invalid state for lock word!"); | ||
| } | ||
|
|
||
| size_t inline_shared_lock::get_reader_count() |
There was a problem hiding this comment.
All the functions below could go into a cpp file. We could also agree to not bother with inline methods at all and move everything into a cpp file and let the compiler figure things out.
There was a problem hiding this comment.
The compiler cannot "figure things out" if you move the methods into a .cpp file. It can't inline anything outside the current translation unit.
There was a problem hiding this comment.
BTW, I will move the inline keyword to the definitions rather than the declarations, based on the arguments here: https://isocpp.org/wiki/faq/inline-functions#where-to-put-inline-keyword.
There was a problem hiding this comment.
It seems the linker can inline functions for which definition is not available. Didn't find a lot about this so it's probably safe to keep inline for now.
There was a problem hiding this comment.
There was a problem hiding this comment.
I think that can only happen with LTO, which I don't think we currently use (maybe we should!).
There was a problem hiding this comment.
Copying an answer to a different comment that is relevant to this one as well:
I thought that we discussed this before and there was a way for compilers to do this.
See more discussion and examples of compilers that already do such things here, for example:
https://stackoverflow.com/questions/8450030/can-modern-c-compilers-inline-functions-that-are-defined-in-a-cpp-file
I think you're overthinking this anyway. On one hand you've argued that we shouldn't optimize this code because it's not used in a critical path (yet). OTOH you're now concerned about the questionable loss of performance optimization due to the compiler not being able to inline this code.
This is being added so I can embed a shared-memory rwlock in the chunk header (to enforce mutual exclusion between compaction and GC tasks, while allowing concurrency between GC tasks). Note that no blocking facility is provided, because I don't need it for this application (and the implementation is fairly trivial without it).