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

2022-03-24 Thread Jorn Vernee
On Thu, 24 Mar 2022 18:35:12 GMT, Jorn Vernee  wrote:

>> Sure, this is problematic - but at the same time I don't think there's a 
>> better way to deal with this? I'd prefer to defer this to a separate issue 
>> (and I think the build team is in a much better position to suggest a better 
>> fix). IIRC we had this problem in the past as well.
>
> I'd suggest at least adding `--enable-preview` as an argument when running 
> benchmarks through the build system in that case. I think this should do the 
> trick:
> 
> 
> diff --git a/make/RunTests.gmk b/make/RunTests.gmk
> index 81540266ec0..9ed45fb02a8 100644
> --- a/make/RunTests.gmk
> +++ b/make/RunTests.gmk
> @@ -583,7 +583,7 @@ define SetupRunMicroTestBody
>$$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS))
> 
># Current tests needs to open java.io
> -  $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED
> +  $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED 
> --enable-preview
> 
># Save output as JSON or CSV file
>ifneq ($$(MICRO_RESULTS_FORMAT), )
> 
> 
> People manually running the benchmarks.jar will have to pass 
> `--enable-preview` still though.

After discussing this offline, it seems that javac no longer poisons the minor 
class file version of every class file, but only of those that use preview 
features. So, my concern is not warranted.

-

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


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

2022-03-24 Thread Maurizio Cimadamore
On Thu, 24 Mar 2022 19:17:40 GMT, Jorn Vernee  wrote:

>> I'd suggest at least adding `--enable-preview` as an argument when running 
>> benchmarks through the build system in that case. I think this should do the 
>> trick:
>> 
>> 
>> diff --git a/make/RunTests.gmk b/make/RunTests.gmk
>> index 81540266ec0..9ed45fb02a8 100644
>> --- a/make/RunTests.gmk
>> +++ b/make/RunTests.gmk
>> @@ -583,7 +583,7 @@ define SetupRunMicroTestBody
>>$$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS))
>> 
>># Current tests needs to open java.io
>> -  $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED
>> +  $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED 
>> --enable-preview
>> 
>># Save output as JSON or CSV file
>>ifneq ($$(MICRO_RESULTS_FORMAT), )
>> 
>> 
>> People manually running the benchmarks.jar will have to pass 
>> `--enable-preview` still though.
>
> After discussing this offline, it seems that javac no longer poisons the 
> minor class file version of every class file, but only of those that use 
> preview features. So, my concern is not warranted.

Turns out this is no longer necessary. As part of the support for preview API, 
javac now only pollutes the classfile if a source file is using preview 
features, as described in this PR:
https://github.com/openjdk/jdk/pull/703

-

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


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

2022-03-24 Thread Jorn Vernee
On Thu, 24 Mar 2022 17:48:23 GMT, Maurizio Cimadamore  
wrote:

>> 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.
>
> Sure, this is problematic - but at the same time I don't think there's a 
> better way to deal with this? I'd prefer to defer this to a separate issue 
> (and I think the build team is in a much better position to suggest a better 
> fix). IIRC we had this problem in the past as well.

I'd suggest at least adding `--enable-preview` as an argument when running 
benchmarks through the build system in that case. I think this should do the 
trick:


diff --git a/make/RunTests.gmk b/make/RunTests.gmk
index 81540266ec0..9ed45fb02a8 100644
--- a/make/RunTests.gmk
+++ b/make/RunTests.gmk
@@ -583,7 +583,7 @@ define SetupRunMicroTestBody
   $$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS))

   # Current tests needs to open java.io
-  $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED
+  $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED 
--enable-preview

   # Save output as JSON or CSV file
   ifneq ($$(MICRO_RESULTS_FORMAT), )


People manually running the benchmarks.jar will have to pass `--enable-preview` 
still though.

-

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


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

2022-03-24 Thread Maurizio Cimadamore
On Thu, 24 Mar 2022 13:00:12 GMT, Jorn Vernee  wrote:

>> 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)
>
> 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?

`mapInternal` in FileChannelImpl takes care of that for both flavors of `map`

-

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


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

2022-03-24 Thread Maurizio Cimadamore
On Thu, 24 Mar 2022 13:10:20 GMT, Jorn Vernee  wrote:

>> 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)
>
> 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.

Sure, this is problematic - but at the same time I don't think there's a better 
way to deal with this? I'd prefer to defer this to a separate issue (and I 
think the build team is in a much better position to suggest a better fix). 
IIRC we had this problem in the past as well.

-

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


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

2022-03-24 Thread Jorn Vernee
On Wed, 23 Mar 2022 14:06:56 GMT, Maurizio Cimadamore  
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:  * {@linkplain MemorySegment#allocateNative(long, long, 
> MemorySession) native memory segments}, backed by off-heap memory;
> 60:  * {@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:  * 
> 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:  * 
> 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); 
> 

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

2022-03-23 Thread Maurizio Cimadamore
> 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)

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/7888/files
  - new: https://git.openjdk.java.net/jdk/pull/7888/files/7ec71f73..c9bc9a70

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=7888=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7888=03-04

  Stats: 12 lines in 2 files changed: 3 ins; 8 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/7888.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888

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