You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Xuanwo (via GitHub)" <gi...@apache.org> on 2025/12/22 14:55:34 UTC
[PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Xuanwo opened a new pull request, #7088:
URL: https://github.com/apache/opendal/pull/7088
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
# Rationale for this change
This PR is used to speed up the performance of `OperatorInputStream`:
Before:
```shell
❯ java -jar target/benchmarks.jar org.apache.opendal.bench.jmh.OperatorReadJmhBenchmark
Benchmark MiB/s
org.apache.opendal.bench.jmh.OperatorReadJmhBenchmark.createInputStream 1468.264
org.apache.opendal.bench.jmh.OperatorReadJmhBenchmark.readRange 44586.656
```
After:
```shell
❯ java -jar target/benchmarks.jar org.apache.opendal.bench.jmh.OperatorReadJmhBenchmark
Benchmark MiB/s
org.apache.opendal.bench.jmh.OperatorReadJmhBenchmark.createInputStream 22430.267
org.apache.opendal.bench.jmh.OperatorReadJmhBenchmark.readRange 48399.451
```
Still slower than `readRange` but much faster than before.
# What changes are included in this PR?
- Add a basic benchmark framework for opendal java
- Implement `int read(byte[] b, int off, int len)` for `OperatorInputStream`
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking-changes` label.
-->
# AI Usage Statement
<!--
If you are using AI tools to build this PR, please include a statement specifying the tool and models you are using.
-->
**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.**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #7088:
URL: https://github.com/apache/opendal/pull/7088#discussion_r2640208265
##########
bindings/java/src/main/java/org/apache/opendal/OperatorInputStream.java:
##########
@@ -58,6 +58,33 @@ public int read() throws IOException {
return -1;
}
+ @Override
+ public int read(byte[] b, int off, int len) throws IOException {
+ if (b == null) {
+ throw new NullPointerException();
+ }
+ if (off < 0 || len < 0 || len > b.length - off) {
+ throw new IndexOutOfBoundsException();
+ }
+ if (len == 0) {
+ return 0;
+ }
+
+ 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;
Review Comment:
Perhaps read as much as `len` as possible?
Currently, it reads what remaining in the bytes buffer or read at most once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #7088:
URL: https://github.com/apache/opendal/pull/7088#discussion_r2640169471
##########
bindings/java/src/jmh/java/org/apache/opendal/bench/jmh/Main.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.opendal.bench.jmh;
+
+import java.util.Collection;
+import org.openjdk.jmh.infra.BenchmarkParams;
+import org.openjdk.jmh.results.RunResult;
+import org.openjdk.jmh.runner.Runner;
+import org.openjdk.jmh.runner.options.CommandLineOptions;
+import org.openjdk.jmh.runner.options.ChainedOptionsBuilder;
+import org.openjdk.jmh.runner.options.Options;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+import org.openjdk.jmh.runner.options.VerboseMode;
Review Comment:
jmh is GPL code. We cannot release source file with this dependency. Typically we have such a bench lib outside or exclude the code from any form of releases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #7088:
URL: https://github.com/apache/opendal/pull/7088#discussion_r2640210149
##########
bindings/java/src/main/java/org/apache/opendal/OperatorInputStream.java:
##########
@@ -58,6 +58,33 @@ public int read() throws IOException {
return -1;
}
+ @Override
+ public int read(byte[] b, int off, int len) throws IOException {
+ if (b == null) {
+ throw new NullPointerException();
+ }
+ if (off < 0 || len < 0 || len > b.length - off) {
+ throw new IndexOutOfBoundsException();
+ }
+ if (len == 0) {
+ return 0;
+ }
+
+ while (bytes != null && offset >= bytes.length) {
+ bytes = readNextBytes(reader.nativeHandle);
+ offset = 0;
+ }
Review Comment:
Weird `while` here.
Perhaps loop over len > 0 and try to copy from bytes until len fulfilled or end.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #7088:
URL: https://github.com/apache/opendal/pull/7088#discussion_r2641904794
##########
bindings/java/src/executor.rs:
##########
@@ -129,7 +129,7 @@ pub(crate) fn make_tokio_executor(env: &mut JNIEnv, cores: usize) -> Result<Exec
move || {
ENV.with(|cell| {
let mut env = vm
- .attach_current_thread_permanently()
+ .attach_current_thread_as_daemon()
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on code in PR #7088:
URL: https://github.com/apache/opendal/pull/7088#discussion_r2640164354
##########
bindings/java/src/executor.rs:
##########
@@ -129,7 +129,7 @@ pub(crate) fn make_tokio_executor(env: &mut JNIEnv, cores: usize) -> Result<Exec
move || {
ENV.with(|cell| {
let mut env = vm
- .attach_current_thread_permanently()
+ .attach_current_thread_as_daemon()
Review Comment:
We may tag @rib here.
Later we can try to migrate to jni-rs 0.22 but there should be some similar issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "rib (via GitHub)" <gi...@apache.org>.
rib commented on code in PR #7088:
URL: https://github.com/apache/opendal/pull/7088#discussion_r2640516654
##########
bindings/java/src/executor.rs:
##########
@@ -129,7 +129,7 @@ pub(crate) fn make_tokio_executor(env: &mut JNIEnv, cores: usize) -> Result<Exec
move || {
ENV.with(|cell| {
let mut env = vm
- .attach_current_thread_permanently()
+ .attach_current_thread_as_daemon()
Review 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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #7088:
URL: https://github.com/apache/opendal/pull/7088#discussion_r2640135327
##########
bindings/java/src/executor.rs:
##########
@@ -129,7 +129,7 @@ pub(crate) fn make_tokio_executor(env: &mut JNIEnv, cores: usize) -> Result<Exec
move || {
ENV.with(|cell| {
let mut env = vm
- .attach_current_thread_permanently()
+ .attach_current_thread_as_daemon()
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #7088:
URL: https://github.com/apache/opendal/pull/7088#discussion_r2640317316
##########
bindings/java/src/jmh/java/org/apache/opendal/bench/jmh/Main.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.opendal.bench.jmh;
+
+import java.util.Collection;
+import org.openjdk.jmh.infra.BenchmarkParams;
+import org.openjdk.jmh.results.RunResult;
+import org.openjdk.jmh.runner.Runner;
+import org.openjdk.jmh.runner.options.CommandLineOptions;
+import org.openjdk.jmh.runner.options.ChainedOptionsBuilder;
+import org.openjdk.jmh.runner.options.Options;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+import org.openjdk.jmh.runner.options.VerboseMode;
Review Comment:
I'm fine with just removing them. They are one-shot benchmarks, and I believe we can add them later when we actually need a proper benchmark suite.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(bindings/java): Speed up the performance of `OperatorInputStream` [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #7088:
URL: https://github.com/apache/opendal/pull/7088
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org