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

chore(core): growable DirectByteSink in native code via byte_sink.h #3864

Merged
merged 25 commits into from
Oct 20, 2023

Conversation

amunra
Copy link
Contributor

@amunra amunra commented Oct 17, 2023

Technical enhancements to enable populating a byte sink from native code.

  • Extracted the core of DirectByteCharSink into io.questdb.std.bytes.DirectByteSink.
  • Reworked the DirectByteSink implementation by adding a layer of indirection and exposing its core as a C struct called questdb_byte_sink (see byte_sink.h).
  • This is to allow passing byte sinks to native code via pointer to allow the native code to append to them without calling back into Java via JNI when it is necessary to grow the buffer.

Additionally, this PR comes with:

  • Additional test cases.
  • A new minimum capacity size of 32 bytes (which is the de-facto malloc minimum).
  • On realloc, a more consistent new capacity calculation that finds the next power of two (see https://stackoverflow.com/questions/64734694/why-do-dynamic-arrays-have-to-geometrically-increase-their-capacity-to-gain-o1 on why this is valuable).
  • Introduction of a new io.questdb.std.bytes package and inside it:
    • interface ByteSequence
      • byte byteAt(int index), int size();
      • Note that a future PR from @puzpuzpuz will remove io.questdb.std.str.ByteSequence, so this name collision is temporary.
    • interface DirectByteSequence extends ByteSequence
      • long lo(), long hi(), long ptr()
    • interface NativeByteSink extends QuietCloseable
      • provides ptr() returning the aforementioned questdb_byte_sink*
      • and a close() method to ensure correct memory bookeeping after use.
    • interface BorrowableAsNativeByteSink
      • NativeByteSink borrowDirectByteSink();
      • For types that want to expose a NativeByteSink
    • class DirectByteSink (as aforementioned), containing the core logic for DirectByteCharSink, void of any CharSink logic.
  • Layered the existing {Direct}Utf8Sequence interfaces on top of the new {Direct}ByteSequence.

N.B.: DirectByteCharSink is also going to be removed and replaced by another utf-8 sink type in a future PR by @puzpuzpuz.

N.B.: ByteSequence, for now - as a compromise, has an int-based length rather than long. This is to allow for easier migration to these new interfaces.

@amunra amunra changed the title Native byte sink feat(core): native byte_sink Oct 17, 2023
@amunra amunra requested a review from puzpuzpuz October 17, 2023 21:40
@puzpuzpuz puzpuzpuz added the Core Related to storage, data type, etc. label Oct 18, 2023
Copy link
Contributor

@puzpuzpuz puzpuzpuz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all binaries are updated.

@bluestreak01
Copy link
Member

this does not look like a user facing "feature", PR title should reflect that (chore)

…ctByteSink and introduced JavaCritical_ JNI method variants. Also fixed MemoryTag.
@amunra amunra changed the title feat(core): native byte_sink chore(core): native byte_sink Oct 18, 2023
puzpuzpuz
puzpuzpuz previously approved these changes Oct 19, 2023
@amunra amunra marked this pull request as ready for review October 19, 2023 16:19
@amunra amunra changed the title chore(core): native byte_sink chore(core): growable DirectByteSink in native code via byte_sink.h Oct 19, 2023
@amunra
Copy link
Contributor Author

amunra commented Oct 19, 2023

While I don't have a use for it yet, we might want to be able to transfer ownership of the sink to/from Java and native code by leaking/acquiring the impl pointer.

E.g.:

Transfering ownership from Java to native code.

DirectByteSink dbs = ..;
long ptr = dbs.leak();  // pointer can be handed over to native code.

Transfering ownership from native code to Java.

// some JNI call that constructed the buffer earlier, maybe another thread.
long ptr = ...
DirectByteSink dbs = new DirectByteSink();
dbs.of(ptr);

Something to revisit if a need arises :-)

puzpuzpuz
puzpuzpuz previously approved these changes Oct 19, 2023
core/src/main/c/share/byte_sink.cpp Outdated Show resolved Hide resolved
core/src/main/java/io/questdb/std/Unsafe.java Show resolved Hide resolved
puzpuzpuz
puzpuzpuz previously approved these changes Oct 19, 2023
puzpuzpuz
puzpuzpuz previously approved these changes Oct 19, 2023
puzpuzpuz
puzpuzpuz previously approved these changes Oct 20, 2023
@bluestreak01 bluestreak01 merged commit 99a95d4 into master Oct 20, 2023
2 of 4 checks passed
@bluestreak01 bluestreak01 deleted the native_byte_sink branch October 20, 2023 10:28
@ideoma
Copy link
Collaborator

ideoma commented Oct 20, 2023

[PR Coverage check]

😍 pass : 113 / 116 (97.41%)

file detail

path covered line new line coverage
🔵 io/questdb/std/bytes/DirectByteSink.java 78 81 96.30%
🔵 io/questdb/std/bytes/DirectByteSequence.java 3 3 100.00%
🔵 io/questdb/std/str/DirectUtf8Sequence.java 3 3 100.00%
🔵 io/questdb/std/Unsafe.java 6 6 100.00%
🔵 io/questdb/ServerMain.java 4 4 100.00%
🔵 io/questdb/std/str/DirectByteCharSink.java 18 18 100.00%
🔵 io/questdb/std/MemoryTag.java 1 1 100.00%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Related to storage, data type, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants