Migrate to Java 21 FFM API #241

frankgrimes97 · 2024-11-18T17:17:49Z

The project's minimium java release requirements moved from Java 17 to Java 21.

Package imports changed from jdk.incubator.foreign -> java.lang.foreign and a number of API changes were required.
Notably, the new java.lang.foreign.Arena class needs to be exposed when allocating native memory regions as it controls the scope and lifecycle of allocations.

Respective API docs are as follows:

frankgrimes97 · 2024-11-18T17:18:59Z

context: apache/datasketches-java#577 (comment)

leerho · 2024-11-18T20:27:53Z

Thank you for your work on this. A couple of things:

The GitHub/Actions/Workflows will need to be updated to Java 21. Otherwise, guaranteed to fail.
I am not familiar with NetBeans IDE, but the comments in the nb-configurations.xml file are concerning:
- "The configuration is intended to be shared among all the users of project and
  therefore it is assumed to be part of version control checkout." -- I presume this only applies to all NetBeans users only??
- "Without this configuration present, some functionality in the IDE may be limited or fail altogether."
  - We don't allow IDE specific configuration files to be part of the version control or part of the released project. Our developers use a number of different IDEs, and they keep their own IDE configuration files as part of their own development environment. So we need to figure out how we can do this without this config file.
  - The warning seems severe to me, since all of the changes you have made are nothing but pure java.

frankgrimes97 · 2024-11-18T21:00:19Z

The GitHub/Actions/Workflows will need to be updated to Java 21. Otherwise, guaranteed to fail.

I can look at adding another commit on this PR to address the GitHub Actions/Workflows changes required once I familiarize myself with them.

-- I presume this only applies to all NetBeans users only??

Yes, that's the case... I see now that I should have simply added it to the .gitignore file for consistency for how these kinds of files are currently handled for Eclipse and IntelliJ:

# Eclipse project files 
.classpath
.project
.settings/
.checkstyle

# IntelliJ project files
*.idea
*.iml
*.ipr
*.iws

# Netbeans project files
nb-configurations.xml

I can amend the PR to do so.
FYI, I also added nb-configurations.xml to the apache-rat-plugin's exclusions in the pom.xml to avoid it complaining with:

Too many files with unapproved license

I can remove that as well since when building locally I could just skip the spurious license check failure with the right local mvn build arguments.

The project's minimum Java release requirements moved from Java 17 to Java 21. Package imports changed from `jdk.incubator.foreign` -> `java.lang.foreign` and a number of API changes were required. Notably, the new `java.lang.foreign.Arena` class needs to be exposed when allocating native memory regions as it controls the scope and lifecycle of allocations. Respective API docs are as follows: - https://docs.oracle.com/en/java/javase/17/docs/api/jdk.incubator.foreign/jdk/incubator/foreign/package-summary.html - https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/foreign/package-summary.html

frankgrimes97 · 2024-11-18T21:13:03Z

@leerho FYI, nb-configurations.xml has now been removed and the GitHub Action Workflow files have been updated to also now use Java 21 instead of Java 17.

leerho · 2024-11-18T22:51:53Z

Hmm. Some sort of failure on Windows having to do with file access?? I haven't used Windows for over 16 years, so I'm not sure what that is about. All the other OSs passed! Oddly, the 4.1.0 version, Which I assume you based this on, worked with Windows.

leerho · 2024-11-19T01:10:18Z

src/main/java/org/apache/datasketches/memory/Memory.java

-   * @param scope the given ResourceScope.
-   * It must be non-null.
-   * Typically use <i>ResourceScope.newConfinedScope()</i>.
-   * Warning: specifying a <i>newSharedScope()</i> is not supported.


In cases where the user supplies the Arena I suggest we add in the javadoc something to the effect:

This Memory component is not thread-safe as it was designed to be used in environments where threading and parallel operation are handled at higher levels. Specifying global(), ofAuto() or ofShared() Arenas could produce unpredictable results

If Arena.ofConfined() is the only one which makes sense and is applicable here, then perhaps the Arena shouldn't be exposed in the public API at all?

leerho · 2024-11-19T01:22:17Z

