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

ARROW-17604: [Docs][Java] Make it more obvious that --add-opens is required #14066

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

lidavidm
Copy link
Member

@lidavidm lidavidm commented Sep 7, 2022

  • Improve docs about this

  • Improve error message about this

    java.lang.RuntimeException: Failed to initialize MemoryUtil. Was Java started with `--add-opens=java.base/java.nio=ALL-UNNAMED`? (See https://arrow.apache.org/docs/java/install.html)
        at org.apache.arrow.memory.util.MemoryUtil.<clinit>(MemoryUtil.java:138)
        at org.apache.arrow.memory.DefaultAllocationManagerFactory.<clinit>(DefaultAllocationManagerFactory.java:31)
    

<id>opens-tests</id>
<!-- Run tests WITHOUT add-opens to make sure we fail-fast -->
<activation>
<jdk>[9,]</jdk>
Copy link
Member Author

Choose a reason for hiding this comment

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

This may only apply from Java 16+/JEP396: https://openjdk.org/jeps/396

@lidavidm
Copy link
Member Author

lidavidm commented Sep 7, 2022

JEP 424 may improve this: https://openjdk.org/jeps/424

The new API lets us optionally use the garbage collector for managing memory:

All of the examples above use non-deterministic deallocation: The memory associated with the allocated segments is deallocated by the garbage collector after the memory segment instance becomes unreachable. We say that such segments are implicitly deallocated.

…while still offering precise control:

Memory segments support deterministic deallocation through memory sessions. A memory session models the lifecycle of one or more memory segments. A newly-created memory session is in the alive state, which means that all the segments it manages can be accessed safely. At the client's request a memory session can be closed so that access to the segments managed by the session is no longer allowed. The MemorySession class implements the AutoCloseable interface so that memory sessions work with the try-with-resources statement

This is very similar to our concept of a BufferAllocator.

JEP 424 would also let us implement the C Data Interface without JNI:

…there are situations where clients might only have a MemoryAddress instance, as is often the case when interacting with native code…the client can unsafely turn the address into a segment via the MemorySegment::ofAddress factory. This factory attaches fresh spatial and temporal bounds to an otherwise raw memory address in order to allow dereference operations.

In the very long term, it feels like it could replace much of the implementation of the memory module. In the near term, it may let us provide an allocator implementation that does not require JVM flags on sufficiently recent versions of Java, if we can figure out how to abstract between sun.misc.Unsafe and the new APIs appropriately and without penalty.

There are a few important caveats:

  • The API is a preview and cannot be used without flags yet. It will be a long time before we can use it at all, and a very very long time before we can use it exclusively.

  • As a replacement for sun.misc.Unsafe and the JNI code in the C Data Interface, it will still require JVM flags, or else every use of the API will incur a warning, making it pointless:

    …the use of unsafe methods in the FFM API is restricted: Their use is permitted but, by default, every such use causes a warning to be issued at run time. To allow code in a module M to use unsafe methods without warnings, specify the --enable-native-access=M option on the java command line…In a future release, it is likely that this option will be required in order to use unsafe methods.

So my conclusion is that JEP 424 will eventually be useful, but we will always be dependent on setting special JVM flags that are not normally needed, and so it is not a fundamental user experience improvement.

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

@lidavidm
Copy link
Member Author

lidavidm commented Sep 7, 2022

@github-actions crossbow submit java

@lidavidm lidavidm marked this pull request as ready for review September 7, 2022 22:14
@github-actions
Copy link

github-actions bot commented Sep 7, 2022

Revision: c73a3b9

Submitted crossbow builds: ursacomputing/crossbow @ actions-963cb1f0ad

Task Status
java-jars Github Actions
verify-rc-source-java-linux-almalinux-8-amd64 Github Actions
verify-rc-source-java-linux-conda-latest-amd64 Github Actions
verify-rc-source-java-linux-ubuntu-18.04-amd64 Github Actions
verify-rc-source-java-linux-ubuntu-20.04-amd64 Github Actions
verify-rc-source-java-linux-ubuntu-22.04-amd64 Github Actions
verify-rc-source-java-macos-amd64 Github Actions

</goals>
<configuration>
<!-- Dummy value to stop inheriting the default add-opens flag -->
<argLine>-Dfoo=bar</argLine>
Copy link
Contributor

Choose a reason for hiding this comment

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

I could suggest that this line add more arg configuration instead of reset inheriting .values...

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean parent project configure maven-surefire-plugin to add <argLine>--add-opens=java.base/java.nio=ALL-UNNAMED</argLine>, if the plan is to TestOpens.java fail will be needed to reset Surefire parent configuration

When you run the TestOpens test it's throw the exception?

<id>opens-tests</id>
<!-- Run tests WITHOUT add-opens to make sure we fail-fast -->
<activation>
<jdk>[16,]</jdk>
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to get the TestOpens exception with JRE18 but if finished without that error, Is there some additional configuration needed? Probably mvn -Dtest="TestOpens" clean test not thrown that exception because for test this inherit parent configuration.

[ERROR] Failures: 
[ERROR]   TestOpens.testMemoryUtilFailsLoudly:32 Expected java.lang.Throwable to be thrown, but nothing was thrown.

Copy link
Member Author

Choose a reason for hiding this comment

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

It works with just mvn test. I presume specifying the test name manually is causing it to bypass this configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, that was the case

Copy link
Contributor

@davisusanibar davisusanibar left a comment

Choose a reason for hiding this comment

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

LGTM, than you

@lidavidm lidavidm merged commit 0310add into apache:master Sep 22, 2022
@lidavidm lidavidm deleted the arrow-17604 branch September 22, 2022 16:49
@ursabot
Copy link

ursabot commented Sep 23, 2022

Benchmark runs are scheduled for baseline = 5256e43 and contender = 0310add. 0310add is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.78% ⬆️0.07%] test-mac-arm
[Failed ⬇️0.27% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.14% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 0310add1 ec2-t3-xlarge-us-east-2
[Finished] 0310add1 test-mac-arm
[Failed] 0310add1 ursa-i9-9960x
[Finished] 0310add1 ursa-thinkcentre-m75q
[Finished] 5256e434 ec2-t3-xlarge-us-east-2
[Failed] 5256e434 test-mac-arm
[Failed] 5256e434 ursa-i9-9960x
[Finished] 5256e434 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
…quired (apache#14066)

- Improve docs about this
- Improve error message about this

  ```
  java.lang.RuntimeException: Failed to initialize MemoryUtil. Was Java started with `--add-opens=java.base/java.nio=ALL-UNNAMED`? (See https://arrow.apache.org/docs/java/install.html)
	  at org.apache.arrow.memory.util.MemoryUtil.<clinit>(MemoryUtil.java:138)
	  at org.apache.arrow.memory.DefaultAllocationManagerFactory.<clinit>(DefaultAllocationManagerFactory.java:31)
  ```

Authored-by: David Li <[email protected]>
Signed-off-by: David Li <[email protected]>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
…quired (apache#14066)

- Improve docs about this
- Improve error message about this

  ```
  java.lang.RuntimeException: Failed to initialize MemoryUtil. Was Java started with `--add-opens=java.base/java.nio=ALL-UNNAMED`? (See https://arrow.apache.org/docs/java/install.html)
	  at org.apache.arrow.memory.util.MemoryUtil.<clinit>(MemoryUtil.java:138)
	  at org.apache.arrow.memory.DefaultAllocationManagerFactory.<clinit>(DefaultAllocationManagerFactory.java:31)
  ```

Authored-by: David Li <[email protected]>
Signed-off-by: David Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants