-
Notifications
You must be signed in to change notification settings - Fork 75
refactor: optimize with shared_lock/unique_lock for InMemoryCatalog #405
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
Conversation
| Status InMemoryCatalog::CreateNamespace( | ||
| const Namespace& ns, const std::unordered_map<std::string, std::string>& properties) { | ||
| std::lock_guard guard(mutex_); | ||
| std::unique_lock lock(mutex_); |
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.
Keep std::lock_guard if you only need write guard here.
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.
std::lock_guard cannot be used to acquire a write lock on a std::shared_mutex
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.
std::lock_guard is a template wrapper (cppreference link). Any lock which meets BasicLockable can be used in std::lock_guard, including std::shared_mutex.
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.
I was wrong — lock_guard can indeed lock a shared_mutex, but it is still recommended to use unique_lock and shared_lock for read-write locking on a shared_mutex for better code readability.
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.
I don't think there are some differences about code readability here. std::unique_lock has additional overhead but std::scoped_lock and std::lock_guard don't. It should not be compared with std::scoped_lock or std::lock_guard.
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.
Using unique_lock and shared_lock with shared_mutex makes it clear that this is a read-write lock, as seeing unique_lock immediately indicates that exclusive (write) access is being acquired. And, in other mutex scenarios, use lock_guard.
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.
+1 on std::lock_guard if we care the performance here.
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.
Compiler can optimize the simple scene (godbolt). But I still suggest to keep the code from doing unnecessary things. The simpler the better.
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.
I don't think it's unnecessary, and I recommend use the unique_lock/shared_lock pair for locking shared_mutex
795a5d6 to
4e89954
Compare
db5d805 to
fe4b32d
Compare
| ICEBERG_ASSIGN_OR_RAISE(auto metadata, | ||
| TableMetadataUtil::Read(*file_io_, metadata_file_location)); |
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.
It might be delayed until register is successful. Or we can add a new lockless LoadTableImpl to wrap the internal.
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.
Here's an issue: if a file is invalid, register returns failure, but the map may already contain an entry. Subsequent calls to register will then return "already exists".
Therefore, I think we should validate the file's legitimacy in advance, ensuring that only valid table metadata can be successfully registered.
| #include <mutex> | ||
| #include <shared_mutex> | ||
|
|
||
| #include "iceberg/exception.h" |
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.
Please not add this since we cannot throw in most of the codebase.
| Status InMemoryCatalog::CreateNamespace( | ||
| const Namespace& ns, const std::unordered_map<std::string, std::string>& properties) { | ||
| std::lock_guard guard(mutex_); | ||
| std::unique_lock lock(mutex_); |
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.
+1 on std::lock_guard if we care the performance here.
fe4b32d to
c8a49c3
Compare
No description provided.