Extend Java RocksDB iterators to support indirect Byte Buffers #9222

alanpaxton · 2021-11-29T09:11:25Z

Extend Java RocksDB iterators to support indirect byte buffers, to add to the existing support for direct byte buffers.
Code to distinguish direct/indirect buffers is switched in Java, and a 2nd separate JNI call implemented to support indirect
buffers. Indirect support passes contained buffers using byte[]

There are some Java subclasses of iterator (WBWIIterator, SstFileReaderIterator) which also now have parallel JNI support functions implemented, along with direct/indirect switches in Java methods.

Closes #6282

adamretter

LGTM :-)
Thanks @alanpaxton

java/src/test/java/org/rocksdb/util/ByteBufferAllocator.java

javeme · 2021-12-06T15:35:38Z

java/rocksjni/sst_file_reader_iterator.cc

+    jint jkey_len) {
+  auto* it = reinterpret_cast<ROCKSDB_NAMESPACE::Iterator*>(handle);
+  ROCKSDB_NAMESPACE::Slice key_slice = it->key();
+  jsize copy_size = std::min(static_cast<uint32_t>(key_slice.size()),


define var actual_size=key_slice.size()

I've done auto slice_size = key_slice.size(); ? (actual suggests to me the presence of non-actual).

javeme · 2021-12-06T15:35:51Z

java/rocksjni/sst_file_reader_iterator.cc

+      jkey, jkey_off, copy_size,
+      const_cast<jbyte*>(reinterpret_cast<const jbyte*>(key_slice.data())));
+
+  return static_cast<jsize>(key_slice.size());


can use actual_size here

javeme · 2021-12-06T15:36:10Z

java/rocksjni/sst_file_reader_iterator.cc

+    jint jvalue_off, jint jvalue_len) {
+  auto* it = reinterpret_cast<ROCKSDB_NAMESPACE::Iterator*>(handle);
+  ROCKSDB_NAMESPACE::Slice value_slice = it->value();
+  jsize copy_size = std::min(static_cast<uint32_t>(value_slice.size()),


java/src/main/java/org/rocksdb/RocksIterator.java

java/rocksjni/iterator.cc

javeme · 2021-12-06T15:46:27Z

java/src/main/java/org/rocksdb/RocksIterator.java

-  public int value(ByteBuffer value) {
-    assert (isOwningHandle() && value.isDirect());
-    int result = valueDirect0(nativeHandle_, value, value.position(), value.remaining());
+  public int value(final ByteBuffer value) {


do we plan to support more methods in the future? RocksDB.get/put/delete and WriteBatch.put/delete

@javeme Yes, but that will be done in separate PRs

adamretter · 2021-12-20T09:47:21Z

@jay-zhuang @mrambacher Can we get some review/merge here please?

mrambacher · 2021-12-20T14:46:09Z

java/rocksjni/iterator.cc

+  auto* it = reinterpret_cast<ROCKSDB_NAMESPACE::Iterator*>(handle);
+  it->Seek(target_slice);
+}
+


Can we do something either through templates or otherwise to reduce the code duplication here? Essentially the same code is in multiple places.

I've done some helper-based stuff.

mrambacher · 2021-12-20T14:47:28Z

java/rocksjni/iterator.cc

+void Java_org_rocksdb_RocksIterator_seekByteArray0(
+    JNIEnv* env, jobject /*jobj*/, jlong handle, jbyteArray jtarget,
+    jint jtarget_off, jint jtarget_len) {
+  const std::unique_ptr<char[]> target(new char[jtarget_len]);


I want to understand if this is correct. Memory is being allocated here and freed at the end of this method. But it is also being passed to GetByteArrayRegion. Some comments here would help those (like me) who are less familiar with the JNIEnv understand what is going on.

GetByteArrayRegion is a copy from the Java object into a C++ buffer (in this case the char[] contents). These are being used as the slice key to the seek. I know you need to look up what GetByteArrayRegion does to grok this, but I think an accompanying comment would turn into an overly verbose half-wrong explanation of GetByteArrayRegion and I think the right thing is to let the code speak for itself, at the cost of a bit of reading time. I'm willing to be argued out of this position.

alanpaxton · 2022-02-14T13:09:10Z

I've gone through and done what I hope answers your concerns @Javame @mrambacher . I think it would be good for this to get into Rocks 7 if you can give it the once over.

adamretter · 2022-02-14T14:51:45Z

@jay-zhuang Can we get this merged for Rocks 7 please?

adamretter

LGTM. Thanks @alanpaxton

Rework tests to be TDD for key(), value() Implement JNI for key() for indirect Left to do - tests for seek(), implement value(), and seek()

And associated test now passes. SstFileReaderIterator and WBWIRocksIterator stubbed.

Tests for WBWI. A bit of improvement to the WBWI tests.

Same pattern of implementing a native byte[] method with offsets.

seek0()/seekForPrev0(), methods using GetByteArrayElements now share a helper with seekByteArray0()/seekByteArrayForPrev0().

darrenclark · 2022-03-24T14:40:51Z

Thanks for making this change, really looking forward to this!

I ran some quick benchmarks using indirect ByteBuffers in our iterator-heavy app and it was a significant performance boost.

facebook-github-bot · 2022-03-24T15:30:17Z

@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot added the CLA Signed label Nov 29, 2021

adamretter changed the title ~~Fb 6282 java iterator bb extension~~ Extend Java RocksDB iterators to support indirect Byte Buffers Nov 29, 2021

adamretter self-requested a review November 29, 2021 13:28

adamretter added the java-api label Nov 29, 2021

adamretter approved these changes Nov 29, 2021

View reviewed changes

javeme reviewed Dec 6, 2021

View reviewed changes

mrambacher reviewed Dec 20, 2021

View reviewed changes

alanpaxton force-pushed the fb-6282-java-iterator-bb-extension branch from e90a6a3 to 48ab703 Compare January 17, 2022 10:19

alanpaxton force-pushed the fb-6282-java-iterator-bb-extension branch from a9cc7ae to c59f2eb Compare February 14, 2022 11:53

adamretter approved these changes Feb 14, 2022

View reviewed changes

Alan Paxton and others added 15 commits March 24, 2022 10:09

Bytebuffer support via byte[] for iterator key()

3ec22ea

Rework tests to be TDD for key(), value() Implement JNI for key() for indirect Left to do - tests for seek(), implement value(), and seek()

Indirect buffer cases for RocksIterator

1180b65

And associated test now passes. SstFileReaderIterator and WBWIRocksIterator stubbed.

format fixes

49d27b7

WBWI iterator w/ indirect BB.

d830b0d

Tests for WBWI. A bit of improvement to the WBWI tests.

Pulll out ByteBufferAlllocator

c06b0e0

SstFileReaderIterator indirect BB seek, get, value

90cf96d

Same pattern of implementing a native byte[] method with offsets.

Sliced bytebuffer tests

9e605c7

Method comments

6112465

Avoid unnessecary object creation

c82ae28

Fix code for Java 7 (previously some syntax was Java 14+)

5683534

format fixes

812fdfa

fix name shadow which is a CI build error

554358b

Tiny review items

bb82987

Factor out common code, use GetByteArrayRegion

e1702f1

seek0()/seekForPrev0(), methods using GetByteArrayElements now share a helper with seekByteArray0()/seekByteArrayForPrev0().

Name changes per review, small test class re-org

58234c5

alanpaxton force-pushed the fb-6282-java-iterator-bb-extension branch from 6e19562 to 58234c5 Compare March 24, 2022 10:09

facebook-github-bot closed this in dec144f Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend Java RocksDB iterators to support indirect Byte Buffers #9222

Extend Java RocksDB iterators to support indirect Byte Buffers #9222

alanpaxton commented Nov 29, 2021 •

edited

Loading

adamretter left a comment

javeme Dec 6, 2021

alanpaxton Feb 14, 2022

javeme Dec 6, 2021

alanpaxton Feb 14, 2022

javeme Dec 6, 2021

alanpaxton Feb 14, 2022

javeme Dec 6, 2021

adamretter Dec 7, 2021 •

edited

Loading

adamretter commented Dec 20, 2021

mrambacher Dec 20, 2021

alanpaxton Feb 14, 2022

mrambacher Dec 20, 2021

alanpaxton Jan 18, 2022

alanpaxton commented Feb 14, 2022

adamretter commented Feb 14, 2022

adamretter left a comment

darrenclark commented Mar 24, 2022

facebook-github-bot commented Mar 24, 2022

Extend Java RocksDB iterators to support indirect Byte Buffers #9222

Extend Java RocksDB iterators to support indirect Byte Buffers #9222

Conversation

alanpaxton commented Nov 29, 2021 • edited Loading

adamretter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamretter Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

adamretter commented Dec 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanpaxton commented Feb 14, 2022

adamretter commented Feb 14, 2022

adamretter left a comment

Choose a reason for hiding this comment

darrenclark commented Mar 24, 2022

facebook-github-bot commented Mar 24, 2022

alanpaxton commented Nov 29, 2021 •

edited

Loading

adamretter Dec 7, 2021 •

edited

Loading