Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite to N-API #100

Closed
14 tasks done
vweevers opened this issue Apr 7, 2019 · 10 comments · Fixed by #111
Closed
14 tasks done

Rewrite to N-API #100

vweevers opened this issue Apr 7, 2019 · 10 comments · Fixed by #111
Labels
refactor Requires or pertains to refactoring semver-major Changes that break backward compatibility
Milestone

Comments

@vweevers
Copy link
Member

vweevers commented Apr 7, 2019

  • Copy binding.cc, binding.js, leveldown.js, chained-batch.js and iterator.js from leveldown
  • Restore windows destroy patch
  • Change includes and add namespace alias
  • Add RocksDB specifics (see diff below)
    • rocksdb::Options
    • readOnly option
    • CompactRangeOptions
    • blockCache (shared pointer and no delete)
  • Copy tests from leveldown then restore RocksDB-specifics
  • Copy prebuild setup from leveldown (without cross-compilation)
  • Update minimum node version and related docs
  • Ensure prebuilds/ is included in npm package
  • Run leak-tester-batch.js, leak-tester-iterator.js, leak-tester.js
  • Add GitHub tokens
@vweevers vweevers added semver-major Changes that break backward compatibility refactor Requires or pertains to refactoring labels Apr 7, 2019
@vweevers
Copy link
Member Author

vweevers commented Apr 7, 2019

Diff on src/ against the leveldown#4.x branch (modified by hand in both directions to remove irrelevant differences like code style, missing destructors that have been fixed in the N-API codebase, comments, class names, etc - so this is only for reference):

