RocksJava: Improve 'get' performance #7597
Conversation
This reverts commit a21c035b2128eb728595895fe89bec847197ec77.
(Sometimes) when the function is NOT returning a copy and the array is released with JNI_ABORT mode, the writes to the array will be rolled back (maybe always).
There was a problem hiding this comment.
@zhichao-cao has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@alucarded has updated the pull request. You must reimport the pull request before landing. |
|
I will shortly do some final benchmarking, where I just check a couple of this before we merge this. |
| // For testing correctness: | ||
| // assert res > 0; | ||
| // final byte[] ret = new byte[valueSize]; | ||
| // valueBuf.get(ret); | ||
| // System.out.println(str(ret)); | ||
| // valueBuf.flip(); |
| assert(pinnable_slice != nullptr); | ||
| pinnable_slice->Reset(); | ||
| delete pinnable_slice; | ||
| } |
| ret_arr[0] = reinterpret_cast<jlong>(value_slice); | ||
| ret_arr[1] = reinterpret_cast<jlong>(value_slice->data()); | ||
|
|
||
| env->ReleasePrimitiveArrayCritical(jret_data, ret_arr, 0); | ||
|
|
||
| *has_exception = false; | ||
|
|
||
| return static_cast<jint>(value_slice->size_); |
There was a problem hiding this comment.
can we construct a PinnableSlice instance and just return it here?
| } | ||
|
|
||
| @Benchmark | ||
| public void preallocatedByteBufferGet() throws RocksDBException { |
There was a problem hiding this comment.
seems this test has the best throughput
| static const int kStatusError = -2; | ||
|
|
||
| char* key = | ||
| reinterpret_cast<char*>(env->GetPrimitiveArrayCritical(jkey, nullptr)); |
There was a problem hiding this comment.
can we use direct buffer to pass key to prevent GetPrimitiveArrayCritical+ReleasePrimitiveArrayCritical?
| ROCKSDB_NAMESPACE::Slice key_slice(key_start_ptr, jkey_len); | ||
|
|
||
| // TODO(yhchiang): we might save one memory allocation here by adding | ||
| // a DB::Get() function which takes preallocated jbyte* as input. |
There was a problem hiding this comment.
looking forward to this feature
|
Based on the Benchmark results provided by @alucarded (Benchmark results ("Score" column contains throughput in operations per second)). I have plotted a very simple rplot to show the results more quickly In summary, preallocated methods offer better performance as you can potentially allocate once and amortize the cost over subsequent calls.
@adamretter and @alanpaxton will pick up this work that our colleague @alucarded started, and continue with it. Likely producing one or two PR's that supersede and incorporate the moist important aspects of this PR. |
Benchmarks are a subset of facebook#7597 We just want to benchmark existing JNI methods with some potential performance improvements.
Benchmarks are a subset of facebook#7597 We just want to benchmark existing JNI methods with some potential performance improvements.
Benchmarks are a subset of facebook#7597 We just want to benchmark existing JNI methods with some potential performance improvements.
Summary: