From 53e280b56866ac4c90a9f5fcfe02ebdfd4a19832 Mon Sep 17 00:00:00 2001
From: Chris Mumford <cmumford@google.com>
Date: Thu, 13 Jun 2019 14:59:06 -0700
Subject: [PATCH] Simplify unlocking in DeleteObsoleteFiles.

A recent change (4cb80b7ddce6f) to DBImpl::DeleteObsoleteFiles
unlocked DBImpl::mutex_ while deleting files to allow for
greater concurrency. This change improves on the prior in
a few areas:

1. The table is evicted from the table cache before unlocking
   the mutex. This should only improve performance.
2. This implementation is slightly simpler, but at the cost of
   a bit more memory usage.
3. A comment adding more detail as to why the mutex is being
   unlocked and why it is safe to do so.

PiperOrigin-RevId: 253111645
---
 db/db_impl.cc | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/db/db_impl.cc b/db/db_impl.cc
index 7695d0b..4754ba3 100644
--- a/db/db_impl.cc
+++ b/db/db_impl.cc
@@ -229,32 +229,27 @@ void DBImpl::DeleteObsoleteFiles() {
     return;
   }
 
-  const uint64_t log_number = versions_->LogNumber();
-  const uint64_t prev_log_number = versions_->PrevLogNumber();
-  const uint64_t manifest_file_number = versions_->ManifestFileNumber();
-
   // Make a set of all of the live files
   std::set<uint64_t> live = pending_outputs_;
   versions_->AddLiveFiles(&live);
 
   std::vector<std::string> filenames;
   env_->GetChildren(dbname_, &filenames);  // Ignoring errors on purpose
-
-  // Unlock while deleting obsolete files
-  mutex_.Unlock();
   uint64_t number;
   FileType type;
-  for (size_t i = 0; i < filenames.size(); i++) {
-    if (ParseFileName(filenames[i], &number, &type)) {
+  std::vector<std::string> files_to_delete;
+  for (std::string& filename : filenames) {
+    if (ParseFileName(filename, &number, &type)) {
       bool keep = true;
       switch (type) {
         case kLogFile:
-          keep = ((number >= log_number) || (number == prev_log_number));
+          keep = ((number >= versions_->LogNumber()) ||
+                  (number == versions_->PrevLogNumber()));
           break;
         case kDescriptorFile:
           // Keep my manifest file, and any newer incarnations'
           // (in case there is a race that allows other incarnations)
-          keep = (number >= manifest_file_number);
+          keep = (number >= versions_->ManifestFileNumber());
           break;
         case kTableFile:
           keep = (live.find(number) != live.end());
@@ -272,15 +267,23 @@ void DBImpl::DeleteObsoleteFiles() {
       }
 
       if (!keep) {
+        files_to_delete.push_back(std::move(filename));
         if (type == kTableFile) {
           table_cache_->Evict(number);
         }
         Log(options_.info_log, "Delete type=%d #%lld\n", static_cast<int>(type),
             static_cast<unsigned long long>(number));
-        env_->DeleteFile(dbname_ + "/" + filenames[i]);
       }
     }
   }
+
+  // While deleting all files unblock other threads. All files being deleted
+  // have unique names which will not collide with newly created files and
+  // are therefore safe to delete while allowing other threads to proceed.
+  mutex_.Unlock();
+  for (const std::string& filename : files_to_delete) {
+    env_->DeleteFile(dbname_ + "/" + filename);
+  }
   mutex_.Lock();
 }