From 84c5bd7eb9e3db101b562eddc747cf15e1fed76d Mon Sep 17 00:00:00 2001 From: agiardullo Date: Wed, 8 Apr 2015 21:10:35 -0700 Subject: [PATCH] Add thread-safety documentation to MemTable and related classes Summary: Other than making some class members private, this is a documentation-only change Test Plan: unit tests Reviewers: sdong, yhchiang, igor Reviewed By: igor Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D36567 --- db/column_family.h | 18 ++++++++---- db/memtable.h | 52 ++++++++++++++++++++++++++++++++--- db/memtable_list.h | 8 ++++++ include/rocksdb/memtablerep.h | 4 ++- 4 files changed, 71 insertions(+), 11 deletions(-) diff --git a/db/column_family.h b/db/column_family.h index 51c656d1a..e90a757c4 100644 --- a/db/column_family.h +++ b/db/column_family.h @@ -85,24 +85,23 @@ class ColumnFamilyHandleInternal : public ColumnFamilyHandleImpl { // holds references to memtable, all immutable memtables and version struct SuperVersion { + // Accessing members of this class is not thread-safe and requires external + // synchronization (ie db mutex held or on write thread). MemTable* mem; MemTableListVersion* imm; Version* current; MutableCFOptions mutable_cf_options; - std::atomic refs; - // We need to_delete because during Cleanup(), imm->Unref() returns - // all memtables that we need to free through this vector. We then - // delete all those memtables outside of mutex, during destruction - autovector to_delete; // Version number of the current SuperVersion uint64_t version_number; + InstrumentedMutex* db_mutex; // should be called outside the mutex SuperVersion() = default; ~SuperVersion(); SuperVersion* Ref(); - + // If Unref() returns true, Cleanup() should be called with mutex held + // before deleting this SuperVersion. bool Unref(); // call these two methods with db mutex held @@ -121,6 +120,13 @@ struct SuperVersion { static int dummy; static void* const kSVInUse; static void* const kSVObsolete; + + private: + std::atomic refs; + // We need to_delete because during Cleanup(), imm->Unref() returns + // all memtables that we need to free through this vector. We then + // delete all those memtables outside of mutex, during destruction + autovector to_delete; }; extern ColumnFamilyOptions SanitizeOptions(const DBOptions& db_options, diff --git a/db/memtable.h b/db/memtable.h index 66fc98282..aa26b321f 100644 --- a/db/memtable.h +++ b/db/memtable.h @@ -54,6 +54,19 @@ struct MemTableOptions { Logger* info_log; }; +// Note: Many of the methods in this class have comments indicating that +// external synchromization is required as these methods are not thread-safe. +// It is up to higher layers of code to decide how to prevent concurrent +// invokation of these methods. This is usually done by acquiring either +// the db mutex or the single writer thread. +// +// Some of these methods are documented to only require external +// synchronization if this memtable is immutable. Calling MarkImmutable() is +// not sufficient to guarantee immutability. It is up to higher layers of +// code to determine if this MemTable can still be modified by other threads. +// Eg: The Superversion stores a pointer to the current MemTable (that can +// be modified) and a separate list of the MemTables that can no longer be +// written to (aka the 'immutable memtables'). class MemTable { public: struct KeyComparator : public MemTableRep::KeyComparator { @@ -72,13 +85,18 @@ class MemTable { const MutableCFOptions& mutable_cf_options, WriteBuffer* write_buffer); + // Do not delete this MemTable unless Unref() indicates it not in use. ~MemTable(); // Increase reference count. + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable. void Ref() { ++refs_; } // Drop reference count. - // If the refcount goes to zero return this memtable, otherwise return null + // If the refcount goes to zero return this memtable, otherwise return null. + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable. MemTable* Unref() { --refs_; assert(refs_ >= 0); @@ -92,7 +110,7 @@ class MemTable { // data structure. // // REQUIRES: external synchronization to prevent simultaneous - // operations on the same MemTable. + // operations on the same MemTable (unless this Memtable is immutable). size_t ApproximateMemoryUsage(); // This method heuristically determines if the memtable should continue to @@ -120,6 +138,9 @@ class MemTable { // Add an entry into memtable that maps key to value at the // specified sequence number and with the specified type. // Typically value will be empty if type==kTypeDeletion. + // + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable. void Add(SequenceNumber seq, ValueType type, const Slice& key, const Slice& value); @@ -142,6 +163,9 @@ class MemTable { // update inplace // else add(key, new_value) // else add(key, new_value) + // + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable. void Update(SequenceNumber seq, const Slice& key, const Slice& value); @@ -155,6 +179,9 @@ class MemTable { // update inplace // else add(key, new_value) // else return false + // + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable. bool UpdateCallback(SequenceNumber seq, const Slice& key, const Slice& delta); @@ -165,29 +192,46 @@ class MemTable { size_t CountSuccessiveMergeEntries(const LookupKey& key); // Get total number of entries in the mem table. + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable (unless this Memtable is immutable). uint64_t num_entries() const { return num_entries_; } + // Get total number of deletes in the mem table. + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable (unless this Memtable is immutable). uint64_t num_deletes() const { return num_deletes_; } // Returns the edits area that is needed for flushing the memtable VersionEdit* GetEdits() { return &edit_; } // Returns if there is no entry inserted to the mem table. + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable (unless this Memtable is immutable). bool IsEmpty() const { return first_seqno_ == 0; } // Returns the sequence number of the first element that was inserted - // into the memtable + // into the memtable. + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable (unless this Memtable is immutable). SequenceNumber GetFirstSequenceNumber() { return first_seqno_; } // Returns the next active logfile number when this memtable is about to // be flushed to storage + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable. uint64_t GetNextLogNumber() { return mem_next_logfile_number_; } // Sets the next active logfile number when this memtable is about to // be flushed to storage + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable. void SetNextLogNumber(uint64_t num) { mem_next_logfile_number_ = num; } - // Notify the underlying storage that no more items will be added + // Notify the underlying storage that no more items will be added. + // REQUIRES: external synchronization to prevent simultaneous + // operations on the same MemTable. + // After MarkImmutable() is called, you should not attempt to + // write anything to this MemTable(). (Ie. do not call Add() or Update()). void MarkImmutable() { table_->MarkReadOnly(); allocator_.DoneAllocating(); diff --git a/db/memtable_list.h b/db/memtable_list.h index 6573728a1..6e1be0768 100644 --- a/db/memtable_list.h +++ b/db/memtable_list.h @@ -35,6 +35,9 @@ class MergeIteratorBuilder; // keeps a list of immutable memtables in a vector. the list is immutable // if refcount is bigger than one. It is used as a state for Get() and // Iterator code paths +// +// This class is not thread-safe. External synchronization is required +// (such as holding the db mutex or being on the write thread). class MemTableListVersion { public: explicit MemTableListVersion(MemTableListVersion* old = nullptr); @@ -77,6 +80,11 @@ class MemTableListVersion { // flushes can occur concurrently. However, they are 'committed' // to the manifest in FIFO order to maintain correctness and // recoverability from a crash. +// +// +// Other than imm_flush_needed, this class is not thread-safe and requires +// external synchronization (such as holding the db mutex or being on the +// write thread.) class MemTableList { public: // A list of memtables. diff --git a/include/rocksdb/memtablerep.h b/include/rocksdb/memtablerep.h index 97141cc73..c369e888a 100644 --- a/include/rocksdb/memtablerep.h +++ b/include/rocksdb/memtablerep.h @@ -84,7 +84,9 @@ class MemTableRep { virtual bool Contains(const char* key) const = 0; // Notify this table rep that it will no longer be added to. By default, does - // nothing. + // nothing. After MarkReadOnly() is called, this table rep will not be + // written to (ie No more calls to Allocate(), Insert(), or any writes done + // directly to entries accessed through the iterator.) virtual void MarkReadOnly() { } // Look up key from the mem table, since the first key in the mem table whose