src/main/java/org/apache/datasketches/memory/WritableMemory.java

  }

  /**
-   * Maps the specified portion of the given file into Memory for write operations.
-   * @param file the given file to map. It must be non-null and writable.
+   * Maps the specified portion of the given file into Memory for write operations with a ResourceScope.


I think you meant Arena, but that isn't correct either. The write operations of WritableMemory assume that the MemorySegment exists and has already been allocated using an Arena. (I think I made that mistake too.)

leerho · 2024-11-19T01:24:16Z

src/main/java/org/apache/datasketches/memory/WritableMemory.java

+   * @param byteOrder the byte order to be used.  It must be non-null.
+   * @return mapped WritableMemory.
+   * @throws IllegalArgumentException -- if file is not readable or writable.
+   * @throws IllegalArgumentException -- if file is not writable.


Good Catch!

leerho · 2024-11-19T01:35:15Z

src/main/java/org/apache/datasketches/memory/WritableMemory.java

-   * @param scope the given ResourceScope.
-   * It must be non-null.
-   * Typically use <i>ResourceScope.newConfinedScope()</i>.
-   * Warning: specifying a <i>newSharedScope()</i> is not supported.


Similar warning as above.

leerho

Wow! This represents a huge amount of work on your part! Thank you!
I also recognize some improvements you made along the way!

I don't know if I caught all the issues, many of which were related to comments.

This is a huge PR and when I merge I will be merging into a dedicated branch, which will make it easier for both of us to see and find some things and run tests as we go. I do suspect this will take several passes to review and get right. --cheers!

leerho · 2024-11-19T01:42:23Z

src/main/java/org/apache/datasketches/memory/MemoryRequestServer.java

  }

  /**
   * Request new WritableMemory with the given newCapacityBytes. The current Writable Memory can be used to
   * determine the byte order of the returned WritableMemory and other properties.
   * @param currentWritableMemory the current writableMemory of the client. It must be non-null.
   * @param newCapacityBytes The capacity being requested. It must be &gt; the capacity of the currentWritableMemory.
-   * @param scope the ResourceScope to be used for the newly allocated memory.
-   * It must be non-null.
+   * @param arena the Arena to be used for the newly allocated memory. It must be non-null.
   * Typically use <i>ResourceScope.newConfinedScope()</i>.
   * Warning: specifying a <i>newSharedScope()</i> is not supported.


Add warning as above. Also replace ResourceScope

leerho · 2024-11-19T01:56:43Z

src/main/java/org/apache/datasketches/memory/WritableMemory.java

@@ -194,12 +193,13 @@ static WritableMemory allocateDirect(
   * @return WritableMemory


Add warning as above, also ResourceScope needs to be replaced at about L186

leerho · 2024-11-19T01:59:37Z

src/main/java/org/apache/datasketches/memory/WritableMemory.java

      long capacityBytes,
      long alignmentBytes,
-      ResourceScope scope,
+      Scope scope,


I don't think we need and Arena and a MemorySegment.Scope.

frankgrimes97 · 2024-11-19T13:54:05Z

Hmm. Some sort of failure on Windows having to do with file access?? I haven't used Windows for over 16 years, so I'm not sure what that is about. All the other OSs passed! Oddly, the 4.1.0 version, Which I assume you based this on, worked with Windows.

Interesting...
FWIW, I have access to a fully up-to-date Windows 11 machine at home and it compiles and runs the test suite without errors with the latest Amazon Corretto 21 release.

My guess is that the default directory on new File("FileName") changed from Java 17 to Java 21 and that path is not fully accessible on the Windows Action Runners provided by GitHub.
I added a new commit to use File.createTempFile instead which I expect will behave correctly.

This is to try to address file permission errors seen in some GitHub Action Workflow runs on Windows: "[INFO] Error: Failures: Error: LeafImplTest.checkMapLeafs:118 » AccessDenied TestFile2.bin Error: SpecificLeafTest.checkMapLeafs:108 » AccessDenied TestFile2.bin"

leerho · 2024-11-20T01:11:10Z

Now it is getting an Apache Rat failure on the file: force_original.txt

leerho · 2024-11-20T01:12:57Z

If this is a temp file we can either make sure it is deleted before you run the rat test, if possible, or put it in the excludes list.

This is to try to address following errors seen in some GitHub Action Workflow runs on Windows: "Warning: Files with unapproved licenses: force_original.txt"

leerho · 2024-11-20T17:01:34Z

Excellent! It now passes all tests. Good work!
I want to go through it again before I merge.

leerho · 2024-11-20T19:46:17Z

There are a couple of things I want to look at once I get this into a branch where it will be easier to me look at and use other tools to search for consistency, etc.

leerho · 2024-12-07T00:11:45Z

@frankgrimes97,
Thank you again for your contribution!
I have worked on it some more and migrated much of the Java 17 FFM API to use Java21 FFM API. Also did some cleanup and code consolidation, and updated the POM, and GH action workflows.
I would welcome you adding your review to the PR#242.
(Note that I found a bug in Java 21.0.2 release and had to upgrade to Java 21.0.5. See the README.)

frankgrimes97 force-pushed the feature/java-21-ffm branch from 65edda9 to fac061c Compare November 18, 2024 21:07

frankgrimes97 added 2 commits November 18, 2024 16:11

Update GHA workflows to Java 21

56bc3f3

frankgrimes97 force-pushed the feature/java-21-ffm branch from fac061c to 56bc3f3 Compare November 18, 2024 21:12

leerho changed the base branch from main to frank_grimes_java-21-ffm November 19, 2024 00:23

leerho reviewed Nov 19, 2024

View reviewed changes

leerho requested changes Nov 19, 2024

View reviewed changes

frankgrimes97 added 2 commits November 19, 2024 09:03

Minor cleanup based on PR feedback

44d31d7

Improve temp file creation in test suite

98a0ac8

This is to try to address following errors seen in some GitHub Action Workflow runs on Windows: "Warning: Files with unapproved licenses: force_original.txt"

leerho approved these changes Nov 20, 2024

View reviewed changes

leerho merged commit da1dd62 into apache:frank_grimes_java-21-ffm Nov 20, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to Java 21 FFM API #241

Migrate to Java 21 FFM API #241

frankgrimes97 commented Nov 18, 2024

frankgrimes97 commented Nov 18, 2024

leerho commented Nov 18, 2024

frankgrimes97 commented Nov 18, 2024

frankgrimes97 commented Nov 18, 2024

leerho commented Nov 18, 2024 •

edited

Loading

leerho Nov 19, 2024 •

edited

Loading

frankgrimes97 Nov 20, 2024

leerho Nov 19, 2024 •

edited

Loading

leerho Nov 19, 2024

leerho Nov 19, 2024

leerho left a comment

leerho Nov 19, 2024

leerho Nov 19, 2024

leerho Nov 19, 2024

frankgrimes97 commented Nov 19, 2024 •

edited

Loading

leerho commented Nov 20, 2024

leerho commented Nov 20, 2024 •

edited

Loading

leerho commented Nov 20, 2024

leerho commented Nov 20, 2024

leerho commented Dec 7, 2024

		@@ -194,12 +193,13 @@ static WritableMemory allocateDirect(
		* @return WritableMemory

Migrate to Java 21 FFM API #241

Migrate to Java 21 FFM API #241

Conversation

frankgrimes97 commented Nov 18, 2024

frankgrimes97 commented Nov 18, 2024

leerho commented Nov 18, 2024

frankgrimes97 commented Nov 18, 2024

frankgrimes97 commented Nov 18, 2024

leerho commented Nov 18, 2024 • edited Loading

leerho Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

frankgrimes97 Nov 20, 2024

Choose a reason for hiding this comment

leerho Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

leerho Nov 19, 2024

Choose a reason for hiding this comment

leerho Nov 19, 2024

Choose a reason for hiding this comment

leerho left a comment

Choose a reason for hiding this comment

leerho Nov 19, 2024

Choose a reason for hiding this comment

leerho Nov 19, 2024

Choose a reason for hiding this comment

leerho Nov 19, 2024

Choose a reason for hiding this comment

frankgrimes97 commented Nov 19, 2024 • edited Loading

leerho commented Nov 20, 2024

leerho commented Nov 20, 2024 • edited Loading

leerho commented Nov 20, 2024

leerho commented Nov 20, 2024

leerho commented Dec 7, 2024

leerho commented Nov 18, 2024 •

edited

Loading

leerho Nov 19, 2024 •

edited

Loading

leerho Nov 19, 2024 •

edited

Loading

frankgrimes97 commented Nov 19, 2024 •

edited

Loading

leerho commented Nov 20, 2024 •

edited

Loading