[RocksDB] cleanup EnvOptions

Summary: This diff simplifies EnvOptions by treating it as POD, similar to Options. - virtual functions are removed and member fields are accessed directly. - StorageOptions is removed. - Options.allow_readahead and Options.allow_readahead_compactions are deprecated. - Unused global variables are removed: useOsBuffer, useFsReadAhead, useMmapRead, useMmapWrite Test Plan: make check; db_stress Reviewers: dhruba CC: leveldb Differential Revision: https://reviews.facebook.net/D11175
mdeora · Jun 12, 2013 · bdf1085 · bdf1085
1 parent 5679107
commit bdf1085
Show file tree

Hide file tree

Showing 24 changed files with 103 additions and 180 deletions.
diff --git a/db/builder.cc b/db/builder.cc
@@ -19,7 +19,7 @@ namespace leveldb {
 Status BuildTable(const std::string& dbname,
                   Env* env,
                   const Options& options,
-                  const StorageOptions& soptions,
+                  const EnvOptions& soptions,
                   TableCache* table_cache,
                   Iterator* iter,
                   FileMetaData* meta,

diff --git a/db/builder.h b/db/builder.h
@@ -8,14 +8,14 @@
 #include "leveldb/comparator.h"
 #include "leveldb/status.h"
 #include "leveldb/types.h"
-#include "util/storage_options.h"
 
 namespace leveldb {
 
 struct Options;
 struct FileMetaData;
 
 class Env;
+class EnvOptions;
 class Iterator;
 class TableCache;
 class VersionEdit;
@@ -28,7 +28,7 @@ class VersionEdit;
 extern Status BuildTable(const std::string& dbname,
                          Env* env,
                          const Options& options,
-                         const StorageOptions& soptions,
+                         const EnvOptions& soptions,
                          TableCache* table_cache,
                          Iterator* iter,
                          FileMetaData* meta,

diff --git a/db/db_bench.cc b/db/db_bench.cc
@@ -1157,7 +1157,6 @@ unique_ptr<char []> GenerateKeyFromInt(int v, const char* suffix = "")
     options.allow_readahead = FLAGS_use_fsreadahead;
     options.allow_mmap_reads = FLAGS_use_mmap_reads;
     options.allow_mmap_writes = FLAGS_use_mmap_writes;
-    options.allow_readahead_compactions = FLAGS_use_readahead_compactions;
     options.advise_random_on_open = FLAGS_advise_random_on_open;
     options.access_hint_on_compaction_start = FLAGS_compaction_fadvice;
 
@@ -1714,7 +1713,7 @@ unique_ptr<char []> GenerateKeyFromInt(int v, const char* suffix = "")
 
   void HeapProfile() {
     char fname[100];
-    StorageOptions soptions;
+    EnvOptions soptions;
     snprintf(fname, sizeof(fname), "%s/heap-%04d", FLAGS_db, ++heap_counter_);
     unique_ptr<WritableFile> file;
     Status s = FLAGS_env->NewWritableFile(fname, &file, soptions);
@@ -1742,12 +1741,9 @@ int main(int argc, char** argv) {
     leveldb::Options().max_background_compactions;
   // Compression test code above refers to FLAGS_block_size
   FLAGS_block_size = leveldb::Options().block_size;
-  FLAGS_use_os_buffer = leveldb::StorageOptions().UseOsBuffer();
-  FLAGS_use_fsreadahead = leveldb::StorageOptions().UseReadahead();
-  FLAGS_use_mmap_reads = leveldb::StorageOptions().UseMmapReads();
-  FLAGS_use_mmap_writes = leveldb::StorageOptions().UseMmapWrites();
-  FLAGS_use_readahead_compactions =
-    leveldb::StorageOptions().UseReadaheadCompactions();
+  FLAGS_use_os_buffer = leveldb::EnvOptions().use_os_buffer;
+  FLAGS_use_mmap_reads = leveldb::EnvOptions().use_mmap_reads;
+  FLAGS_use_mmap_writes = leveldb::EnvOptions().use_mmap_writes;
 
   std::string default_db_path;
 

diff --git a/db/db_impl.cc b/db/db_impl.cc
@@ -2393,8 +2393,8 @@ Status DBImpl::MakeRoomForWrite(bool force) {
       assert(versions_->PrevLogNumber() == 0);
       uint64_t new_log_number = versions_->NewFileNumber();
       unique_ptr<WritableFile> lfile;
-      StorageOptions soptions(storage_options_);
-      soptions.DisableMmapWrites();
+      EnvOptions soptions(storage_options_);
+      soptions.use_mmap_writes = false;
       s = env_->NewWritableFile(
             LogFileName(dbname_, new_log_number),
             &lfile,
@@ -2668,7 +2668,7 @@ DB::~DB() { }
 Status DB::Open(const Options& options, const std::string& dbname,
                 DB** dbptr) {
   *dbptr = nullptr;
-  StorageOptions soptions;
+  EnvOptions soptions;
 
   if (options.block_cache != nullptr && options.no_block_cache) {
     return Status::InvalidArgument(
@@ -2686,7 +2686,7 @@ Status DB::Open(const Options& options, const std::string& dbname,
   if (s.ok()) {
     uint64_t new_log_number = impl->versions_->NewFileNumber();
     unique_ptr<WritableFile> lfile;
-    soptions.DisableMmapWrites();
+    soptions.use_mmap_writes = false;
     s = options.env->NewWritableFile(LogFileName(dbname, new_log_number),
                                      &lfile, soptions);
     if (s.ok()) {

diff --git a/db/db_impl.h b/db/db_impl.h
@@ -366,7 +366,7 @@ class DBImpl : public DB {
   SequenceNumber last_flushed_sequence_;
 
   // The options to access storage files
-  const StorageOptions storage_options_;
+  const EnvOptions storage_options_;
 
   // No copying allowed
   DBImpl(const DBImpl&);

diff --git a/db/db_test.cc b/db/db_test.cc
@@ -20,7 +20,6 @@
 #include "util/mutexlock.h"
 #include "util/testharness.h"
 #include "util/testutil.h"
-#include "util/storage_options.h"
 #include "utilities/merge_operators.h"
 
 namespace leveldb {
@@ -606,7 +605,7 @@ TEST(DBTest, LevelLimitReopen) {
 TEST(DBTest, Preallocation) {
   const std::string src = dbname_ + "/alloc_test";
   unique_ptr<WritableFile> srcfile;
-  const StorageOptions soptions;
+  const EnvOptions soptions;
   ASSERT_OK(env_->NewWritableFile(src, &srcfile, soptions));
   srcfile->SetPreallocationBlockSize(1024 * 1024);
 
@@ -2383,7 +2382,7 @@ TEST(DBTest, BloomFilter) {
 
 TEST(DBTest, SnapshotFiles) {
   Options options = CurrentOptions();
-  const StorageOptions soptions;
+  const EnvOptions soptions;
   options.write_buffer_size = 100000000;        // Large write buffer
   Reopen(&options);
 
@@ -3299,7 +3298,7 @@ void BM_LogAndApply(int iters, int num_base_files) {
 
   InternalKeyComparator cmp(BytewiseComparator());
   Options options;
-  StorageOptions sopt;
+  EnvOptions sopt;
   VersionSet vset(dbname, &options, sopt, nullptr, &cmp);
   ASSERT_OK(vset.Recover());
   VersionEdit vbase(vset.NumberLevels());

diff --git a/db/repair.cc b/db/repair.cc
@@ -105,7 +105,7 @@ class Repairer {
   std::vector<uint64_t> logs_;
   std::vector<TableInfo> tables_;
   uint64_t next_file_number_;
-  const StorageOptions storage_options_;
+  const EnvOptions storage_options_;
 
   Status FindFiles() {
     std::vector<std::string> filenames;

diff --git a/db/table_cache.cc b/db/table_cache.cc
@@ -26,7 +26,7 @@ static void UnrefEntry(void* arg1, void* arg2) {
 
 TableCache::TableCache(const std::string& dbname,
                        const Options* options,
-                       const StorageOptions& storage_options,
+                       const EnvOptions& storage_options,
                        int entries)
     : env_(options->env),
       dbname_(dbname),

diff --git a/db/table_cache.h b/db/table_cache.h
@@ -14,7 +14,6 @@
 #include "leveldb/cache.h"
 #include "port/port.h"
 #include "table/table.h"
-#include "util/storage_options.h"
 
 namespace leveldb {
 
@@ -23,7 +22,7 @@ class Env;
 class TableCache {
  public:
   TableCache(const std::string& dbname, const Options* options,
-             const StorageOptions& storage_options, int entries);
+             const EnvOptions& storage_options, int entries);
   ~TableCache();
 
   // Return an iterator for the specified file number (the corresponding
@@ -58,7 +57,7 @@ class TableCache {
   Env* const env_;
   const std::string dbname_;
   const Options* options_;
-  const StorageOptions& storage_options_;
+  const EnvOptions& storage_options_;
   std::shared_ptr<Cache> cache_;
 
   Status FindTable(const EnvOptions& toptions,

diff --git a/db/transaction_log_iterator_impl.cc b/db/transaction_log_iterator_impl.cc
@@ -7,7 +7,7 @@ namespace leveldb {
 TransactionLogIteratorImpl::TransactionLogIteratorImpl(
                            const std::string& dbname,
                            const Options* options,
-                           const StorageOptions& soptions,
+                           const EnvOptions& soptions,
                            SequenceNumber& seq,
                            std::unique_ptr<std::vector<LogFile>> files,
                            SequenceNumber const * const lastFlushedSequence) :

diff --git a/db/transaction_log_iterator_impl.h b/db/transaction_log_iterator_impl.h
@@ -10,7 +10,6 @@
 #include "leveldb/transaction_log_iterator.h"
 #include "db/log_file.h"
 #include "db/log_reader.h"
-#include "util/storage_options.h"
 
 namespace leveldb {
 
@@ -26,7 +25,7 @@ class TransactionLogIteratorImpl : public TransactionLogIterator {
  public:
   TransactionLogIteratorImpl(const std::string& dbname,
                              const Options* options,
-                             const StorageOptions& soptions,
+                             const EnvOptions& soptions,
                              SequenceNumber& seqNum,
                              std::unique_ptr<std::vector<LogFile>> files,
                              SequenceNumber const * const lastFlushedSequence);
@@ -42,7 +41,7 @@ class TransactionLogIteratorImpl : public TransactionLogIterator {
  private:
   const std::string& dbname_;
   const Options* options_;
-  const StorageOptions& soptions_;
+  const EnvOptions& soptions_;
   const uint64_t startingSequenceNumber_;
   std::unique_ptr<std::vector<LogFile>> files_;
   bool started_;

diff --git a/db/version_set.cc b/db/version_set.cc
@@ -198,15 +198,15 @@ static Iterator* GetFileIterator(void* arg,
 }
 
 Iterator* Version::NewConcatenatingIterator(const ReadOptions& options,
-                                            const StorageOptions& soptions,
+                                            const EnvOptions& soptions,
                                             int level) const {
   return NewTwoLevelIterator(
       new LevelFileNumIterator(vset_->icmp_, &files_[level]),
       &GetFileIterator, vset_->table_cache_, options, soptions);
 }
 
 void Version::AddIterators(const ReadOptions& options,
-                           const StorageOptions& soptions,
+                           const EnvOptions& soptions,
                            std::vector<Iterator*>* iters) {
   // Merge all level zero files together since they may overlap
   for (size_t i = 0; i < files_[0].size(); i++) {
@@ -971,7 +971,7 @@ class VersionSet::Builder {
 
 VersionSet::VersionSet(const std::string& dbname,
                        const Options* options,
-                       const StorageOptions& storage_options,
+                       const EnvOptions& storage_options,
                        TableCache* table_cache,
                        const InternalKeyComparator* cmp)
     : env_(options->env),

diff --git a/db/version_set.h b/db/version_set.h
@@ -63,7 +63,7 @@ class Version {
   // Append to *iters a sequence of iterators that will
   // yield the contents of this Version when merged together.
   // REQUIRES: This version has been saved (see VersionSet::SaveTo)
-  void AddIterators(const ReadOptions&, const StorageOptions& soptions,
+  void AddIterators(const ReadOptions&, const EnvOptions& soptions,
                     std::vector<Iterator*>* iters);
 
   // Lookup the value for key.  If found, store it in *val and
@@ -138,7 +138,7 @@ class Version {
 
   class LevelFileNumIterator;
   Iterator* NewConcatenatingIterator(const ReadOptions&,
-                                     const StorageOptions& soptions,
+                                     const EnvOptions& soptions,
                                      int level) const;
 
   VersionSet* vset_;            // VersionSet to which this Version belongs
@@ -207,7 +207,7 @@ class VersionSet {
  public:
   VersionSet(const std::string& dbname,
              const Options* options,
-             const StorageOptions& storage_options,
+             const EnvOptions& storage_options,
              TableCache* table_cache,
              const InternalKeyComparator*);
   ~VersionSet();
@@ -458,11 +458,11 @@ class VersionSet {
   uint64_t last_observed_manifest_size_;
 
   // storage options for all reads and writes except compactions
-  const StorageOptions& storage_options_;
+  const EnvOptions& storage_options_;
 
   // storage options used for compactions. This is a copy of
   // storage_options_ but with readaheads set to readahead_compactions_.
-  const StorageOptions storage_options_compactions_;
+  const EnvOptions storage_options_compactions_;
 
   // No copying allowed
   VersionSet(const VersionSet&);

diff --git a/helpers/memenv/memenv_test.cc b/helpers/memenv/memenv_test.cc
@@ -8,7 +8,6 @@
 #include "leveldb/db.h"
 #include "leveldb/env.h"
 #include "util/testharness.h"
-#include "util/storage_options.h"
 #include <memory>
 #include <string>
 #include <vector>
@@ -18,7 +17,7 @@ namespace leveldb {
 class MemEnvTest {
  public:
   Env* env_;
-  const StorageOptions soptions_;
+  const EnvOptions soptions_;
 
   MemEnvTest()
       : env_(NewMemEnv(Env::Default())) {

diff --git a/include/leveldb/env.h b/include/leveldb/env.h
@@ -23,16 +23,40 @@
 namespace leveldb {
 
 class FileLock;
-class EnvOptions;
 class Logger;
 class RandomAccessFile;
 class SequentialFile;
 class Slice;
 class WritableFile;
+class Options;
 
 using std::unique_ptr;
 using std::shared_ptr;
 
+
+// Options while opening a file to read/write
+struct EnvOptions {
+
+  // construct with default Options
+  EnvOptions();
+
+  // construct from Options
+  EnvOptions(const Options& options);
+
+  // If true, then allow caching of data in environment buffers
+  bool use_os_buffer;
+
+   // If true, then use mmap to read data
+  bool use_mmap_reads;
+
+   // If true, then use mmap to write data
+  bool use_mmap_writes;
+
+  // If true, set the FD_CLOEXEC on open fd.
+  bool set_fd_cloexec;
+
+};
+
 class Env {
  public:
   Env() { }
@@ -375,27 +399,6 @@ class FileLock {
   void operator=(const FileLock&);
 };
 
-// Options while opening a file to read/write
-class EnvOptions {
-  public:
-   virtual ~EnvOptions() {}
-
-   // If true, then allow caching of data in environment buffers
-   virtual bool UseOsBuffer() const = 0;
-
-   // If true, then allow the environment to readahead data
-   virtual bool UseReadahead() const = 0;
-
-   // If true, then use mmap to read data
-   virtual bool UseMmapReads() const = 0;
-
-   // If true, then use mmap to write data
-   virtual bool UseMmapWrites() const = 0;
-
-   // If true, set the FD_CLOEXEC on open fd.
-   virtual bool IsFDCloseOnExec() const = 0;
-};
-
 // Log the specified data to *info_log if info_log is non-nullptr.
 extern void Log(const shared_ptr<Logger>& info_log, const char* format, ...)
 #   if defined(__GNUC__) || defined(__clang__)

diff --git a/include/leveldb/options.h b/include/leveldb/options.h
@@ -409,11 +409,13 @@ struct Options {
 
   // Reading a single block from a file can cause the OS/FS to start
   // readaheads of other blocks from the file. Default: true
+  // Note: Deprecated
   bool allow_readahead;
 
   // The reads triggered by compaction allows data to be readahead
   // by the OS/FS. This overrides the setting of 'allow_readahead'
   // for compaction-reads. Default: true
+  // Note: Deprecated
   bool allow_readahead_compactions;
 
   // Allow the OS to mmap file for reading. Default: false

diff --git a/table/table_test.cc b/table/table_test.cc
@@ -293,7 +293,7 @@ class TableConstructor: public Constructor {
   TableConstructor();
 
   static uint64_t cur_uniq_id_;
-  const StorageOptions soptions;
+  const EnvOptions soptions;
 };
 uint64_t TableConstructor::cur_uniq_id_ = 1;