Replace ScopedArenaIterator with ScopedArenaPtr<InternalIterator> (#1…

…2470) Summary: ScopedArenaIterator is not an iterator. It is a pointer wrapper. And we don't need a custom implemented pointer wrapper when std::unique_ptr can be instantiated with what we want. So this adds ScopedArenaPtr<T> to replace those uses. Pull Request resolved: #12470 Test Plan: CI (including ASAN/UBSAN) Reviewed By: jowlyzhang Differential Revision: D55254362 Pulled By: pdillinger fbshipit-source-id: cc96a0b9840df99aa807f417725e120802c0ae18
facebook · Mar 22, 2024 · b515a5d · b515a5d
1 parent 3b736c4
commit b515a5d
Show file tree

Hide file tree

Showing 27 changed files with 40 additions and 103 deletions.
diff --git a/db/builder.h b/db/builder.h
@@ -23,7 +23,6 @@
 #include "rocksdb/status.h"
 #include "rocksdb/table_properties.h"
 #include "rocksdb/types.h"
-#include "table/scoped_arena_iterator.h"
 
 namespace ROCKSDB_NAMESPACE {
 

diff --git a/db/column_family.cc b/db/column_family.cc
@@ -1204,7 +1204,7 @@ Status ColumnFamilyData::RangesOverlapWithMemtables(
   super_version->imm->AddIterators(read_opts, /*seqno_to_time_mapping=*/nullptr,
                                    &merge_iter_builder,
                                    false /* add_range_tombstone_iter */);
-  ScopedArenaIterator memtable_iter(merge_iter_builder.Finish());
+  ScopedArenaPtr<InternalIterator> memtable_iter(merge_iter_builder.Finish());
 
   auto read_seq = super_version->current->version_set()->LastSequence();
   ReadRangeDelAggregator range_del_agg(&internal_comparator_, read_seq);

diff --git a/db/compaction/compaction_job.h b/db/compaction/compaction_job.h
@@ -41,7 +41,6 @@
 #include "rocksdb/env.h"
 #include "rocksdb/memtablerep.h"
 #include "rocksdb/transaction_log.h"
-#include "table/scoped_arena_iterator.h"
 #include "util/autovector.h"
 #include "util/stop_watch.h"
 #include "util/thread_local.h"

diff --git a/db/compaction/compaction_job_stats_test.cc b/db/compaction/compaction_job_stats_test.cc
@@ -46,7 +46,6 @@
 #include "table/block_based/block_based_table_factory.h"
 #include "table/mock_table.h"
 #include "table/plain/plain_table_factory.h"
-#include "table/scoped_arena_iterator.h"
 #include "test_util/sync_point.h"
 #include "test_util/testharness.h"
 #include "test_util/testutil.h"

diff --git a/db/db_compaction_filter_test.cc b/db/db_compaction_filter_test.cc
@@ -342,7 +342,7 @@ TEST_F(DBTestCompactionFilter, CompactionFilter) {
   {
     InternalKeyComparator icmp(options.comparator);
     ReadOptions read_options;
-    ScopedArenaIterator iter(dbfull()->NewInternalIterator(
+    ScopedArenaPtr<InternalIterator> iter(dbfull()->NewInternalIterator(
         read_options, &arena, kMaxSequenceNumber, handles_[1]));
     iter->SeekToFirst();
     ASSERT_OK(iter->status());
@@ -434,7 +434,7 @@ TEST_F(DBTestCompactionFilter, CompactionFilter) {
   {
     InternalKeyComparator icmp(options.comparator);
     ReadOptions read_options;
-    ScopedArenaIterator iter(dbfull()->NewInternalIterator(
+    ScopedArenaPtr<InternalIterator> iter(dbfull()->NewInternalIterator(
         read_options, &arena, kMaxSequenceNumber, handles_[1]));
     iter->SeekToFirst();
     ASSERT_OK(iter->status());
@@ -717,8 +717,8 @@ TEST_F(DBTestCompactionFilter, CompactionFilterContextManual) {
     Arena arena;
     InternalKeyComparator icmp(options.comparator);
     ReadOptions read_options;
-    ScopedArenaIterator iter(dbfull()->NewInternalIterator(read_options, &arena,
-                                                           kMaxSequenceNumber));
+    ScopedArenaPtr<InternalIterator> iter(dbfull()->NewInternalIterator(
+        read_options, &arena, kMaxSequenceNumber));
     iter->SeekToFirst();
     ASSERT_OK(iter->status());
     while (iter->Valid()) {

diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h
@@ -59,7 +59,6 @@
 #include "rocksdb/utilities/replayer.h"
 #include "rocksdb/write_buffer_manager.h"
 #include "table/merging_iterator.h"
-#include "table/scoped_arena_iterator.h"
 #include "util/autovector.h"
 #include "util/hash.h"
 #include "util/repeatable_thread.h"

diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc
@@ -1641,7 +1641,7 @@ Status DBImpl::WriteLevel0TableForRecovery(int job_id, ColumnFamilyData* cfd,
   Status s;
   TableProperties table_properties;
   {
-    ScopedArenaIterator iter(
+    ScopedArenaPtr<InternalIterator> iter(
         mem->NewIterator(ro, /*seqno_to_time_mapping=*/nullptr, &arena));
     ROCKS_LOG_DEBUG(immutable_db_options_.info_log,
                     "[%s] [WriteLevel0TableForRecovery]"

diff --git a/db/db_range_del_test.cc b/db/db_range_del_test.cc
@@ -2833,8 +2833,8 @@ TEST_F(DBRangeDelTest, LeftSentinelKeyTestWithNewerKey) {
   Arena arena;
   InternalKeyComparator icmp(options.comparator);
   ReadOptions read_options;
-  ScopedArenaIterator iter;
-  iter.set(
+  ScopedArenaPtr<InternalIterator> iter;
+  iter.reset(
       dbfull()->NewInternalIterator(read_options, &arena, kMaxSequenceNumber));
 
   auto k = Key(4);

diff --git a/db/db_test.cc b/db/db_test.cc
@@ -59,7 +59,6 @@
 #include "rocksdb/utilities/optimistic_transaction_db.h"
 #include "rocksdb/utilities/write_batch_with_index.h"
 #include "table/mock_table.h"
-#include "table/scoped_arena_iterator.h"
 #include "test_util/sync_point.h"
 #include "test_util/testharness.h"
 #include "test_util/testutil.h"

diff --git a/db/db_test_util.cc b/db/db_test_util.cc
@@ -992,13 +992,13 @@ std::string DBTestBase::AllEntriesFor(const Slice& user_key, int cf) {
   auto options = CurrentOptions();
   InternalKeyComparator icmp(options.comparator);
   ReadOptions read_options;
-  ScopedArenaIterator iter;
+  ScopedArenaPtr<InternalIterator> iter;
   if (cf == 0) {
-    iter.set(dbfull()->NewInternalIterator(read_options, &arena,
-                                           kMaxSequenceNumber));
+    iter.reset(dbfull()->NewInternalIterator(read_options, &arena,
+                                             kMaxSequenceNumber));
   } else {
-    iter.set(dbfull()->NewInternalIterator(read_options, &arena,
-                                           kMaxSequenceNumber, handles_[cf]));
+    iter.reset(dbfull()->NewInternalIterator(read_options, &arena,
+                                             kMaxSequenceNumber, handles_[cf]));
   }
   InternalKey target(user_key, kMaxSequenceNumber, kTypeValue);
   iter->Seek(target.Encode());
@@ -1471,13 +1471,13 @@ void DBTestBase::validateNumberOfEntries(int numValues, int cf) {
   auto options = CurrentOptions();
   InternalKeyComparator icmp(options.comparator);
   ReadOptions read_options;
-  ScopedArenaIterator iter;
+  ScopedArenaPtr<InternalIterator> iter;
   if (cf != 0) {
-    iter.set(dbfull()->NewInternalIterator(read_options, &arena,
-                                           kMaxSequenceNumber, handles_[cf]));
+    iter.reset(dbfull()->NewInternalIterator(read_options, &arena,
+                                             kMaxSequenceNumber, handles_[cf]));
   } else {
-    iter.set(dbfull()->NewInternalIterator(read_options, &arena,
-                                           kMaxSequenceNumber));
+    iter.reset(dbfull()->NewInternalIterator(read_options, &arena,
+                                             kMaxSequenceNumber));
   }
   iter->SeekToFirst();
   ASSERT_OK(iter->status());

diff --git a/db/db_test_util.h b/db/db_test_util.h
@@ -41,7 +41,6 @@
 #include "rocksdb/table.h"
 #include "rocksdb/utilities/checkpoint.h"
 #include "table/mock_table.h"
-#include "table/scoped_arena_iterator.h"
 #include "test_util/sync_point.h"
 #include "test_util/testharness.h"
 #include "util/cast_util.h"

diff --git a/db/external_sst_file_ingestion_job.cc b/db/external_sst_file_ingestion_job.cc
@@ -17,7 +17,6 @@
 #include "file/random_access_file_reader.h"
 #include "logging/logging.h"
 #include "table/merging_iterator.h"
-#include "table/scoped_arena_iterator.h"
 #include "table/sst_file_writer_collectors.h"
 #include "table/table_builder.h"
 #include "table/unique_id_impl.h"

diff --git a/db/flush_job.cc b/db/flush_job.cc
@@ -440,7 +440,7 @@ Status FlushJob::MemPurge() {
                          : earliest_seqno;
   }
 
-  ScopedArenaIterator iter(
+  ScopedArenaPtr<InternalIterator> iter(
       NewMergingIterator(&(cfd_->internal_comparator()), memtables.data(),
                          static_cast<int>(memtables.size()), &arena));
 
@@ -917,7 +917,7 @@ Status FlushJob::WriteLevel0Table() {
                          << GetFlushReasonString(flush_reason_);
 
     {
-      ScopedArenaIterator iter(
+      ScopedArenaPtr<InternalIterator> iter(
           NewMergingIterator(&cfd_->internal_comparator(), memtables.data(),
                              static_cast<int>(memtables.size()), &arena));
       ROCKS_LOG_INFO(db_options_.info_log,

diff --git a/db/flush_job.h b/db/flush_job.h
@@ -39,7 +39,6 @@
 #include "rocksdb/listener.h"
 #include "rocksdb/memtablerep.h"
 #include "rocksdb/transaction_log.h"
-#include "table/scoped_arena_iterator.h"
 #include "util/autovector.h"
 #include "util/stop_watch.h"
 #include "util/thread_local.h"

diff --git a/db/import_column_family_job.cc b/db/import_column_family_job.cc
@@ -18,7 +18,6 @@
 #include "file/random_access_file_reader.h"
 #include "logging/logging.h"
 #include "table/merging_iterator.h"
-#include "table/scoped_arena_iterator.h"
 #include "table/sst_file_writer_collectors.h"
 #include "table/table_builder.h"
 #include "table/unique_id_impl.h"

diff --git a/db/range_del_aggregator.cc b/db/range_del_aggregator.cc
@@ -13,7 +13,6 @@
 #include "rocksdb/comparator.h"
 #include "rocksdb/types.h"
 #include "table/internal_iterator.h"
-#include "table/scoped_arena_iterator.h"
 #include "table/table_builder.h"
 #include "util/heap.h"
 #include "util/kv_map.h"

diff --git a/db/range_del_aggregator.h b/db/range_del_aggregator.h
@@ -22,7 +22,6 @@
 #include "rocksdb/comparator.h"
 #include "rocksdb/types.h"
 #include "table/internal_iterator.h"
-#include "table/scoped_arena_iterator.h"
 #include "table/table_builder.h"
 #include "util/heap.h"
 #include "util/kv_map.h"

diff --git a/db/repair.cc b/db/repair.cc
@@ -81,7 +81,6 @@
 #include "rocksdb/env.h"
 #include "rocksdb/options.h"
 #include "rocksdb/write_buffer_manager.h"
-#include "table/scoped_arena_iterator.h"
 #include "table/unique_id_impl.h"
 #include "util/string_util.h"
 
@@ -443,7 +442,7 @@ class Repairer {
       ReadOptions ro;
       ro.total_order_seek = true;
       Arena arena;
-      ScopedArenaIterator iter(
+      ScopedArenaPtr<InternalIterator> iter(
           mem->NewIterator(ro, /*seqno_to_time_mapping=*/nullptr, &arena));
       int64_t _current_time = 0;
       immutable_db_options_.clock->GetCurrentTime(&_current_time)

diff --git a/db/version_set.cc b/db/version_set.cc
@@ -2104,7 +2104,7 @@ Status Version::OverlapWithLevelIterator(const ReadOptions& read_options,
           BeforeFile(ucmp, &largest_user_key, file)) {
         continue;
       }
-      ScopedArenaIterator iter(cfd_->table_cache()->NewIterator(
+      ScopedArenaPtr<InternalIterator> iter(cfd_->table_cache()->NewIterator(
           read_options, file_options, cfd_->internal_comparator(),
           *file->file_metadata, &range_del_agg,
           mutable_cf_options_.prefix_extractor, nullptr,
@@ -2123,7 +2123,7 @@ Status Version::OverlapWithLevelIterator(const ReadOptions& read_options,
     }
   } else if (storage_info_.LevelFilesBrief(level).num_files > 0) {
     auto mem = arena.AllocateAligned(sizeof(LevelIterator));
-    ScopedArenaIterator iter(new (mem) LevelIterator(
+    ScopedArenaPtr<InternalIterator> iter(new (mem) LevelIterator(
         cfd_->table_cache(), read_options, file_options,
         cfd_->internal_comparator(), &storage_info_.LevelFilesBrief(level),
         mutable_cf_options_.prefix_extractor, should_sample_file_read(),

diff --git a/db/write_batch_test.cc b/db/write_batch_test.cc
@@ -21,7 +21,6 @@
 #include "rocksdb/memtablerep.h"
 #include "rocksdb/utilities/write_batch_with_index.h"
 #include "rocksdb/write_buffer_manager.h"
-#include "table/scoped_arena_iterator.h"
 #include "test_util/testharness.h"
 #include "test_util/testutil.h"
 #include "util/string_util.h"
@@ -55,13 +54,13 @@ static std::string PrintContents(WriteBatch* b,
   int merge_count = 0;
   for (int i = 0; i < 2; ++i) {
     Arena arena;
-    ScopedArenaIterator arena_iter_guard;
+    ScopedArenaPtr<InternalIterator> arena_iter_guard;
     std::unique_ptr<InternalIterator> iter_guard;
     InternalIterator* iter;
     if (i == 0) {
       iter = mem->NewIterator(ReadOptions(), /*seqno_to_time_mapping=*/nullptr,
                               &arena);
-      arena_iter_guard.set(iter);
+      arena_iter_guard.reset(iter);
     } else {
       iter = mem->NewRangeTombstoneIterator(ReadOptions(),
                                             kMaxSequenceNumber /* read_seq */,

diff --git a/java/rocksjni/write_batch.cc b/java/rocksjni/write_batch.cc
@@ -22,7 +22,6 @@
 #include "rocksjni/cplusplus_to_java_convert.h"
 #include "rocksjni/portal.h"
 #include "rocksjni/writebatchhandlerjnicallback.h"
-#include "table/scoped_arena_iterator.h"
 
 /*
  * Class:     org_rocksdb_WriteBatch

diff --git a/java/rocksjni/write_batch_test.cc b/java/rocksjni/write_batch_test.cc
@@ -22,7 +22,6 @@
 #include "rocksdb/status.h"
 #include "rocksdb/write_buffer_manager.h"
 #include "rocksjni/portal.h"
-#include "table/scoped_arena_iterator.h"
 #include "test_util/testharness.h"
 #include "util/string_util.h"
 
@@ -59,7 +58,7 @@ jbyteArray Java_org_rocksdb_WriteBatchTest_getContents(JNIEnv* env,
                                                         nullptr, nullptr);
   unsigned int count = 0;
   ROCKSDB_NAMESPACE::Arena arena;
-  ROCKSDB_NAMESPACE::ScopedArenaIterator iter(
+  ROCKSDB_NAMESPACE::ScopedArenaPtr<ROCKSDB_NAMESPACE::InternalIterator> iter(
       mem->NewIterator(ROCKSDB_NAMESPACE::ReadOptions(),
                        /*seqno_to_time_mapping=*/nullptr, &arena));
   for (iter->SeekToFirst(); iter->Valid(); iter->Next()) {

diff --git a/memory/arena.h b/memory/arena.h
@@ -132,4 +132,15 @@ inline char* Arena::Allocate(size_t bytes) {
   return AllocateFallback(bytes, false /* unaligned */);
 }
 
+// Like std::destroy_at but a callable type
+template <typename T>
+struct Destroyer {
+  void operator()(T* ptr) { ptr->~T(); }
+};
+
+// Like std::unique_ptr but only placement-deletes the object (for
+// objects allocated on an arena).
+template <typename T>
+using ScopedArenaPtr = std::unique_ptr<T, Destroyer<T>>;
+
 }  // namespace ROCKSDB_NAMESPACE
diff --git a/table/scoped_arena_iterator.h b/table/scoped_arena_iterator.h
diff --git a/table/table_test.cc b/table/table_test.cc
@@ -63,7 +63,6 @@
 #include "table/internal_iterator.h"
 #include "table/meta_blocks.h"
 #include "table/plain/plain_table_factory.h"
-#include "table/scoped_arena_iterator.h"
 #include "table/sst_file_writer_collectors.h"
 #include "table/unique_id_impl.h"
 #include "test_util/sync_point.h"
@@ -4895,13 +4894,13 @@ TEST_F(MemTableTest, Simple) {
 
   for (int i = 0; i < 2; ++i) {
     Arena arena;
-    ScopedArenaIterator arena_iter_guard;
+    ScopedArenaPtr<InternalIterator> arena_iter_guard;
     std::unique_ptr<InternalIterator> iter_guard;
     InternalIterator* iter;
     if (i == 0) {
       iter = GetMemTable()->NewIterator(
           ReadOptions(), /*seqno_to_time_mapping=*/nullptr, &arena);
-      arena_iter_guard.set(iter);
+      arena_iter_guard.reset(iter);
     } else {
       iter = GetMemTable()->NewRangeTombstoneIterator(
           ReadOptions(), kMaxSequenceNumber /* read_seq */,

diff --git a/tools/ldb_cmd.cc b/tools/ldb_cmd.cc
@@ -38,7 +38,6 @@
 #include "rocksdb/utilities/options_util.h"
 #include "rocksdb/write_batch.h"
 #include "rocksdb/write_buffer_manager.h"
-#include "table/scoped_arena_iterator.h"
 #include "table/sst_file_dumper.h"
 #include "tools/ldb_cmd_impl.h"
 #include "util/cast_util.h"