Make Configurable/Customizable options copyable #8704
Conversation
The atomic variable "is_prepared_" was keeping Configurable objects from being copy-constructed. Removed the atomic to allow copies. Since the variable is only changed from false to true (and never back), there is no reason it had to be atomic. Added tests that simple Configurable and Customizable objects can be put on the stack and copied.
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
include/rocksdb/configurable.h
Outdated
| @@ -288,7 +288,7 @@ class Configurable { | |||
| protected: | |||
| // True once the object is prepared. Once the object is prepared, only | |||
| // mutable options can be configured. | |||
| std::atomic<bool> is_prepared_; | |||
| bool is_prepared_; | |||
There was a problem hiding this comment.
I think there's more to this. I did some more digging and actually made the same change locally, then ran the unit test suite under TSAN. A handful of remote compaction related UTs were flagged due to multiple threads racing to update this variable for the BytewiseComparator Meyers singleton while deserializing CompactionServiceInput (which includes the column family options). I'm assuming the reason the flag got turned into an atomic in #8336 in the first place was to make these warnings go away. It did not, however, solve the root cause of the issue, namely that it is not safe to use the Configurable / Customizable framework from multiple threads (without synchronization) in the presence of "global" objects like BytewiseComparator (which is what the compaction service does).
Also Cc @jay-zhuang because this affects remote compaction (and might be related to the flaky test you were looking at).
There was a problem hiding this comment.
Having said that, I support ditching the atomic since it only gives the illusion of thread safety. The race affecting the compaction service could be handled in a separate PR I think.
There was a problem hiding this comment.
The race affecting the compaction service could be handled in a separate PR I think.
Actually, scratch that part. The issue affects more than remote compaction, see e.g. how DBTest2.MultiDBParallelOpenTest fails under TSAN. I think it would be best if we fixed this in this PR.
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
options/configurable.cc
Outdated
| Configurable::Configurable(const Configurable& o) | ||
| : is_prepared_(o.is_prepared_.load()) { | ||
| options_ = o.options_; | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd say let's remove is_prepared_ / IsPrepared altogether. Two reasons:
- It's unused except for unit tests.
- This is not thread-safe regardless of whether the flag is atomic or not because the interface is inherently thread-unsafe. I.e. in the presence of multiple threads, there is a race condition in the following even if you make the flag atomic because multiple threads may find the flag unset and proceed to call
PrepareOptions:
if (!cfgable->IsPrepared()) {
Status s = cfgable->PrepareOptions(cfg_options);
// ...
}
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
|
@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: The atomic variable "is_prepared_" was keeping Configurable objects from being copy-constructed. Removed the atomic to allow copies. Since the variable is only changed from false to true (and never back), there is no reason it had to be atomic. Added tests that simple Configurable and Customizable objects can be put on the stack and copied. Pull Request resolved: #8704 Reviewed By: anand1976 Differential Revision: D30530526 Pulled By: ltamasi fbshipit-source-id: 4dd4439b3e5ad7fa396573d0b25d9fb709160576
Summary: The atomic variable "is_prepared_" was keeping Configurable objects from being copy-constructed. Removed the atomic to allow copies. Since the variable is only changed from false to true (and never back), there is no reason it had to be atomic. Added tests that simple Configurable and Customizable objects can be put on the stack and copied. Pull Request resolved: facebook/rocksdb#8704 Reviewed By: anand1976 Differential Revision: D30530526 Pulled By: ltamasi fbshipit-source-id: 4dd4439b3e5ad7fa396573d0b25d9fb709160576
The atomic variable "is_prepared_" was keeping Configurable objects from being copy-constructed. Removed the atomic to allow copies.
Since the variable is only changed from false to true (and never back), there is no reason it had to be atomic.
Added tests that simple Configurable and Customizable objects can be put on the stack and copied.