From b515a5db3f8013d5a8b6c1deaf99b50b90bd5b81 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Fri, 22 Mar 2024 13:40:42 -0700 Subject: [PATCH] Replace ScopedArenaIterator with ScopedArenaPtr (#12470) 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 to replace those uses. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12470 Test Plan: CI (including ASAN/UBSAN) Reviewed By: jowlyzhang Differential Revision: D55254362 Pulled By: pdillinger fbshipit-source-id: cc96a0b9840df99aa807f417725e120802c0ae18 --- db/builder.h | 1 - db/column_family.cc | 2 +- db/compaction/compaction_job.h | 1 - db/compaction/compaction_job_stats_test.cc | 1 - db/db_compaction_filter_test.cc | 8 +-- db/db_impl/db_impl.h | 1 - db/db_impl/db_impl_open.cc | 2 +- db/db_range_del_test.cc | 4 +- db/db_test.cc | 1 - db/db_test_util.cc | 20 ++++---- db/db_test_util.h | 1 - db/external_sst_file_ingestion_job.cc | 1 - db/flush_job.cc | 4 +- db/flush_job.h | 1 - db/import_column_family_job.cc | 1 - db/range_del_aggregator.cc | 1 - db/range_del_aggregator.h | 1 - db/repair.cc | 3 +- db/version_set.cc | 4 +- db/write_batch_test.cc | 5 +- java/rocksjni/write_batch.cc | 1 - java/rocksjni/write_batch_test.cc | 3 +- memory/arena.h | 11 +++++ table/scoped_arena_iterator.h | 57 ---------------------- table/table_test.cc | 5 +- tools/ldb_cmd.cc | 1 - utilities/debug.cc | 2 +- 27 files changed, 40 insertions(+), 103 deletions(-) delete mode 100644 table/scoped_arena_iterator.h diff --git a/db/builder.h b/db/builder.h index 6e6bc63c51..f228f8d0fe 100644 --- 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 index 73e695beca..2b606ec409 100644 --- 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 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 index e812cfc72a..caa1593e72 100644 --- 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 index 1cc6b31148..6909945197 100644 --- 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 index 6cedb6fd53..d84e24c41c 100644 --- 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 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 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 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 index a7181c9e0f..244d979c1a 100644 --- 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 index 786abb74f2..a0e3f7cf15 100644 --- 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 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 index b5d63a2298..f92fa27aed 100644 --- 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 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 index b2c6aab6d6..3d514e39ad 100644 --- 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 index fd751782d1..cbc394b042 100644 --- 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 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 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 index 2a671e1e8c..775c161d36 100644 --- 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 index a671eea5e1..684952c98e 100644 --- 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 index 7995ea786e..1a317f3b23 100644 --- a/db/flush_job.cc +++ b/db/flush_job.cc @@ -440,7 +440,7 @@ Status FlushJob::MemPurge() { : earliest_seqno; } - ScopedArenaIterator iter( + ScopedArenaPtr iter( NewMergingIterator(&(cfd_->internal_comparator()), memtables.data(), static_cast(memtables.size()), &arena)); @@ -917,7 +917,7 @@ Status FlushJob::WriteLevel0Table() { << GetFlushReasonString(flush_reason_); { - ScopedArenaIterator iter( + ScopedArenaPtr iter( NewMergingIterator(&cfd_->internal_comparator(), memtables.data(), static_cast(memtables.size()), &arena)); ROCKS_LOG_INFO(db_options_.info_log, diff --git a/db/flush_job.h b/db/flush_job.h index 9e636e6a85..337e9cd9bc 100644 --- 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 index c40bb0cab9..4d5c65616e 100644 --- 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 index 652afe65a9..f41521e116 100644 --- 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 index f7fa87af40..f367a26787 100644 --- 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 index 34d79c5df5..4fe8b47886 100644 --- 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 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 index 1b36ad34cc..23f6d770f6 100644 --- 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 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 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 index 05aec899a5..8db8c32a0a 100644 --- 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 arena_iter_guard; std::unique_ptr 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 index 8ead84671d..f2f13b6b75 100644 --- 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 index f3163374cb..53f10998ca 100644 --- 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 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 index 39399aa71b..2257a4935d 100644 --- 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 +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 +using ScopedArenaPtr = std::unique_ptr>; + } // namespace ROCKSDB_NAMESPACE diff --git a/table/scoped_arena_iterator.h b/table/scoped_arena_iterator.h deleted file mode 100644 index 2b8824d95e..0000000000 --- a/table/scoped_arena_iterator.h +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright (c) 2011-present, Facebook, Inc. All rights reserved. -// This source code is licensed under both the GPLv2 (found in the -// COPYING file in the root directory) and Apache 2.0 License -// (found in the LICENSE.Apache file in the root directory). -// Copyright (c) 2011 The LevelDB Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. See the AUTHORS file for names of contributors. -#pragma once - -#include "port/port.h" -#include "table/internal_iterator.h" - -namespace ROCKSDB_NAMESPACE { -class ScopedArenaIterator { - void reset(InternalIterator* iter) noexcept { - if (iter_ != nullptr) { - iter_->~InternalIterator(); - } - iter_ = iter; - } - - public: - explicit ScopedArenaIterator(InternalIterator* iter = nullptr) - : iter_(iter) {} - - ScopedArenaIterator(const ScopedArenaIterator&) = delete; - ScopedArenaIterator& operator=(const ScopedArenaIterator&) = delete; - - ScopedArenaIterator(ScopedArenaIterator&& o) noexcept { - iter_ = o.iter_; - o.iter_ = nullptr; - } - - ScopedArenaIterator& operator=(ScopedArenaIterator&& o) noexcept { - reset(o.iter_); - o.iter_ = nullptr; - return *this; - } - - InternalIterator* operator->() { return iter_; } - InternalIterator* get() { return iter_; } - - void set(InternalIterator* iter) { reset(iter); } - - InternalIterator* release() { - assert(iter_ != nullptr); - auto* res = iter_; - iter_ = nullptr; - return res; - } - - ~ScopedArenaIterator() { reset(nullptr); } - - private: - InternalIterator* iter_; -}; -} // namespace ROCKSDB_NAMESPACE diff --git a/table/table_test.cc b/table/table_test.cc index 432799468b..02a8d899d7 100644 --- 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 arena_iter_guard; std::unique_ptr 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 index dcc03347b8..355e26fa7c 100644 --- 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" diff --git a/utilities/debug.cc b/utilities/debug.cc index c7599797da..6274cd6c2d 100644 --- a/utilities/debug.cc +++ b/utilities/debug.cc @@ -82,7 +82,7 @@ Status GetAllKeyVersions(DB* db, ColumnFamilyHandle* cfh, Slice begin_key, auto icmp = InternalKeyComparator(idb->GetOptions(cfh).comparator); ReadOptions read_options; Arena arena; - ScopedArenaIterator iter( + ScopedArenaPtr iter( idb->NewInternalIterator(read_options, &arena, kMaxSequenceNumber, cfh)); if (!begin_key.empty()) {