From 184fbeaaea04370e9afb2d3ffbbc4ba14bda101a Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 3 Apr 2022 00:57:08 +0200 Subject: [PATCH 1/4] Reduce heap allocations and use modern cpp --- binding.cc | 232 ++++++++++++++++++++++------------------------------- 1 file changed, 96 insertions(+), 136 deletions(-) diff --git a/binding.cc b/binding.cc index c87debf..882f34f 100644 --- a/binding.cc +++ b/binding.cc @@ -271,12 +271,12 @@ static std::string* RangeOption (napi_env env, napi_value opts, const char* name /** * Converts an array containing Buffer or string keys to a vector. */ -static std::vector* KeyArray (napi_env env, napi_value arr) { +static std::vector KeyArray (napi_env env, napi_value arr) { uint32_t length; - std::vector* result = new std::vector(); + std::vector result; if (napi_get_array_length(env, arr, &length) == napi_ok) { - result->reserve(length); + result.reserve(length); for (uint32_t i = 0; i < length; i++) { napi_value element; @@ -284,7 +284,7 @@ static std::vector* KeyArray (napi_env env, napi_value arr) { if (napi_get_element(env, arr, i, &element) == napi_ok && StringOrBufferLength(env, element) >= 0) { LD_STRING_OR_BUFFER_TO_COPY(env, element, to); - result->emplace_back(toCh_, toSz_); + result.emplace_back(toCh_, toSz_); delete [] toCh_; } } @@ -318,36 +318,36 @@ enum Mode { * Helper struct for caching and converting a key-value pair to napi_values. */ struct Entry { - Entry (const leveldb::Slice* key, const leveldb::Slice* value) - : key_(key->data(), key->size()), - value_(value->data(), value->size()) {} + Entry (const leveldb::Slice& key, const leveldb::Slice& value) + : key_(key.data(), key.size()), + value_(value.data(), value.size()) {} - void ConvertByMode (napi_env env, Mode mode, const bool keyAsBuffer, const bool valueAsBuffer, napi_value* result) { + void ConvertByMode (napi_env env, Mode mode, const bool keyAsBuffer, const bool valueAsBuffer, napi_value& result) { if (mode == Mode::entries) { - napi_create_array_with_length(env, 2, result); + napi_create_array_with_length(env, 2, &result); napi_value keyElement; napi_value valueElement; - Convert(env, &key_, keyAsBuffer, &keyElement); - Convert(env, &value_, valueAsBuffer, &valueElement); + Convert(env, key_, keyAsBuffer, keyElement); + Convert(env, value_, valueAsBuffer, valueElement); - napi_set_element(env, *result, 0, keyElement); - napi_set_element(env, *result, 1, valueElement); + napi_set_element(env, result, 0, keyElement); + napi_set_element(env, result, 1, valueElement); } else if (mode == Mode::keys) { - Convert(env, &key_, keyAsBuffer, result); + Convert(env, key_, keyAsBuffer, result); } else { - Convert(env, &value_, valueAsBuffer, result); + Convert(env, value_, valueAsBuffer, result); } } - static void Convert (napi_env env, const std::string* s, const bool asBuffer, napi_value* result) { - if (s == NULL) { - napi_get_undefined(env, result); + static void Convert (napi_env env, const std::optional& s, const bool asBuffer, napi_value& result) { + if (!s) { + napi_get_undefined(env, &result); } else if (asBuffer) { - napi_create_buffer_copy(env, s->size(), s->data(), NULL, result); + napi_create_buffer_copy(env, s->size(), s->data(), nullptr, &result); } else { - napi_create_string_utf8(env, s->data(), s->size(), result); + napi_create_string_utf8(env, s->data(), s->size(), &result); } } @@ -371,7 +371,7 @@ struct BaseWorker { Database* database, napi_value callback, const char* resourceName) - : database_(database), errMsg_(NULL) { + : database_(database) { NAPI_STATUS_THROWS_VOID(napi_create_reference(env, callback, 1, &callbackRef_)); napi_value asyncResourceName; NAPI_STATUS_THROWS_VOID(napi_create_string_utf8(env, resourceName, @@ -385,11 +385,10 @@ struct BaseWorker { } virtual ~BaseWorker () { - delete [] errMsg_; } static void Execute (napi_env env, void* data) { - BaseWorker* self = (BaseWorker*)data; + auto self = reinterpret_cast(data); // Don't pass env to DoExecute() because use of Node-API // methods should generally be avoided in async work. @@ -406,16 +405,13 @@ struct BaseWorker { } void SetErrorMessage(const char *msg) { - delete [] errMsg_; - size_t size = strlen(msg) + 1; - errMsg_ = new char[size]; - memcpy(errMsg_, msg, size); + errMsg_ = std::string(msg); } virtual void DoExecute () = 0; static void Complete (napi_env env, napi_status status, void* data) { - BaseWorker* self = (BaseWorker*)data; + auto self = reinterpret_cast(data); self->DoComplete(env); self->DoFinally(env); @@ -441,20 +437,22 @@ struct BaseWorker { virtual void HandleErrorCallback (napi_env env, napi_value callback) { napi_value argv; + auto errMsg = errMsg_.c_str(); + if (status_.IsNotFound()) { - argv = CreateCodeError(env, "LEVEL_NOT_FOUND", errMsg_); + argv = CreateCodeError(env, "LEVEL_NOT_FOUND", errMsg); } else if (status_.IsCorruption()) { - argv = CreateCodeError(env, "LEVEL_CORRUPTION", errMsg_); + argv = CreateCodeError(env, "LEVEL_CORRUPTION", errMsg); } else if (status_.IsIOError()) { - if (strlen(errMsg_) > 15 && strncmp("IO error: lock ", errMsg_, 15) == 0) { // env_posix.cc - argv = CreateCodeError(env, "LEVEL_LOCKED", errMsg_); - } else if (strlen(errMsg_) > 19 && strncmp("IO error: LockFile ", errMsg_, 19) == 0) { // env_win.cc - argv = CreateCodeError(env, "LEVEL_LOCKED", errMsg_); + if (strlen(errMsg) > 15 && strncmp("IO error: lock ", errMsg, 15) == 0) { // env_posix.cc + argv = CreateCodeError(env, "LEVEL_LOCKED", errMsg); + } else if (strlen(errMsg) > 19 && strncmp("IO error: LockFile ", errMsg, 19) == 0) { // env_win.cc + argv = CreateCodeError(env, "LEVEL_LOCKED", errMsg); } else { - argv = CreateCodeError(env, "LEVEL_IO_ERROR", errMsg_); + argv = CreateCodeError(env, "LEVEL_IO_ERROR", errMsg); } } else { - argv = CreateError(env, errMsg_); + argv = CreateError(env, errMsg); } CallFunction(env, callback, 1, &argv); @@ -477,41 +475,24 @@ struct BaseWorker { napi_ref callbackRef_; napi_async_work asyncWork_; leveldb::Status status_; - char *errMsg_; + std::string errMsg_; }; /** * Owns the LevelDB storage, cache, filter policy and iterators. */ struct Database { - Database () - : db_(NULL), - blockCache_(NULL), - filterPolicy_(leveldb::NewBloomFilterPolicy(10)), - currentIteratorId_(0), - pendingCloseWorker_(NULL), - ref_(NULL), - priorityWork_(0) {} - - ~Database () { - if (db_ != NULL) { - delete db_; - db_ = NULL; - } - } - leveldb::Status Open (const leveldb::Options& options, const char* location) { - return leveldb::DB::Open(options, location, &db_); + leveldb::DB* db = nullptr; + auto ret = leveldb::DB::Open(options, location, &db); + db_.reset(db); + return ret; } void CloseDatabase () { - delete db_; - db_ = NULL; - if (blockCache_) { - delete blockCache_; - blockCache_ = NULL; - } + db_.reset(); + blockCache_.reset(); } leveldb::Status Put (const leveldb::WriteOptions& options, @@ -580,9 +561,9 @@ struct Database { void DecrementPriorityWork (napi_env env) { napi_reference_unref(env, ref_, &priorityWork_); - if (priorityWork_ == 0 && pendingCloseWorker_ != NULL) { + if (priorityWork_ == 0 && pendingCloseWorker_ != nullptr) { pendingCloseWorker_->Queue(env); - pendingCloseWorker_ = NULL; + pendingCloseWorker_ = nullptr; } } @@ -590,16 +571,16 @@ struct Database { return priorityWork_ > 0; } - leveldb::DB* db_; - leveldb::Cache* blockCache_; - const leveldb::FilterPolicy* filterPolicy_; - uint32_t currentIteratorId_; - BaseWorker *pendingCloseWorker_; + std::unique_ptr db_; + std::unique_ptr blockCache_; + const leveldb::FilterPolicy* filterPolicy_ = leveldb::NewBloomFilterPolicy(10); + uint32_t currentIteratorId_ = 0; + BaseWorker *pendingCloseWorker_ = nullptr; std::map< uint32_t, Iterator * > iterators_; - napi_ref ref_; + napi_ref ref_ = nullptr; private: - uint32_t priorityWork_; + uint32_t priorityWork_ = 0; }; /** @@ -641,10 +622,9 @@ struct BaseIterator { gte_(gte), limit_(limit), count_(0) { - options_ = new leveldb::ReadOptions(); - options_->fill_cache = fillCache; - options_->snapshot = database->NewSnapshot(); - dbIterator_ = database_->NewIterator(options_); + options_.fill_cache = fillCache; + options_.snapshot = database->NewSnapshot(); + dbIterator_.reset(database_->NewIterator(&options_)); } virtual ~BaseIterator () { @@ -654,8 +634,6 @@ struct BaseIterator { if (gt_ != NULL) delete gt_; if (lte_ != NULL) delete lte_; if (gte_ != NULL) delete gte_; - - delete options_; } bool DidSeek () const { @@ -730,9 +708,8 @@ struct BaseIterator { void Close () { if (!hasClosed_) { hasClosed_ = true; - delete dbIterator_; - dbIterator_ = NULL; - database_->ReleaseSnapshot(options_->snapshot); + dbIterator_.reset(); + database_->ReleaseSnapshot(options_.snapshot); } } @@ -803,7 +780,7 @@ struct BaseIterator { bool hasClosed_; private: - leveldb::Iterator* dbIterator_; + std::unique_ptr dbIterator_; bool didSeek_; const bool reverse_; std::string* lt_; @@ -812,7 +789,7 @@ struct BaseIterator { std::string* gte_; const int limit_; int count_; - leveldb::ReadOptions* options_; + leveldb::ReadOptions options_; }; /** @@ -872,17 +849,17 @@ struct Iterator final : public BaseIterator { if (!Valid() || !Increment()) break; if (keys_ && values_) { - leveldb::Slice k = CurrentKey(); - leveldb::Slice v = CurrentValue(); - cache_.emplace_back(&k, &v); + const auto k = CurrentKey(); + const auto v = CurrentValue(); + cache_.emplace_back(k, v); bytesRead += k.size() + v.size(); } else if (keys_) { - leveldb::Slice k = CurrentKey(); - cache_.emplace_back(&k, &empty); + const auto k = CurrentKey(); + cache_.emplace_back(k, empty); bytesRead += k.size(); } else if (values_) { - leveldb::Slice v = CurrentValue(); - cache_.emplace_back(&empty, &v); + const auto v = CurrentValue(); + cache_.emplace_back(empty, v); bytesRead += v.size(); } @@ -987,7 +964,7 @@ struct OpenWorker final : public BaseWorker { const uint32_t maxFileSize) : BaseWorker(env, database, callback, "classic_level.db.open"), location_(location) { - options_.block_cache = database->blockCache_; + options_.block_cache = database->blockCache_.get(); options_.filter_policy = database->filterPolicy_; options_.create_if_missing = createIfMissing; options_.error_if_exists = errorIfExists; @@ -1032,7 +1009,7 @@ NAPI_METHOD(db_open) { "blockRestartInterval", 16); const uint32_t maxFileSize = Uint32Property(env, options, "maxFileSize", 2 << 20); - database->blockCache_ = leveldb::NewLRUCache(cacheSize); + database->blockCache_.reset(leveldb::NewLRUCache(cacheSize)); napi_value callback = argv[3]; OpenWorker* worker = new OpenWorker(env, database, callback, location, @@ -1170,7 +1147,7 @@ struct GetWorker final : public PriorityWorker { void HandleOKCallback (napi_env env, napi_value callback) override { napi_value argv[2]; napi_get_null(env, &argv[0]); - Entry::Convert(env, &value_, asBuffer_, &argv[1]); + Entry::Convert(env, value_, asBuffer_, argv[1]); CallFunction(env, callback, 2, argv); } @@ -1207,37 +1184,28 @@ NAPI_METHOD(db_get) { struct GetManyWorker final : public PriorityWorker { GetManyWorker (napi_env env, Database* database, - const std::vector* keys, + std::vector keys, napi_value callback, const bool valueAsBuffer, const bool fillCache) : PriorityWorker(env, database, callback, "classic_level.get.many"), - keys_(keys), valueAsBuffer_(valueAsBuffer) { + keys_(std::move(keys)), valueAsBuffer_(valueAsBuffer) { options_.fill_cache = fillCache; options_.snapshot = database->NewSnapshot(); } - ~GetManyWorker() { - delete keys_; - } - void DoExecute () override { - cache_.reserve(keys_->size()); + cache_.reserve(keys_.size()); - for (const std::string& key: *keys_) { - std::string* value = new std::string(); - leveldb::Status status = database_->Get(options_, key, *value); + for (const auto& key: keys_) { + std::string value; + leveldb::Status status = database_->Get(options_, key, value); if (status.ok()) { - cache_.push_back(value); + cache_.push_back(std::move(value)); } else if (status.IsNotFound()) { - delete value; - cache_.push_back(NULL); + cache_.push_back(nullptr); } else { - delete value; - for (const std::string* value: cache_) { - if (value != NULL) delete value; - } SetStatus(status); break; } @@ -1252,11 +1220,9 @@ struct GetManyWorker final : public PriorityWorker { napi_create_array_with_length(env, size, &array); for (size_t idx = 0; idx < size; idx++) { - std::string* value = cache_[idx]; napi_value element; - Entry::Convert(env, value, valueAsBuffer_, &element); + Entry::Convert(env, cache_[idx], valueAsBuffer_, element); napi_set_element(env, array, static_cast(idx), element); - if (value != NULL) delete value; } napi_value argv[2]; @@ -1267,9 +1233,9 @@ struct GetManyWorker final : public PriorityWorker { private: leveldb::ReadOptions options_; - const std::vector* keys_; + const std::vector keys_; const bool valueAsBuffer_; - std::vector cache_; + std::vector> cache_; }; /** @@ -1279,14 +1245,14 @@ NAPI_METHOD(db_get_many) { NAPI_ARGV(4); NAPI_DB_CONTEXT(); - const std::vector* keys = KeyArray(env, argv[1]); + const auto keys = KeyArray(env, argv[1]); napi_value options = argv[2]; const bool asBuffer = EncodingIsBuffer(env, options, "valueEncoding"); const bool fillCache = BooleanProperty(env, options, "fillCache", true); napi_value callback = argv[3]; GetManyWorker* worker = new GetManyWorker( - env, database, keys, callback, asBuffer, fillCache + env, database, std::move(keys), callback, asBuffer, fillCache ); worker->Queue(env); @@ -1349,19 +1315,13 @@ struct ClearWorker final : public PriorityWorker { std::string* lte, std::string* gt, std::string* gte) - : PriorityWorker(env, database, callback, "classic_level.db.clear") { - iterator_ = new BaseIterator(database, reverse, lt, lte, gt, gte, limit, false); - writeOptions_ = new leveldb::WriteOptions(); - writeOptions_->sync = false; - } - - ~ClearWorker () { - delete iterator_; - delete writeOptions_; + : PriorityWorker(env, database, callback, "classic_level.db.clear") + , iterator_(database, reverse, lt, lte, gt, gte, limit, false) { + writeOptions_.sync = false; } void DoExecute () override { - iterator_->SeekToRange(); + iterator_.SeekToRange(); // TODO: add option uint32_t hwm = 16 * 1024; @@ -1370,30 +1330,30 @@ struct ClearWorker final : public PriorityWorker { while (true) { size_t bytesRead = 0; - while (bytesRead <= hwm && iterator_->Valid() && iterator_->Increment()) { - leveldb::Slice key = iterator_->CurrentKey(); + while (bytesRead <= hwm && iterator_.Valid() && iterator_.Increment()) { + const auto key = iterator_.CurrentKey(); batch.Delete(key); bytesRead += key.size(); - iterator_->Next(); + iterator_.Next(); } - if (!SetStatus(iterator_->Status()) || bytesRead == 0) { + if (!SetStatus(iterator_.Status()) || bytesRead == 0) { break; } - if (!SetStatus(database_->WriteBatch(*writeOptions_, &batch))) { + if (!SetStatus(database_->WriteBatch(writeOptions_, &batch))) { break; } batch.Clear(); } - iterator_->Close(); + iterator_.Close(); } private: - BaseIterator* iterator_; - leveldb::WriteOptions* writeOptions_; + BaseIterator iterator_; + leveldb::WriteOptions writeOptions_; }; /** @@ -1757,16 +1717,16 @@ struct NextWorker final : public BaseWorker { } void HandleOKCallback (napi_env env, napi_value callback) override { - size_t size = iterator_->cache_.size(); + const auto size = iterator_->cache_.size(); napi_value jsArray; napi_create_array_with_length(env, size, &jsArray); - const bool kab = iterator_->keyAsBuffer_; - const bool vab = iterator_->valueAsBuffer_; + const auto kab = iterator_->keyAsBuffer_; + const auto vab = iterator_->valueAsBuffer_; for (uint32_t idx = 0; idx < size; idx++) { napi_value element; - iterator_->cache_[idx].ConvertByMode(env, Mode::entries, kab, vab, &element); + iterator_->cache_[idx].ConvertByMode(env, Mode::entries, kab, vab, element); napi_set_element(env, jsArray, idx, element); } From 0c1180e3a04cbb9dc488a416086233e67ea4b7ac Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 3 Apr 2022 17:47:07 +0200 Subject: [PATCH 2/4] fixup --- binding.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/binding.cc b/binding.cc index 882f34f..a3345e2 100644 --- a/binding.cc +++ b/binding.cc @@ -10,6 +10,7 @@ #include #include +#include #include /** From 64f1c5a86e24b79d46b70e6c10e9c5b6381afc53 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 3 Apr 2022 17:47:53 +0200 Subject: [PATCH 3/4] fixup --- .github/workflows/test.yml | 2 +- .vscode/settings.json | 83 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 .vscode/settings.json diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a348910..edbe06a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -8,7 +8,7 @@ jobs: matrix: # At the time of writing macos-latest is mac 10; we need 11 to build a universal binary. os: [ubuntu-latest, macos-11, windows-latest] - node: [12, 14, 16] + node: [14, 16] arch: [x86, x64] exclude: - { os: ubuntu-latest, arch: x86 } diff --git a/.vscode/settings.json b/.vscode/settings.json new file mode 100644 index 0000000..0f2f52c --- /dev/null +++ b/.vscode/settings.json @@ -0,0 +1,83 @@ +{ + "files.associations": { + "__bit_reference": "cpp", + "__bits": "cpp", + "__config": "cpp", + "__debug": "cpp", + "__errc": "cpp", + "__functional_base": "cpp", + "__hash_table": "cpp", + "__locale": "cpp", + "__mutex_base": "cpp", + "__node_handle": "cpp", + "__nullptr": "cpp", + "__split_buffer": "cpp", + "__string": "cpp", + "__threading_support": "cpp", + "__tree": "cpp", + "__tuple": "cpp", + "algorithm": "cpp", + "array": "cpp", + "atomic": "cpp", + "bit": "cpp", + "bitset": "cpp", + "cctype": "cpp", + "chrono": "cpp", + "clocale": "cpp", + "cmath": "cpp", + "compare": "cpp", + "complex": "cpp", + "concepts": "cpp", + "condition_variable": "cpp", + "cstdarg": "cpp", + "cstddef": "cpp", + "cstdint": "cpp", + "cstdio": "cpp", + "cstdlib": "cpp", + "cstring": "cpp", + "ctime": "cpp", + "cwchar": "cpp", + "cwctype": "cpp", + "deque": "cpp", + "exception": "cpp", + "functional": "cpp", + "initializer_list": "cpp", + "ios": "cpp", + "iosfwd": "cpp", + "iostream": "cpp", + "istream": "cpp", + "iterator": "cpp", + "limits": "cpp", + "locale": "cpp", + "map": "cpp", + "memory": "cpp", + "mutex": "cpp", + "new": "cpp", + "numbers": "cpp", + "numeric": "cpp", + "optional": "cpp", + "ostream": "cpp", + "queue": "cpp", + "random": "cpp", + "ratio": "cpp", + "semaphore": "cpp", + "set": "cpp", + "sstream": "cpp", + "stdexcept": "cpp", + "streambuf": "cpp", + "string": "cpp", + "string_view": "cpp", + "system_error": "cpp", + "thread": "cpp", + "tuple": "cpp", + "type_traits": "cpp", + "typeinfo": "cpp", + "unordered_map": "cpp", + "utility": "cpp", + "vector": "cpp", + "*.tcc": "cpp", + "memory_resource": "cpp", + "stop_token": "cpp", + "__memory": "cpp" + } +} \ No newline at end of file From c0d1628327687ff80b03b78de9c2be8221c6d33d Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Sun, 3 Apr 2022 17:48:02 +0200 Subject: [PATCH 4/4] fixup --- .vscode/settings.json | 83 ------------------------------------------- 1 file changed, 83 deletions(-) delete mode 100644 .vscode/settings.json diff --git a/.vscode/settings.json b/.vscode/settings.json deleted file mode 100644 index 0f2f52c..0000000 --- a/.vscode/settings.json +++ /dev/null @@ -1,83 +0,0 @@ -{ - "files.associations": { - "__bit_reference": "cpp", - "__bits": "cpp", - "__config": "cpp", - "__debug": "cpp", - "__errc": "cpp", - "__functional_base": "cpp", - "__hash_table": "cpp", - "__locale": "cpp", - "__mutex_base": "cpp", - "__node_handle": "cpp", - "__nullptr": "cpp", - "__split_buffer": "cpp", - "__string": "cpp", - "__threading_support": "cpp", - "__tree": "cpp", - "__tuple": "cpp", - "algorithm": "cpp", - "array": "cpp", - "atomic": "cpp", - "bit": "cpp", - "bitset": "cpp", - "cctype": "cpp", - "chrono": "cpp", - "clocale": "cpp", - "cmath": "cpp", - "compare": "cpp", - "complex": "cpp", - "concepts": "cpp", - "condition_variable": "cpp", - "cstdarg": "cpp", - "cstddef": "cpp", - "cstdint": "cpp", - "cstdio": "cpp", - "cstdlib": "cpp", - "cstring": "cpp", - "ctime": "cpp", - "cwchar": "cpp", - "cwctype": "cpp", - "deque": "cpp", - "exception": "cpp", - "functional": "cpp", - "initializer_list": "cpp", - "ios": "cpp", - "iosfwd": "cpp", - "iostream": "cpp", - "istream": "cpp", - "iterator": "cpp", - "limits": "cpp", - "locale": "cpp", - "map": "cpp", - "memory": "cpp", - "mutex": "cpp", - "new": "cpp", - "numbers": "cpp", - "numeric": "cpp", - "optional": "cpp", - "ostream": "cpp", - "queue": "cpp", - "random": "cpp", - "ratio": "cpp", - "semaphore": "cpp", - "set": "cpp", - "sstream": "cpp", - "stdexcept": "cpp", - "streambuf": "cpp", - "string": "cpp", - "string_view": "cpp", - "system_error": "cpp", - "thread": "cpp", - "tuple": "cpp", - "type_traits": "cpp", - "typeinfo": "cpp", - "unordered_map": "cpp", - "utility": "cpp", - "vector": "cpp", - "*.tcc": "cpp", - "memory_resource": "cpp", - "stop_token": "cpp", - "__memory": "cpp" - } -} \ No newline at end of file