On Wed, 23 Mar 2022 14:06:56 GMT, Maurizio Cimadamore <mcimadam...@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
