Skip to content

Commit

Permalink
fix memory leak in two_level_iterator
Browse files Browse the repository at this point in the history
Summary:
this PR fixes a few failed contbuild:
1. ASAN memory leak in Block::NewIterator (table/block.cc:429). the proper destruction of first_level_iter_ and second_level_iter_ of two_level_iterator.cc is missing from the code after the refactoring in #3406
2. various unused param errors introduced by #3662
3. updated comment for `ForceReleaseCachedEntry` to emphasize the use of `force_erase` flag.
Closes #3718

Reviewed By: maysamyabandeh

Differential Revision: D7621192

Pulled By: miasantreble

fbshipit-source-id: 476c94264083a0730ded957c29de7807e4f5b146
  • Loading branch information
miasantreble authored and facebook-github-bot committed Apr 16, 2018
1 parent 9fcd82e commit 954b496
Show file tree
Hide file tree
Showing 53 changed files with 87 additions and 57 deletions.
7 changes: 7 additions & 0 deletions db/column_family_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,9 @@ class ColumnFamilyTest : public testing::Test {
void AssertFilesPerLevel(const std::string& value, int cf) {
#ifndef ROCKSDB_LITE
ASSERT_EQ(value, FilesPerLevel(cf));
#else
(void) value;
(void) cf;
#endif
}

Expand All @@ -405,6 +408,8 @@ class ColumnFamilyTest : public testing::Test {
void AssertCountLiveFiles(int expected_value) {
#ifndef ROCKSDB_LITE
ASSERT_EQ(expected_value, CountLiveFiles());
#else
(void) expected_value;
#endif
}

Expand Down Expand Up @@ -453,6 +458,8 @@ class ColumnFamilyTest : public testing::Test {
void AssertCountLiveLogFiles(int value) {
#ifndef ROCKSDB_LITE // GetSortedWalFiles is not supported
ASSERT_EQ(value, CountLiveLogFiles());
#else
(void) value;
#endif // !ROCKSDB_LITE
}

Expand Down
2 changes: 1 addition & 1 deletion db/compact_files_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr,
"SKIPPED as DBImpl::CompactFiles is not supported in ROCKSDB_LITE\n");
return 0;
Expand Down
4 changes: 2 additions & 2 deletions db/compaction_job_stats_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1034,7 +1034,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED, not supported in ROCKSDB_LITE\n");
return 0;
}
Expand All @@ -1043,5 +1043,5 @@ int main(int argc, char** argv) {

#else

int main(int argc, char** argv) { return 0; }
int main(int /*argc*/, char** /*argv*/) { return 0; }
#endif // !defined(IOS_CROSS_COMPILE)
2 changes: 1 addition & 1 deletion db/compaction_job_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -946,7 +946,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr,
"SKIPPED as CompactionJobStats is not supported in ROCKSDB_LITE\n");
return 0;
Expand Down
2 changes: 1 addition & 1 deletion db/corruption_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as RepairDB() is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion db/cuckoo_table_db_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as Cuckoo table is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 2 additions & 0 deletions db/db_compaction_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3766,6 +3766,8 @@ int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
#else
(void) argc;
(void) argv;
return 0;
#endif
}
2 changes: 2 additions & 0 deletions db/db_dynamic_level_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,8 @@ int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
#else
(void) argc;
(void) argv;
return 0;
#endif
}
2 changes: 2 additions & 0 deletions db/db_log_iter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,8 @@ int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
#else
(void) argc;
(void) argv;
return 0;
#endif
}
2 changes: 2 additions & 0 deletions db/db_tailing_iter_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -809,6 +809,8 @@ int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
#else
(void) argc;
(void) argv;
return 0;
#endif
}
2 changes: 2 additions & 0 deletions db/db_universal_compaction_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1899,6 +1899,8 @@ int main(int argc, char** argv) {
::testing::InitGoogleTest(&argc, argv);
return RUN_ALL_TESTS();
#else
(void) argc;
(void) argv;
return 0;
#endif
}
2 changes: 1 addition & 1 deletion db/deletefile_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr,
"SKIPPED as DBImpl::DeleteFile is not supported in ROCKSDB_LITE\n");
return 0;
Expand Down
2 changes: 1 addition & 1 deletion db/external_sst_file_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2056,7 +2056,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr,
"SKIPPED as External SST File Writer and Ingestion are not supported "
"in ROCKSDB_LITE\n");
Expand Down
2 changes: 1 addition & 1 deletion db/obsolete_files_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr,
"SKIPPED as DBImpl::DeleteFile is not supported in ROCKSDB_LITE\n");
return 0;
Expand Down
2 changes: 1 addition & 1 deletion db/options_file_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ int main(int argc, char** argv) {

#include <cstdio>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
printf("Skipped as Options file is not supported in RocksDBLite.\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion db/plain_table_db_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1170,7 +1170,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as plain table is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion db/prefix_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr,
"SKIPPED as HashSkipList and HashLinkList are not supported in "
"ROCKSDB_LITE\n");
Expand Down
2 changes: 1 addition & 1 deletion db/repair_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as RepairDB is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion db/wal_manager_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as WalManager is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion db/write_callback_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr,
"SKIPPED as WriteWithCallback is not supported in ROCKSDB_LITE\n");
return 0;
Expand Down
6 changes: 4 additions & 2 deletions env/io_posix.cc
Original file line number Diff line number Diff line change
Expand Up @@ -890,10 +890,12 @@ void PosixWritableFile::SetWriteLifeTimeHint(Env::WriteLifeTimeHint hint) {
if (fcntl(fd_, F_SET_RW_HINT, &hint) == 0) {
write_hint_ = hint;
}
#endif
#else
(void)hint;
#endif
#endif // ROCKSDB_VALGRIND_RUN
#else
(void)hint;
#endif // OS_LINUX
}

Status PosixWritableFile::InvalidateCache(size_t offset, size_t length) {
Expand Down
2 changes: 1 addition & 1 deletion table/block_based_table_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ void ReleaseCachedEntry(void* arg, void* h) {
void ForceReleaseCachedEntry(void* arg, void* h) {
Cache* cache = reinterpret_cast<Cache*>(arg);
Cache::Handle* handle = reinterpret_cast<Cache::Handle*>(h);
cache->Release(handle, true);
cache->Release(handle, true /* force_erase */);
}

Slice GetCacheKeyFromOffset(const char* cache_key_prefix,
Expand Down
2 changes: 1 addition & 1 deletion table/cuckoo_table_builder_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -625,7 +625,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as Cuckoo table is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion table/cuckoo_table_reader_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as Cuckoo table is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
6 changes: 5 additions & 1 deletion table/two_level_iterator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,11 @@ class TwoLevelIterator : public InternalIterator {
explicit TwoLevelIterator(TwoLevelIteratorState* state,
InternalIterator* first_level_iter);

virtual ~TwoLevelIterator() { delete state_; }
virtual ~TwoLevelIterator() {
first_level_iter_.DeleteIter(false /* is_arena_mode */);
second_level_iter_.DeleteIter(false /* is_arena_mode */);
delete state_;
}

virtual void Seek(const Slice& target) override;
virtual void SeekForPrev(const Slice& target) override;
Expand Down
8 changes: 2 additions & 6 deletions table/two_level_iterator.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ namespace rocksdb {

struct ReadOptions;
class InternalKeyComparator;
class Arena;

// TwoLevelIteratorState expects iterators are not created using the arena
struct TwoLevelIteratorState {
TwoLevelIteratorState() {}

Expand All @@ -35,11 +35,7 @@ struct TwoLevelIteratorState {
//
// Uses a supplied function to convert an index_iter value into
// an iterator over the contents of the corresponding block.
// arena: If not null, the arena is used to allocate the Iterator.
// When destroying the iterator, the destructor will destroy
// all the states but those allocated in arena.
// need_free_iter_and_state: free `state` and `first_level_iter` if
// true. Otherwise, just call destructor.
// Note: this function expects first_level_iter was not created using the arena
extern InternalIterator* NewTwoLevelIterator(
TwoLevelIteratorState* state, InternalIterator* first_level_iter);

Expand Down
5 changes: 5 additions & 0 deletions tools/db_bench_tool.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2944,6 +2944,8 @@ void VerifyDBFromDB(std::string& truth_db_name) {
FLAGS_options_file.c_str(), s.ToString().c_str());
exit(1);
}
#else
(void)opts;
#endif
return false;
}
Expand Down Expand Up @@ -4000,6 +4002,9 @@ void VerifyDBFromDB(std::string& truth_db_name) {
}
return Status::OK();
#else
(void)thread;
(void)compaction_style;
(void)write_mode;
fprintf(stderr, "Rocksdb Lite doesn't support filldeterministic\n");
return Status::NotSupported(
"Rocksdb Lite doesn't support filldeterministic");
Expand Down
15 changes: 9 additions & 6 deletions tools/db_stress.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1039,20 +1039,16 @@ class DbStressListener : public EventListener {
column_families_(column_families) {}
virtual ~DbStressListener() {}
#ifndef ROCKSDB_LITE
virtual void OnFlushCompleted(DB* db, const FlushJobInfo& info) override {
assert(db);
assert(db->GetName() == db_name_);
virtual void OnFlushCompleted(DB* /*db*/, const FlushJobInfo& info) override {
assert(IsValidColumnFamilyName(info.cf_name));
VerifyFilePath(info.file_path);
// pretending doing some work here
std::this_thread::sleep_for(
std::chrono::microseconds(Random::GetTLSInstance()->Uniform(5000)));
}

virtual void OnCompactionCompleted(DB* db,
virtual void OnCompactionCompleted(DB* /*db*/,
const CompactionJobInfo& ci) override {
assert(db);
assert(db->GetName() == db_name_);
assert(IsValidColumnFamilyName(ci.cf_name));
assert(ci.input_files.size() + ci.output_files.size() > 0U);
for (const auto& file_path : ci.input_files) {
Expand Down Expand Up @@ -1111,6 +1107,8 @@ class DbStressListener : public EventListener {
}
}
assert(false);
#else
(void)file_dir;
#endif // !NDEBUG
}

Expand All @@ -1121,6 +1119,8 @@ class DbStressListener : public EventListener {
bool result = ParseFileName(file_name, &file_number, &file_type);
assert(result);
assert(file_type == kTableFile);
#else
(void)file_name;
#endif // !NDEBUG
}

Expand All @@ -1135,6 +1135,8 @@ class DbStressListener : public EventListener {
}
VerifyFileName(file_path.substr(pos));
}
#else
(void)file_path;
#endif // !NDEBUG
}
#endif // !ROCKSDB_LITE
Expand Down Expand Up @@ -2315,6 +2317,7 @@ class StressTest {
size_t value_sz =
((rand % kRandomValueMaxFactor) + 1) * FLAGS_value_size_mult;
assert(value_sz <= max_sz && value_sz >= sizeof(uint32_t));
(void) max_sz;
*((uint32_t*)v) = rand;
for (size_t i=sizeof(uint32_t); i < value_sz; i++) {
v[i] = (char)(rand ^ i);
Expand Down
2 changes: 1 addition & 1 deletion tools/ldb_cmd_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as LDBCommand is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion tools/reduce_levels_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as LDBCommand is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion tools/sst_dump_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr, "SKIPPED as SSTDumpTool is not supported in ROCKSDB_LITE\n");
return 0;
}
Expand Down
2 changes: 1 addition & 1 deletion util/auto_roll_logger_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ int main(int argc, char** argv) {
#else
#include <stdio.h>

int main(int argc, char** argv) {
int main(int /*argc*/, char** /*argv*/) {
fprintf(stderr,
"SKIPPED as AutoRollLogger is not supported in ROCKSDB_LITE\n");
return 0;
Expand Down
Loading

0 comments on commit 954b496

Please sign in to comment.