feat(bindings/java): Speed up the performance of OperatorInputStream#7088
feat(bindings/java): Speed up the performance of OperatorInputStream#7088
OperatorInputStream#7088Conversation
| ENV.with(|cell| { | ||
| let mut env = vm | ||
| .attach_current_thread_permanently() | ||
| .attach_current_thread_as_daemon() |
There was a problem hiding this comment.
I found that if we don't change this, the program will hang indefinitely in JMH.
cc @tisonkun what do you think about this change?
There was a problem hiding this comment.
We may tag @rib here.
Later we can try to migrate to jni-rs 0.22 but there should be some similar issues.
There was a problem hiding this comment.
A hang from not using _as_daemon implies you have some code that is using JavaVM::destroy() / DestroyJavaVM which is probably unsafe / unsound in any project with RAII types (including global references from the jni crate) that may result in jni usage after the vm is destroyed.
The most likely reason it appears to hang is because DestroyJavaVM will block and wait for (non-daemon) jni threads to exit.
Attaching as daemon threads will avoid that blocking but will almost certainly be completely unsound.
The hang itself indicates that the non-blocking case is unsound because it implies you have non-daemon threads that are associated with the jni crate which has RAII types that may trigger JNI usage after the VM has been destroyed.
The safe fix to avoid the blocking would be to have a mechanism to terminate any jni threads you have (e.g. destroying your tokio runtime + thread pool). Or don't ever try and destroy the VM (this is a very poorly defined operation).
There was a problem hiding this comment.
From the code itself there is no JavaVM::destroy() explicitly. But perhaps there are some implict calls.
Thanks for your information. I'll try to figure it out.
| while (bytes != null && offset >= bytes.length) { | ||
| bytes = readNextBytes(reader.nativeHandle); | ||
| offset = 0; | ||
| } | ||
|
|
||
| if (bytes == null) { | ||
| return -1; | ||
| } | ||
|
|
||
| final int n = Math.min(len, bytes.length - offset); | ||
| System.arraycopy(bytes, offset, b, off, n); | ||
| offset += n; | ||
| return n; |
There was a problem hiding this comment.
Perhaps read as much as len as possible?
Currently, it reads what remaining in the bytes buffer or read at most once.
| while (bytes != null && offset >= bytes.length) { | ||
| bytes = readNextBytes(reader.nativeHandle); | ||
| offset = 0; | ||
| } |
There was a problem hiding this comment.
Weird while here.
Perhaps loop over len > 0 and try to copy from bytes until len fulfilled or end.
Which issue does this PR close?
Closes #.
Rationale for this change
This PR is used to speed up the performance of
OperatorInputStream:Before:
After:
Still slower than
readRangebut much faster than before.What changes are included in this PR?
int read(byte[] b, int off, int len)forOperatorInputStreamAre there any user-facing changes?
AI Usage Statement
Parts of this PR were drafted with assistance from Codex (with
gpt-5.2) and fully reviewed and edited by me. I take full responsibility for all changes.