From 5fe176d8f61ded795a9d26f94f25c65f274dd220 Mon Sep 17 00:00:00 2001 From: Bob Potter Date: Tue, 20 May 2014 18:52:27 -0500 Subject: [PATCH 1/5] Java Bindings: prevent segfault from double delete Give BackupableDB sole responsibility of the native object to prevent RocksDB from also trying to delete it. --- java/org/rocksdb/BackupableDB.java | 4 ++++ java/org/rocksdb/RocksDB.java | 12 ++++++++++++ 2 files changed, 16 insertions(+) diff --git a/java/org/rocksdb/BackupableDB.java b/java/org/rocksdb/BackupableDB.java index 91607d4df..b85f792fc 100644 --- a/java/org/rocksdb/BackupableDB.java +++ b/java/org/rocksdb/BackupableDB.java @@ -31,6 +31,10 @@ public class BackupableDB extends RocksDB { BackupableDB bdb = new BackupableDB(RocksDB.open(opt, db_path)); bdb.open(bdb.db_.nativeHandle_, bopt.nativeHandle_); + // Prevent the RocksDB object from attempting to delete + // the underly C++ DB object. + bdb.db_.disOwnNativeObject(); + return bdb; } diff --git a/java/org/rocksdb/RocksDB.java b/java/org/rocksdb/RocksDB.java index 23dad1dc4..bc0390d9f 100644 --- a/java/org/rocksdb/RocksDB.java +++ b/java/org/rocksdb/RocksDB.java @@ -337,6 +337,18 @@ public class RocksDB extends RocksObject { opt.filter_ = null; } + /** + * Revoke ownership of the native object. + * + * This will prevent the object from attempting to delete the underlying + * native object in its finalizer. This must be used when another object + * (e.g. BackupableDB) takes over ownership of the native object or both + * will attempt to delete the underlying object when garbage collected. + */ + protected void disOwnNativeObject() { + nativeHandle_ = 0; + } + // native methods protected native void open( long optionsHandle, long cacheSize, String path) throws RocksDBException; From 05dd018353aeb9448ccd02fb954389cb28090086 Mon Sep 17 00:00:00 2001 From: Bob Potter Date: Tue, 27 May 2014 13:54:02 -0500 Subject: [PATCH 2/5] BackupableDB: don't keep a reference to the RocksDB object now that we have disOwnNativeObject --- java/org/rocksdb/BackupableDB.java | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/java/org/rocksdb/BackupableDB.java b/java/org/rocksdb/BackupableDB.java index b85f792fc..f57d47df1 100644 --- a/java/org/rocksdb/BackupableDB.java +++ b/java/org/rocksdb/BackupableDB.java @@ -25,15 +25,14 @@ public class BackupableDB extends RocksDB { public static BackupableDB open( Options opt, BackupableDBOptions bopt, String db_path) throws RocksDBException { - // since BackupableDB c++ will handle the life cycle of - // the returned RocksDB of RocksDB.open(), here we store - // it as a BackupableDB member variable to avoid GC. - BackupableDB bdb = new BackupableDB(RocksDB.open(opt, db_path)); - bdb.open(bdb.db_.nativeHandle_, bopt.nativeHandle_); + + RocksDB db = RocksDB.open(opt, db_path); + BackupableDB bdb = new BackupableDB(); + bdb.open(db.nativeHandle_, bopt.nativeHandle_); // Prevent the RocksDB object from attempting to delete // the underly C++ DB object. - bdb.db_.disOwnNativeObject(); + db.disOwnNativeObject(); return bdb; } @@ -68,9 +67,8 @@ public class BackupableDB extends RocksDB { * A protected construction that will be used in the static factory * method BackupableDB.open(). */ - protected BackupableDB(RocksDB db) { + protected BackupableDB() { super(); - db_ = db; } @Override protected void finalize() { @@ -79,6 +77,4 @@ public class BackupableDB extends RocksDB { protected native void open(long rocksDBHandle, long backupDBOptionsHandle); protected native void createNewBackup(long handle, boolean flag); - - private final RocksDB db_; } From 8e41e54e1bc6dc26c2035793d3844ca174f8f883 Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Tue, 27 May 2014 13:52:44 -0700 Subject: [PATCH 3/5] [Java] Makes DbBenchmark takes 0 and 1 as boolean values. Summary: Originally, the boolean arguments in Java DB benchmark only takes true and false as valid input. This diff allows boolean arguments to be set using 0 or 1. Test Plan: make rocksdbjava make jdb_bench java/jdb_bench.sh --db=/tmp/path-not-exist --use_existing_db=1 java/jdb_bench.sh --db=/tmp/path-not-exist --use_existing_db=0 java/jdb_bench.sh --db=/tmp/path-not-exist --use_existing_db=1 Reviewers: haobo, dhruba, sdong, ankgup87, rsumbaly, swapnilghike, zzbennett Reviewed By: haobo Subscribers: leveldb Differential Revision: https://reviews.facebook.net/D18687 --- java/org/rocksdb/benchmark/DbBenchmark.java | 53 ++++++++++++--------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/java/org/rocksdb/benchmark/DbBenchmark.java b/java/org/rocksdb/benchmark/DbBenchmark.java index 5eec3e990..c34ae9b0a 100644 --- a/java/org/rocksdb/benchmark/DbBenchmark.java +++ b/java/org/rocksdb/benchmark/DbBenchmark.java @@ -534,8 +534,6 @@ public class DbBenchmark { (Long)flags_.get(Flag.block_size)); options.setMaxOpenFiles( (Integer)flags_.get(Flag.open_files)); - options.setCreateIfMissing( - !(Boolean)flags_.get(Flag.use_existing_db)); options.setTableCacheRemoveScanCountLimit( (Integer)flags_.get(Flag.cache_remove_scan_count_limit)); options.setDisableDataSync( @@ -939,7 +937,7 @@ public class DbBenchmark { "\tflag and also specify a benchmark that wants a fresh database,\n" + "\tthat benchmark will fail.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, num(1000000, @@ -1028,7 +1026,7 @@ public class DbBenchmark { use_plain_table(false, "Use plain-table sst format.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, cache_size(-1L, @@ -1085,7 +1083,7 @@ public class DbBenchmark { }, histogram(false,"Print histogram of operation timings.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, min_write_buffer_number_to_merge( @@ -1203,12 +1201,12 @@ public class DbBenchmark { verify_checksum(false,"Verify checksum for every block read\n" + "\tfrom storage.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, statistics(false,"Database statistics.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, writes(-1,"Number of write operations to do. If negative, do\n" + @@ -1219,23 +1217,23 @@ public class DbBenchmark { }, sync(false,"Sync all writes to disk.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, disable_data_sync(false,"If true, do not wait until data is\n" + "\tsynced to disk.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, use_fsync(false,"If true, issue fsync instead of fdatasync.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, disable_wal(false,"If true, do not write WAL for write.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, wal_dir("", "If not empty, use the given dir for WAL.") { @@ -1312,7 +1310,7 @@ public class DbBenchmark { disable_seek_compaction(false,"Option to disable compaction\n" + "\ttriggered by read.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, delete_obsolete_files_period_micros(0,"Option to delete\n" + @@ -1393,12 +1391,12 @@ public class DbBenchmark { }, readonly(false,"Run read only benchmarks.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, disable_auto_compactions(false,"Do not auto trigger compactions.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, source_compaction_factor(1,"Cap the size of data in level-K for\n" + @@ -1423,26 +1421,26 @@ public class DbBenchmark { bufferedio(rocksdb::EnvOptions().use_os_buffer, "Allow buffered io using OS buffers.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, */ mmap_read(false, "Allow reads to occur via mmap-ing files.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, mmap_write(false, "Allow writes to occur via mmap-ing files.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, advise_random_on_open(defaultOptions_.adviseRandomOnOpen(), "Advise random access on table file open.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, compaction_fadvice("NORMAL", @@ -1454,13 +1452,13 @@ public class DbBenchmark { use_tailing_iterator(false, "Use tailing iterator to access a series of keys instead of get.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, use_adaptive_mutex(defaultOptions_.useAdaptiveMutex(), "Use adaptive mutex.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, bytes_per_sync(defaultOptions_.bytesPerSync(), @@ -1474,7 +1472,7 @@ public class DbBenchmark { filter_deletes(false," On true, deletes use bloom-filter and drop\n" + "\tthe delete if key not present.") { @Override public Object parseValue(String value) { - return Boolean.parseBoolean(value); + return parseBoolean(value); } }, max_successive_merges(0,"Maximum number of successive merge\n" + @@ -1495,8 +1493,6 @@ public class DbBenchmark { desc_ = desc; } - protected abstract Object parseValue(String value); - public Object getDefaultValue() { return defaultValue_; } @@ -1505,6 +1501,17 @@ public class DbBenchmark { return desc_; } + public boolean parseBoolean(String value) { + if (value.equals("1")) { + return true; + } else if (value.equals("0")) { + return false; + } + return Boolean.parseBoolean(value); + } + + protected abstract Object parseValue(String value); + private final Object defaultValue_; private final String desc_; } From ab3e566699b17469e27522b791e72c8a3f20d36d Mon Sep 17 00:00:00 2001 From: Yueh-Hsuan Chiang Date: Wed, 28 May 2014 18:16:29 -0700 Subject: [PATCH 4/5] [Java] Generalize dis-own native handle and refine dispose framework. Summary: 1. Move disOwnNativeHandle() function from RocksDB to RocksObject to allow other RocksObject to use disOwnNativeHandle() when its ownership of native handle has been transferred. 2. RocksObject now has an abstract implementation of dispose(), which does the following two things. First, it checks whether both isOwningNativeHandle() and isInitialized() return true. If so, it will call the protected abstract function dispose0(), which all the subclasses of RocksObject should implement. Second, it sets nativeHandle_ = 0. This redesign ensure all subclasses of RocksObject have the same dispose behavior. 3. All subclasses of RocksObject now should implement dispose0() instead of dispose(), and dispose0() will be called only when isInitialized() returns true. Test Plan: make rocksdbjava make jtest Reviewers: dhruba, sdong, ankgup87, rsumbaly, swapnilghike, zzbennett, haobo Reviewed By: haobo Subscribers: leveldb Differential Revision: https://reviews.facebook.net/D18801 --- java/org/rocksdb/BackupableDB.java | 2 +- java/org/rocksdb/BackupableDBOptions.java | 9 +++--- java/org/rocksdb/Filter.java | 9 +++--- java/org/rocksdb/Options.java | 9 +++--- java/org/rocksdb/ReadOptions.java | 21 +++++------- java/org/rocksdb/RocksDB.java | 26 +++------------ java/org/rocksdb/RocksIterator.java | 10 +++--- java/org/rocksdb/RocksObject.java | 39 ++++++++++++++++++++++- java/org/rocksdb/WriteBatch.java | 9 +++--- java/org/rocksdb/WriteOptions.java | 9 +++--- java/rocksjni/backupablejni.cc | 4 +-- java/rocksjni/filter.cc | 9 ++---- java/rocksjni/iterator.cc | 4 +-- java/rocksjni/options.cc | 20 ++++++------ java/rocksjni/rocksjni.cc | 8 ++--- java/rocksjni/write_batch.cc | 13 +++----- 16 files changed, 99 insertions(+), 102 deletions(-) diff --git a/java/org/rocksdb/BackupableDB.java b/java/org/rocksdb/BackupableDB.java index f57d47df1..90d4a2a9a 100644 --- a/java/org/rocksdb/BackupableDB.java +++ b/java/org/rocksdb/BackupableDB.java @@ -32,7 +32,7 @@ public class BackupableDB extends RocksDB { // Prevent the RocksDB object from attempting to delete // the underly C++ DB object. - db.disOwnNativeObject(); + db.disOwnNativeHandle(); return bdb; } diff --git a/java/org/rocksdb/BackupableDBOptions.java b/java/org/rocksdb/BackupableDBOptions.java index 2c64b60a3..f229d88aa 100644 --- a/java/org/rocksdb/BackupableDBOptions.java +++ b/java/org/rocksdb/BackupableDBOptions.java @@ -32,13 +32,12 @@ public class BackupableDBOptions extends RocksObject { * Release the memory allocated for the current instance * in the c++ side. */ - @Override public synchronized void dispose() { - if (isInitialized()) { - dispose(nativeHandle_); - } + @Override protected void disposeInternal() { + assert(isInitialized()); + disposeInternal(nativeHandle_); } private native void newBackupableDBOptions(String path); private native String backupDir(long handle); - private native void dispose(long handle); + private native void disposeInternal(long handle); } diff --git a/java/org/rocksdb/Filter.java b/java/org/rocksdb/Filter.java index 3a01ad4ee..ce5c41f26 100644 --- a/java/org/rocksdb/Filter.java +++ b/java/org/rocksdb/Filter.java @@ -22,11 +22,10 @@ public abstract class Filter extends RocksObject { * RocksDB instances referencing the filter are closed. * Otherwise an undefined behavior will occur. */ - @Override public synchronized void dispose() { - if (isInitialized()) { - dispose0(nativeHandle_); - } + @Override protected void disposeInternal() { + assert(isInitialized()); + disposeInternal(nativeHandle_); } - private native void dispose0(long handle); + private native void disposeInternal(long handle); } diff --git a/java/org/rocksdb/Options.java b/java/org/rocksdb/Options.java index 02d3e200a..af4b82fab 100644 --- a/java/org/rocksdb/Options.java +++ b/java/org/rocksdb/Options.java @@ -2311,10 +2311,9 @@ public class Options extends RocksObject { * Release the memory allocated for the current instance * in the c++ side. */ - @Override public synchronized void dispose() { - if (isInitialized()) { - dispose0(); - } + @Override protected void disposeInternal() { + assert(isInitialized()); + disposeInternal(nativeHandle_); } static final int DEFAULT_PLAIN_TABLE_BLOOM_BITS_PER_KEY = 10; @@ -2322,7 +2321,7 @@ public class Options extends RocksObject { static final int DEFAULT_PLAIN_TABLE_INDEX_SPARSENESS = 16; private native void newOptions(); - private native void dispose0(); + private native void disposeInternal(long handle); private native void setCreateIfMissing(long handle, boolean flag); private native boolean createIfMissing(long handle); private native void setWriteBufferSize(long handle, long writeBufferSize); diff --git a/java/org/rocksdb/ReadOptions.java b/java/org/rocksdb/ReadOptions.java index 23250fc6f..97c47c7d6 100644 --- a/java/org/rocksdb/ReadOptions.java +++ b/java/org/rocksdb/ReadOptions.java @@ -18,19 +18,6 @@ public class ReadOptions extends RocksObject { } private native void newReadOptions(); - /** - * Release the memory allocated for the current instance - * in the c++ side. - * - * Calling other methods after dispose() leads to undefined behavior. - */ - @Override public synchronized void dispose() { - if (isInitialized()) { - dispose(nativeHandle_); - } - } - private native void dispose(long handle); - /** * If true, all data read from underlying storage will be * verified against corresponding checksums. @@ -127,4 +114,12 @@ public class ReadOptions extends RocksObject { } private native void setTailing( long handle, boolean tailing); + + + @Override protected void disposeInternal() { + assert(isInitialized()); + disposeInternal(nativeHandle_); + } + private native void disposeInternal(long handle); + } diff --git a/java/org/rocksdb/RocksDB.java b/java/org/rocksdb/RocksDB.java index bc0390d9f..1b758e1a2 100644 --- a/java/org/rocksdb/RocksDB.java +++ b/java/org/rocksdb/RocksDB.java @@ -114,11 +114,9 @@ public class RocksDB extends RocksObject { return db; } - @Override public synchronized void dispose() { - if (isInitialized()) { - dispose(nativeHandle_); - nativeHandle_ = 0; - } + @Override protected void disposeInternal() { + assert(isInitialized()); + disposeInternal(nativeHandle_); } /** @@ -315,10 +313,6 @@ public class RocksDB extends RocksObject { return new RocksIterator(iterator0(nativeHandle_)); } - @Override protected void finalize() { - close(); - } - /** * Private constructor. */ @@ -337,18 +331,6 @@ public class RocksDB extends RocksObject { opt.filter_ = null; } - /** - * Revoke ownership of the native object. - * - * This will prevent the object from attempting to delete the underlying - * native object in its finalizer. This must be used when another object - * (e.g. BackupableDB) takes over ownership of the native object or both - * will attempt to delete the underlying object when garbage collected. - */ - protected void disOwnNativeObject() { - nativeHandle_ = 0; - } - // native methods protected native void open( long optionsHandle, long cacheSize, String path) throws RocksDBException; @@ -382,7 +364,7 @@ public class RocksDB extends RocksObject { long handle, long writeOptHandle, byte[] key, int keyLen) throws RocksDBException; protected native long iterator0(long optHandle); - protected native void dispose(long handle); + private native void disposeInternal(long handle); protected Filter filter_; } diff --git a/java/org/rocksdb/RocksIterator.java b/java/org/rocksdb/RocksIterator.java index 5b7d2c172..9ef2e8c24 100644 --- a/java/org/rocksdb/RocksIterator.java +++ b/java/org/rocksdb/RocksIterator.java @@ -118,15 +118,13 @@ public class RocksIterator extends RocksObject { /** * Deletes underlying C++ iterator pointer. */ - @Override public synchronized void dispose() { - if(isInitialized()) { - dispose(nativeHandle_); - nativeHandle_ = 0; - } + @Override protected void disposeInternal() { + assert(isInitialized()); + disposeInternal(nativeHandle_); } private native boolean isValid0(long handle); - private native void dispose(long handle); + private native void disposeInternal(long handle); private native void seekToFirst0(long handle); private native void seekToLast0(long handle); private native void next0(long handle); diff --git a/java/org/rocksdb/RocksObject.java b/java/org/rocksdb/RocksObject.java index 6e36cbaea..31c347daa 100644 --- a/java/org/rocksdb/RocksObject.java +++ b/java/org/rocksdb/RocksObject.java @@ -16,12 +16,48 @@ package org.rocksdb; public abstract class RocksObject { protected RocksObject() { nativeHandle_ = 0; + owningHandle_ = true; } /** * Release the c++ object pointed by the native handle. + * + * Note that once an instance of RocksObject has been disposed, + * calling its function will lead undefined behavior. */ - public abstract void dispose(); + public final synchronized void dispose() { + if (isOwningNativeHandle() && isInitialized()) { + disposeInternal(); + } + nativeHandle_ = 0; + disOwnNativeHandle(); + } + + /** + * The helper function of dispose() which all subclasses of RocksObject + * must implement to release their associated C++ resource. + */ + protected abstract void disposeInternal(); + + /** + * Revoke ownership of the native object. + * + * This will prevent the object from attempting to delete the underlying + * native object in its finalizer. This must be used when another object + * takes over ownership of the native object or both will attempt to delete + * the underlying object when garbage collected. + * + * When disOwnNativeHandle is called, dispose() will simply set nativeHandle_ + * to 0 without releasing its associated C++ resource. As a result, + * incorrectly use this function may cause memory leak. + */ + protected void disOwnNativeHandle() { + owningHandle_ = false; + } + + protected boolean isOwningNativeHandle() { + return owningHandle_; + } protected boolean isInitialized() { return (nativeHandle_ != 0); @@ -32,4 +68,5 @@ public abstract class RocksObject { } protected long nativeHandle_; + private boolean owningHandle_; } diff --git a/java/org/rocksdb/WriteBatch.java b/java/org/rocksdb/WriteBatch.java index 1ddbd449d..f538dc1a0 100644 --- a/java/org/rocksdb/WriteBatch.java +++ b/java/org/rocksdb/WriteBatch.java @@ -86,10 +86,9 @@ public class WriteBatch extends RocksObject { /** * Delete the c++ side pointer. */ - @Override public synchronized void dispose() { - if (isInitialized()) { - dispose0(); - } + @Override protected void disposeInternal() { + assert(isInitialized()); + disposeInternal(nativeHandle_); } private native void newWriteBatch(int reserved_bytes); @@ -99,7 +98,7 @@ public class WriteBatch extends RocksObject { byte[] value, int valueLen); private native void remove(byte[] key, int keyLen); private native void putLogData(byte[] blob, int blobLen); - private native void dispose0(); + private native void disposeInternal(long handle); } /** diff --git a/java/org/rocksdb/WriteOptions.java b/java/org/rocksdb/WriteOptions.java index f4a1d6a0f..d26dbb918 100644 --- a/java/org/rocksdb/WriteOptions.java +++ b/java/org/rocksdb/WriteOptions.java @@ -17,10 +17,9 @@ public class WriteOptions extends RocksObject { newWriteOptions(); } - @Override public synchronized void dispose() { - if (isInitialized()) { - dispose0(nativeHandle_); - } + @Override protected void disposeInternal() { + assert(isInitialized()); + disposeInternal(nativeHandle_); } /** @@ -96,5 +95,5 @@ public class WriteOptions extends RocksObject { private native boolean sync(long handle); private native void setDisableWAL(long handle, boolean flag); private native boolean disableWAL(long handle); - private native void dispose0(long handle); + private native void disposeInternal(long handle); } diff --git a/java/rocksjni/backupablejni.cc b/java/rocksjni/backupablejni.cc index 8b57a0c62..956912ef1 100644 --- a/java/rocksjni/backupablejni.cc +++ b/java/rocksjni/backupablejni.cc @@ -72,10 +72,10 @@ jstring Java_org_rocksdb_BackupableDBOptions_backupDir( /* * Class: org_rocksdb_BackupableDBOptions - * Method: dispose + * Method: disposeInternal * Signature: (J)V */ -void Java_org_rocksdb_BackupableDBOptions_dispose( +void Java_org_rocksdb_BackupableDBOptions_disposeInternal( JNIEnv* env, jobject jopt, jlong jhandle) { auto bopt = reinterpret_cast(jhandle); assert(bopt); diff --git a/java/rocksjni/filter.cc b/java/rocksjni/filter.cc index 7ef959814..572b4a66d 100644 --- a/java/rocksjni/filter.cc +++ b/java/rocksjni/filter.cc @@ -29,13 +29,10 @@ void Java_org_rocksdb_BloomFilter_createNewFilter0( /* * Class: org_rocksdb_Filter - * Method: dispose0 + * Method: disposeInternal * Signature: (J)V */ -void Java_org_rocksdb_Filter_dispose0( +void Java_org_rocksdb_Filter_disposeInternal( JNIEnv* env, jobject jobj, jlong handle) { - auto fp = reinterpret_cast(handle); - delete fp; - - rocksdb::FilterJni::setHandle(env, jobj, nullptr); + delete reinterpret_cast(handle); } diff --git a/java/rocksjni/iterator.cc b/java/rocksjni/iterator.cc index 4c18a3491..84b0b3133 100644 --- a/java/rocksjni/iterator.cc +++ b/java/rocksjni/iterator.cc @@ -135,10 +135,10 @@ void Java_org_rocksdb_RocksIterator_status0( /* * Class: org_rocksdb_RocksIterator - * Method: dispose + * Method: disposeInternal * Signature: (J)V */ -void Java_org_rocksdb_RocksIterator_dispose( +void Java_org_rocksdb_RocksIterator_disposeInternal( JNIEnv* env, jobject jobj, jlong handle) { auto it = reinterpret_cast(handle); delete it; diff --git a/java/rocksjni/options.cc b/java/rocksjni/options.cc index c5849ce39..003d353e6 100644 --- a/java/rocksjni/options.cc +++ b/java/rocksjni/options.cc @@ -35,14 +35,12 @@ void Java_org_rocksdb_Options_newOptions(JNIEnv* env, jobject jobj) { /* * Class: org_rocksdb_Options - * Method: dispose0 - * Signature: ()V + * Method: disposeInternal + * Signature: (J)V */ -void Java_org_rocksdb_Options_dispose0(JNIEnv* env, jobject jobj) { - rocksdb::Options* op = rocksdb::OptionsJni::getHandle(env, jobj); - delete op; - - rocksdb::OptionsJni::setHandle(env, jobj, nullptr); +void Java_org_rocksdb_Options_disposeInternal( + JNIEnv* env, jobject jobj, jlong handle) { + delete reinterpret_cast(handle); } /* @@ -1665,10 +1663,10 @@ void Java_org_rocksdb_WriteOptions_newWriteOptions( /* * Class: org_rocksdb_WriteOptions - * Method: dispose0 + * Method: disposeInternal * Signature: ()V */ -void Java_org_rocksdb_WriteOptions_dispose0( +void Java_org_rocksdb_WriteOptions_disposeInternal( JNIEnv* env, jobject jwrite_options, jlong jhandle) { auto write_options = reinterpret_cast(jhandle); delete write_options; @@ -1732,10 +1730,10 @@ void Java_org_rocksdb_ReadOptions_newReadOptions( /* * Class: org_rocksdb_ReadOptions - * Method: dispose + * Method: disposeInternal * Signature: (J)V */ -void Java_org_rocksdb_ReadOptions_dispose( +void Java_org_rocksdb_ReadOptions_disposeInternal( JNIEnv* env, jobject jobj, jlong jhandle) { delete reinterpret_cast(jhandle); rocksdb::ReadOptionsJni::setHandle(env, jobj, nullptr); diff --git a/java/rocksjni/rocksjni.cc b/java/rocksjni/rocksjni.cc index 1f2941f42..697bd0cef 100644 --- a/java/rocksjni/rocksjni.cc +++ b/java/rocksjni/rocksjni.cc @@ -419,14 +419,12 @@ void Java_org_rocksdb_RocksDB_remove__JJ_3BI( /* * Class: org_rocksdb_RocksDB - * Method: dispose + * Method: disposeInternal * Signature: (J)V */ -void Java_org_rocksdb_RocksDB_dispose( +void Java_org_rocksdb_RocksDB_disposeInternal( JNIEnv* env, jobject java_db, jlong jhandle) { - auto db = reinterpret_cast(jhandle); - assert(db != nullptr); - delete db; + delete reinterpret_cast(jhandle); } /* diff --git a/java/rocksjni/write_batch.cc b/java/rocksjni/write_batch.cc index 035b35f6f..e8b2456ee 100644 --- a/java/rocksjni/write_batch.cc +++ b/java/rocksjni/write_batch.cc @@ -134,15 +134,12 @@ void Java_org_rocksdb_WriteBatch_putLogData( /* * Class: org_rocksdb_WriteBatch - * Method: dispose0 - * Signature: ()V + * Method: disposeInternal + * Signature: (J)V */ -void Java_org_rocksdb_WriteBatch_dispose0(JNIEnv* env, jobject jobj) { - rocksdb::WriteBatch* wb = rocksdb::WriteBatchJni::getHandle(env, jobj); - assert(wb != nullptr); - delete wb; - - rocksdb::WriteBatchJni::setHandle(env, jobj, nullptr); +void Java_org_rocksdb_WriteBatch_disposeInternal( + JNIEnv* env, jobject jobj, jlong handle) { + delete reinterpret_cast(handle); } /* From 9899b12780f779dad707fdac0b042eff4bf70f1a Mon Sep 17 00:00:00 2001 From: sdong Date: Thu, 29 May 2014 10:57:22 -0700 Subject: [PATCH 5/5] ThreadID printed when Thread terminating in the same format as posix_logger Summary: https://github.com/facebook/rocksdb/commit/220132b65ec17abb037d3e79d5abf6ca8d797b96 correctly fixed the issue of thread ID printing when terminating a thread. Nothing wrong with it. This diff prints the ID in the same way as in PosixLogger::logv() so that users can be more easily to correlates them. Test Plan: run env_test and make sure it prints correctly. Reviewers: igor, haobo, ljin, yhchiang Reviewed By: yhchiang Subscribers: dhruba, leveldb Differential Revision: https://reviews.facebook.net/D18819 --- util/env_posix.cc | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/util/env_posix.cc b/util/env_posix.cc index 1f8c3bcf2..267958606 100644 --- a/util/env_posix.cc +++ b/util/env_posix.cc @@ -94,17 +94,6 @@ static Status IOError(const std::string& context, int err_number) { return Status::IOError(context, strerror(err_number)); } -// TODO(sdong): temp logging. Need to help debugging. Remove it when -// the feature is proved to be stable. -inline void PrintThreadInfo(size_t thread_id, pthread_t id) { - unsigned char* ptc = (unsigned char*)(void*)(&id); - fprintf(stdout, "Bg thread %zu terminates 0x", thread_id); - for (size_t i = 0; i < sizeof(id); i++) { - fprintf(stdout, "%02x", (unsigned)(ptc[i])); - } - fprintf(stdout, "\n"); -} - #ifdef NDEBUG // empty in release build #define TEST_KILL_RANDOM(rocksdb_kill_odds) @@ -1293,13 +1282,17 @@ class PosixEnv : public Env { return Status::OK(); } - static uint64_t gettid() { - pthread_t tid = pthread_self(); + static uint64_t gettid(pthread_t tid) { uint64_t thread_id = 0; memcpy(&thread_id, &tid, std::min(sizeof(thread_id), sizeof(tid))); return thread_id; } + static uint64_t gettid() { + pthread_t tid = pthread_self(); + return gettid(tid); + } + virtual Status NewLogger(const std::string& fname, shared_ptr* result) { FILE* f = fopen(fname.c_str(), "w"); @@ -1525,7 +1518,8 @@ class PosixEnv : public Env { PthreadCall("unlock", pthread_mutex_unlock(&mu_)); // TODO(sdong): temp logging. Need to help debugging. Remove it when // the feature is proved to be stable. - PrintThreadInfo(thread_id, terminating_thread); + fprintf(stdout, "Bg thread %zu terminates %llx\n", thread_id, + static_cast(gettid())); break; } void (*function)(void*) = queue_.front().function;