Click to expand
diff --git a/src/database.cc b/src/database.cc
index 5fc8676..5c11c5f 100644
--- a/src/database.cc
+++ b/src/database.cc
@@ -37,69 +32,75 @@ Database::~Database () {
 
 /* Calls from worker threads, NO V8 HERE *****************************/
 
-rocksdb::Status Database::OpenDatabase (
-        rocksdb::Options* options
+rocksdb::Status Database::OpenDatabase (
+        rocksdb::Options* options,
+        bool readOnly
     ) {
-  return rocksdb::DB::Open(*options, **location, &db);
+  if (readOnly) {
+    return rocksdb::DB::OpenForReadOnly(*options, **location, &db);
+  } else {
+    return rocksdb::DB::Open(*options, **location, &db);
+  }
 }
 
-void Database::CompactRangeFromDatabase (const rocksdb::Slice* start, 
-                                         const rocksdb::Slice* end) {
-  db->CompactRange(start, end);
+void Database::CompactRangeFromDatabase (const rocksdb::Slice* start,
+                                         const rocksdb::Slice* end) {
+  rocksdb::CompactRangeOptions options;
+  db->CompactRange(options, start, end);
 }
@@ -120,7 +121,10 @@ void Database::CloseDatabase () {
   delete db;
   db = NULL;
   if (blockCache) {
-    delete blockCache;
+    // According to
+    // https://github.com/facebook/rocksdb/wiki/basic-operations#cache
+    // it doesn't look like this needs to be deleted by hand anymore.
+    // delete blockCache;
     blockCache = NULL;
   }
   if (filterPolicy) {
@@ -182,6 +186,7 @@ v8::Local<v8::Value> Database::NewInstance (v8::Local<v8::String> &location) {
 NAN_METHOD(Database::Open) {
   LD_METHOD_SETUP_COMMON(open, 0, 1)
 
+  bool readOnly = BooleanOptionValue(optionsObj, "readOnly", false);
   bool createIfMissing = BooleanOptionValue(optionsObj, "createIfMissing", true);
   bool errorIfExists = BooleanOptionValue(optionsObj, "errorIfExists");
   bool compression = BooleanOptionValue(optionsObj, "compression", true);
@@ -201,8 +206,9 @@ NAN_METHOD(Database::Open) {
   );
   uint32_t maxFileSize = UInt32OptionValue(optionsObj, "maxFileSize", 2 << 20);
 
-  database->blockCache = rocksdb::NewLRUCache(cacheSize);
+  database->blockCache = cacheSize ? rocksdb::NewLRUCache(cacheSize) :
+                                     NULL;
   database->filterPolicy = rocksdb::NewBloomFilterPolicy(10);
 
   OpenWorker* worker = new OpenWorker(
       database
@@ -217,6 +223,7 @@ NAN_METHOD(Database::Open) {
     , maxOpenFiles
     , blockRestartInterval
     , maxFileSize
+    , readOnly
   );
   // persist to prevent accidental GC
   v8::Local<v8::Object> _this = info.This();
diff --git a/src/database.h b/src/database.h
index d82ec9f..1beaa03 100644
--- a/src/database.h
+++ b/src/database.h
@@ -48,31 +43,31 @@ public:
   static void Init ();
   static v8::Local<v8::Value> NewInstance (v8::Local<v8::String> &location);
 
-  rocksdb::Status OpenDatabase (rocksdb::Options* options);
+  rocksdb::Status OpenDatabase (rocksdb::Options* options, bool readOnly);
   rocksdb::Status PutToDatabase (
       rocksdb::WriteOptions* options
     , rocksdb::Slice key
@@ -81,11 +76,11 @@ public:
 
 private:
   Nan::Utf8String* location;
   rocksdb::DB* db;
   uint32_t currentIteratorId;
   void(*pendingCloseWorker);
-  rocksdb::Cache* blockCache;
+  std::shared_ptr<rocksdb::Cache> blockCache;
   const rocksdb::FilterPolicy* filterPolicy;
 
   std::map< uint32_t, leveldown::Iterator * > iterators;
 
diff --git a/src/database_async.cc b/src/database_async.cc
index 466e42b..c24c6c1 100644
--- a/src/database_async.cc
+++ b/src/database_async.cc
@@ -1,13 +1,18 @@
 #include <rocksdb/write_batch.h>
 #include <rocksdb/filter_policy.h>
+
+#include <rocksdb/utilities/leveldb_options.h>
+#include <rocksdb/cache.h>
+#include <rocksdb/comparator.h>
+#include <rocksdb/env.h>
+#include <rocksdb/options.h>
+#include <rocksdb/table.h>
+
 
 #include "database.h"
 #include "leveldown.h"
@@ -18,54 +23,88 @@ namespace leveldown {
 
 /** OPEN WORKER **/
 
 OpenWorker::OpenWorker(Database *database,
                        Nan::Callback *callback,
-                       rocksdb::Cache* blockCache,
+                       std::shared_ptr<rocksdb::Cache> blockCache,
                        const rocksdb::FilterPolicy* filterPolicy,
                        bool createIfMissing,
                        bool errorIfExists,
                        bool compression,
                        uint32_t writeBufferSize,
                        uint32_t blockSize,
                        uint32_t maxOpenFiles,
                        uint32_t blockRestartInterval,
-                       uint32_t maxFileSize)
+                       uint32_t maxFileSize,
+                       bool readOnly)
 : AsyncWorker(database, callback, "rocksdb:db.open")
+  , readOnly_(readOnly)
 {
-  options = new leveldb::Options();
-  options->block_cache            = blockCache;
-  options->filter_policy          = filterPolicy;
-  options->create_if_missing      = createIfMissing;
-  options->error_if_exists        = errorIfExists;
-  options->compression            = compression
-      ? rocksdb::kSnappyCompression
-      : rocksdb::kNoCompression;
-  options->write_buffer_size      = writeBufferSize;
-  options->block_size             = blockSize;
-  options->max_open_files         = maxOpenFiles;
-  options->block_restart_interval = blockRestartInterval;
-  options->max_file_size          = maxFileSize;
+  rocksdb::LevelDBOptions levelOptions;
+
+  if (blockCache != NULL) {
+    levelOptions.block_cache = blockCache.get();
+  }
+
+  levelOptions.filter_policy          = filterPolicy;
+  levelOptions.create_if_missing      = createIfMissing;
+  levelOptions.error_if_exists        = errorIfExists;
+  levelOptions.compression            = compression
+      ? rocksdb::kSnappyCompression
+      : rocksdb::kNoCompression;
+  levelOptions.write_buffer_size      = writeBufferSize;
+  levelOptions.block_size             = blockSize;
+  levelOptions.max_open_files         = maxOpenFiles;
+  levelOptions.block_restart_interval = blockRestartInterval;
+
+  options = new rocksdb::Options(rocksdb::ConvertOptions(levelOptions));
+  options->max_log_file_size          = maxFileSize;
 };
 
 OpenWorker::~OpenWorker () {
-  delete options;
+  // delete options;
 }
 
 void OpenWorker::Execute () {
-  SetStatus(database->OpenDatabase(options));
+  SetStatus(database->OpenDatabase(options, readOnly_));
 }
 
diff --git a/src/database_async.h b/src/database_async.h
index 87f21fa..61e6c43 100644
--- a/src/database_async.h
+++ b/src/database_async.h
@@ -17,154 +12,175 @@ namespace leveldown {
 
 class OpenWorker : public AsyncWorker {
 public:
   OpenWorker(Database *database,
              Nan::Callback *callback,
-             rocksdb::Cache* blockCache,
+             std::shared_ptr<rocksdb::Cache> blockCache,
              const rocksdb::FilterPolicy* filterPolicy,
              bool createIfMissing,
              bool errorIfExists,
              bool compression,
              uint32_t writeBufferSize,
              uint32_t blockSize,
              uint32_t maxOpenFiles,
              uint32_t blockRestartInterval,
-             uint32_t maxFileSize
+             uint32_t maxFileSize,
+             bool readOnly
   );
 
   virtual ~OpenWorker ();
   virtual void Execute ();
 
 private:
   rocksdb::Options* options;
+  bool readOnly_;
 };

@vweevers
Copy link
Member Author

💡 We don't have to use a shared_ptr for the block cache; we can instead pass the cacheSize option down to OpenWorker which then creates the cache object with rocksdb::NewLRUCache(cacheSize) and makes RocksDB the sole owner of that object.

@vweevers vweevers added this to the 4.0.0 milestone May 19, 2019
@vweevers
Copy link
Member Author

Damn, the N-API Windows prebuild alone is 3.8 MB (compare to 490 kB in leveldown). Maybe we shouldn't use prebuildify here.

@vweevers
Copy link
Member Author

vweevers commented May 19, 2019

Damn, the N-API Windows prebuild alone is 3.8 MB (compare to 490 kB in leveldown).

With ROCKSDB_LITE it's 2.5MB but I can't get that build to work yet (some problem with column families, which we don't use) and I'm not eager to go down that rabbit hole.

Maybe we shouldn't use prebuildify here.

I like prebuildify too much though. A total size of 5-10 MB (a guesstimate for linux+osx+win, it'll be less thanks to gzipping) might be unconventional for npm packages, but users can themselves weigh that downside against the benefits of using RocksDB over LevelDB (or others).

@ralphtheninja @peakji thoughts?

@ralphtheninja
Copy link
Member

Is there any way you can strip debug symbols on windows?

@vweevers
Copy link
Member Author

Debug symbols are external, end up in a .pdb file. So doesn't matter AFAIK.

@vweevers
Copy link
Member Author

upx leveldown.node results in 1MB, slightly less than gzipping (by npm) (1.3MB).

@peakji
Copy link
Member

peakji commented May 19, 2019

With ROCKSDB_LITE it's 2.5MB but I can't get that build to work yet (some problem with column families, which we don't use) and I'm not eager to go down that rabbit hole.

I guess users choose RocksDB over (the default) LevelDB for its added features, thus we'd better provide the full version. After all, 2.5MB and 3.8MB aren't that different ;-P

@vweevers
Copy link
Member Author

I guess users choose RocksDB over (the default) LevelDB for its added features

@peakji It's mostly features that we don't expose in the JS layer. Notable exception is repair() but we could (choose to) live without it. More importantly though, I'm not sure ROCKSDB_LITE ever escaped its experimental status. Anyway, it was a "fun" option to explore.

@vweevers
Copy link
Member Author

I'll try to wrap things up into a PR tomorrow-ish, so we can see what the total size will be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Requires or pertains to refactoring semver-major Changes that break backward compatibility
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants