[java] Changes OrtEnvironment so it can't be closed by users #10670

Craigacp · 2022-02-25T04:17:56Z

Description:
The changes in #10200 enforce that OrtEnv can't be created more than once in a single process. The Java API used to allow this to ensure that users could configure thread pools and logging levels in the environment. This PR changes OrtEnvironment so there can only be a single instance of it for the lifetime of the JVM (well, as much as it can be enforced in Java, it may be possible to unload the OrtEnvironment.class entirely and reload it, resetting the state of its statics and allowing another environment to be created). The environment now registers a JVM shutdown hook which releases the environment as the JVM shuts down. This could cause the environment to be released while a daemon thread which uses ORT is running, as daemon threads concurrently execute with the shutdown hooks, however it's not possible to prevent this from within Java.

This caused a few refactorings in the tests to ensure that the thread pool tests can still run. These refactorings slow down the Java tests slightly as they now need to fork a new JVM for each test class. There might be some implication for the android build system here if it runs the Java tests as part of the build, but I'm not familiar with how those are used.

OrtEnvironment still implements AutoCloseable though the close method is now a no-op. Removing this interface would be a breaking change on users, and we can consider if we want to do that or not.

Much of this PR is visual noise in the tests from the indentation change of moving the environment creation out of a try-with-resources block.

Motivation and Context

Why is this change required? What problem does it solve? Java side parts for Fix race condition in CUDA, ROCm, and TensorRT EP GetKernelRegistry() implementations. #10200.

Craigacp · 2022-02-25T04:21:36Z

Note this PR will conflict with the other two PRs I have open (#10653 and #10318), and so if they are merged before this then it'll need some fixes and vice versa.

yuslepukhin · 2022-02-25T19:14:03Z

java/src/test/java/ai/onnxruntime/InferenceTest.java

-    env.close();
-    assertTrue(env.isClosed());
-  }
+  private static final OrtEnvironment env = OrtEnvironment.getEnvironment();


env = OrtEnvironment.getEnvironment();

Is there a test that checks that repeated calls return the same instance?

No, I've not added an explicit test for that. I can do.

yuslepukhin · 2022-02-25T19:21:16Z

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux CPU x64 NoContribops CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline

yuslepukhin · 2022-02-25T19:21:26Z

/azp run MacOS NoContribops CI Pipeline, Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed

azure-pipelines · 2022-02-25T19:21:55Z

Azure Pipelines successfully started running 7 pipeline(s).

azure-pipelines · 2022-02-25T19:22:06Z

Azure Pipelines successfully started running 7 pipeline(s).

pranavsharma · 2022-02-25T21:27:05Z

FYI see my comment here #10200 (comment). Could close() offer the same functionality that the C API offers? I suppose that would help with the tests of creating the env with different options?

Craigacp · 2022-02-25T22:14:33Z

The close is the problem though, my understanding is that closing the OrtEnv such that its destructor is called causes issues with the changes in #10200 which means that creating a fresh OrtEnv in the same process fails.

We could refcount it (as it was before) but the try-with-resources idiom that is encouraged in Java would mean that it's being closed and reopened multiple times. There's no good way for a user to signal that they are completely done with an object such that it can't be rebuilt ever again in Java, because most of the time that's not a useful thing to do.

yuslepukhin · 2022-02-26T00:52:48Z

/azp run onnxruntime-binary-size-checks-ci-pipeline, onnxruntime-python-checks-ci-pipeline, ONNX Runtime Web CI Pipeline

azure-pipelines · 2022-02-26T00:53:06Z

Azure Pipelines successfully started running 3 pipeline(s).

Craigacp · 2022-02-28T18:31:06Z

Rebased after the merge of #10318.

edgchen1 · 2022-02-28T20:43:11Z

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline

edgchen1 · 2022-02-28T20:43:24Z

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline

azure-pipelines · 2022-02-28T20:43:53Z

Azure Pipelines successfully started running 8 pipeline(s).

azure-pipelines · 2022-02-28T20:44:13Z

Azure Pipelines successfully started running 9 pipeline(s).

pranavsharma

nit: unrelated to the objective of this PR - the check for INSTANCE == null can be avoided in the getEnvironment variants keeping it in only once place.

Craigacp · 2022-02-28T22:31:29Z

nit: unrelated to the objective of this PR - the check for INSTANCE == null can be avoided in the getEnvironment variants keeping it in only once place.

The three checks do different things. The one in getEnvironment() is used to avoid the warning message which is usually printed if you try to change the logging name or level which comes from getEnvironment(OrtLoggingLevel, String). The one in getEnvironment(OrtLoggingLevel, String, ThreadingOptions) is to make it throw an exception because you can't change the thread pool and setting the thread pool incorrectly is (in my judgement, which is open to suggestion) more of a programming error than having the wrong string prepended to all the log messages.

edgchen1 · 2022-02-28T22:49:57Z

/azp run Windows CPU CI Pipeline, Windows GPU CI Pipeline, Windows GPU TensorRT CI Pipeline, Windows WebAssembly CI Pipeline, orttraining-amd-gpu-ci-pipeline, orttraining-linux-ci-pipeline, orttraining-linux-gpu-ci-pipeline, orttraining-ortmodule-distributed, onnxruntime-python-checks-ci-pipeline

edgchen1 · 2022-02-28T22:50:07Z

/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux Nuphar CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline

azure-pipelines · 2022-02-28T22:50:39Z

Azure Pipelines successfully started running 8 pipeline(s).

azure-pipelines · 2022-02-28T22:50:59Z

Azure Pipelines successfully started running 9 pipeline(s).

pranavsharma · 2022-02-28T22:56:23Z

LGTM 👍

yuslepukhin

edgchen1 · 2022-03-01T19:44:18Z

Thanks for the change @Craigacp!

…10670)" This reverts commit f856608.

Craigacp mentioned this pull request Feb 25, 2022

Fix race condition in CUDA, ROCm, and TensorRT EP GetKernelRegistry() implementations. #10200

Merged

yuslepukhin reviewed Feb 25, 2022

View reviewed changes

Changes OrtEnvironment so it can't be closed by users.

b37aee6

Craigacp force-pushed the java-environment-refactor branch from 7c8f09b to b37aee6 Compare February 28, 2022 18:30

pranavsharma previously approved these changes Feb 28, 2022

View reviewed changes

Fix the formatting and add a same instance check.

070e9df

Craigacp dismissed pranavsharma’s stale review via 070e9df February 28, 2022 22:28

yuslepukhin approved these changes Feb 28, 2022

View reviewed changes

pranavsharma approved these changes Feb 28, 2022

View reviewed changes

pranavsharma merged commit f856608 into microsoft:master Mar 1, 2022

This was referenced Mar 1, 2022

[Java] Support configuring CUDA and TensorRT execution providers #10697

Merged

[java] Sparse tensor support #10653

Merged

Craigacp deleted the java-environment-refactor branch March 1, 2022 19:46

pranavsharma added a commit that referenced this pull request Mar 8, 2022

Revert "[java] Changes OrtEnvironment so it can't be closed by users (#…

322d0e4

…10670)" This reverts commit f856608.

pranavsharma mentioned this pull request Mar 8, 2022

Revert "[java] Changes OrtEnvironment so it can't be closed by users" #10808

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[java] Changes OrtEnvironment so it can't be closed by users #10670

[java] Changes OrtEnvironment so it can't be closed by users #10670

Craigacp commented Feb 25, 2022

Craigacp commented Feb 25, 2022

yuslepukhin Feb 25, 2022

Craigacp Feb 25, 2022

yuslepukhin commented Feb 25, 2022

yuslepukhin commented Feb 25, 2022

azure-pipelines bot commented Feb 25, 2022

azure-pipelines bot commented Feb 25, 2022

pranavsharma commented Feb 25, 2022

Craigacp commented Feb 25, 2022 •

edited

Loading

yuslepukhin commented Feb 26, 2022

azure-pipelines bot commented Feb 26, 2022

Craigacp commented Feb 28, 2022

edgchen1 commented Feb 28, 2022

edgchen1 commented Feb 28, 2022

azure-pipelines bot commented Feb 28, 2022

azure-pipelines bot commented Feb 28, 2022

pranavsharma left a comment

Craigacp commented Feb 28, 2022

edgchen1 commented Feb 28, 2022

edgchen1 commented Feb 28, 2022

azure-pipelines bot commented Feb 28, 2022

azure-pipelines bot commented Feb 28, 2022

pranavsharma commented Feb 28, 2022

yuslepukhin left a comment

edgchen1 commented Mar 1, 2022

[java] Changes OrtEnvironment so it can't be closed by users #10670

[java] Changes OrtEnvironment so it can't be closed by users #10670

Conversation

Craigacp commented Feb 25, 2022

Craigacp commented Feb 25, 2022

yuslepukhin Feb 25, 2022

Choose a reason for hiding this comment

Craigacp Feb 25, 2022

Choose a reason for hiding this comment

yuslepukhin commented Feb 25, 2022

yuslepukhin commented Feb 25, 2022

azure-pipelines bot commented Feb 25, 2022

azure-pipelines bot commented Feb 25, 2022

pranavsharma commented Feb 25, 2022

Craigacp commented Feb 25, 2022 • edited Loading

yuslepukhin commented Feb 26, 2022

azure-pipelines bot commented Feb 26, 2022

Craigacp commented Feb 28, 2022

edgchen1 commented Feb 28, 2022

edgchen1 commented Feb 28, 2022

azure-pipelines bot commented Feb 28, 2022

azure-pipelines bot commented Feb 28, 2022

pranavsharma left a comment

Choose a reason for hiding this comment

Craigacp commented Feb 28, 2022

edgchen1 commented Feb 28, 2022

edgchen1 commented Feb 28, 2022

azure-pipelines bot commented Feb 28, 2022

azure-pipelines bot commented Feb 28, 2022

pranavsharma commented Feb 28, 2022

yuslepukhin left a comment

Choose a reason for hiding this comment

edgchen1 commented Mar 1, 2022

Craigacp commented Feb 25, 2022 •

edited

Loading