RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]

[Jorn Vernee]

On Wed, 23 Mar 2022 14:06:56 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> This PR contains the API and implementation changes for JEP-424 [1]. A more detailed description of such changes, to avoid repetitions during the review process, is included as a separate comment.
>> 
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Drop redundant javadoc statements re. handling of nulls
>   (handling of nulls is specified once and for all in the package javadoc)

Some more nits.

One potential issue with adding --enable-preview when building benchmarks (last comment of the bunch). 

Other than that, I think this looks good.

make/test/BuildMicrobenchmark.gmk line 97:

> 95:     SRC := $(MICROBENCHMARK_SRC), \
> 96:     BIN := $(MICROBENCHMARK_CLASSES), \
> 97:     JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED --enable-preview, \

It still seems like this would lead to potential issues. i.e. requiring all benchmarks to be run with `--enable-preview`? We ended up adding `--enable-preview` to our benchmarks, but do other benchmarks still work without it? AFAIK the entire benchmarks.jar will have the altered class file version.

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 61:

> 59:  *     <li>{@linkplain MemorySegment#allocateNative(long, long, MemorySession) native memory segments}, backed by off-heap memory;</li>
> 60:  *     <li>{@linkplain FileChannel#map(FileChannel.MapMode, long, long, MemorySession) mapped memory segments}, obtained by mapping
> 61:  * a file into main memory ({@code mmap}); tha contents of a mapped memory segments can be {@linkplain #force() persisted} and

Suggestion:

 * a file into main memory ({@code mmap}); the contents of a mapped memory segments can be {@linkplain #force() persisted} and

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 298:

> 296: 
> 297:     /**
> 298:      * Returns a slice of this memory segment, at given offset. The returned segment's base address is the base address

Saw a similar change in other places, so I'll suggest this here as well.
Suggestion:

     * Returns a slice of this memory segment, at the given offset. The returned segment's base address is the base address

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 311:

> 309: 
> 310:     /**
> 311:      * Returns a slice of this memory segment, at given offset. The returned segment's base address is the base address

Suggestion:

     * Returns a slice of this memory segment, at the given offset. The returned segment's base address is the base address

src/java.base/share/classes/java/lang/foreign/MemorySession.java line 143:

> 141: 
> 142:     /**
> 143:      * {@return the owner thread associated with this memory session (if any)}

Maybe the "if any" here could be more specific. e.g. saying that `null` is returned if the session doesn't have an owner thread.

src/java.base/share/classes/java/lang/foreign/MemorySession.java line 165:

> 163: 
> 164:     /**
> 165:      * Closes this memory session. As a side effect, if this operation completes without exceptions, this session

I'd suggest change this to "As a result of this", since the effects listed are the main reason for closing a session. (it strikes me as strange. If the things listed are side-effects, then what is the main effect of closing a segment?)
Suggestion:

     * Closes this memory session. As a result of this, if this operation completes without exceptions, this session

src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 51:

> 49:  * <p>
> 50:  * Clients can obtain a {@linkplain #loaderLookup() loader lookup},
> 51:  * which can be used to search symbols in libraries loaded by the current classloader (e.g. using {@link System#load(String)},

"search symbols" sounds a bit unnatural to me... I like the wording in the libraryLookup doc more
Suggestion:

 * which can be used to find symbols in libraries loaded by the current classloader (e.g. using {@link System#load(String)},

src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 59:

> 57:  * <p>
> 58:  * Finally, clients can load a library and obtain a {@linkplain #libraryLookup(Path, MemorySession) library lookup} which can be used
> 59:  * to search symbols in that library. A library lookup is associated with a {@linkplain  MemorySession memory session},

Suggestion:

 * to find symbols in that library. A library lookup is associated with a {@linkplain  MemorySession memory session},

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7895:

> 7893:      *     VarHandle handle = MethodHandles.memorySegmentViewVarHandle(ValueLayout.JAVA_INT.withOrder(ByteOrder.BIG_ENDIAN)); //(MemorySegment, long) -> int
> 7894:      *     handle = MethodHandles.insertCoordinates(handle, 1, 4); //(MemorySegment) -> int
> 7895:      * }</pre></blockquote>

These could be snippets.

Also, I think it would be nice to add a link to MemoryLayout.varHandle here.

src/java.base/share/classes/java/nio/channels/FileChannel.java line 975:

> 973:     /**
> 974:      * Maps a region of this channel's file into a new mapped memory segment,
> 975:      * with a given offset, size and memory session.

Suggestion:

     * with the given offset, size and memory session.

src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java line 51:

> 49: 
> 50:     /* A fallback lookup, used when creation of system lookup fails. */
> 51:     private static final Function<String, Optional<MemorySegment>> fallbackLookup = name -> Optional.empty();

Now that we have SymbolLookup again, these Function types could potentially be changed to SymbolLookup again. (and also avoid some churn here)

src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java line 135:

> 133:     }
> 134: 
> 135:     public Optional<MemorySegment> lookup(String name) {

`@Override` here?

src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 1071:

> 1069:         sessionImpl.checkValidStateSlow();
> 1070:         if (offset < 0) throw new IllegalArgumentException("Requested bytes offset must be >= 0.");
> 1071:         if (size < 0) throw new IllegalArgumentException("Requested bytes size must be >= 0.");

The javadoc also says that IAE will be thrown if `offset + size < 0` I think to guard against overflow, but I don't see that checked here. Is it missing?

-------------

PR: https://git.openjdk.java.net/jdk/pull/7888