Re: RFR: 8284949: riscv: Add Zero support for the 32-bit RISC-V architecture [v3]
> This patch adds Zero support for the 32-bit RISC-V architecture. > > Additional tests: > > - [x] Linux zero RISCV32 cross-compilation > - [x] Resulting binaries run on QEMU User mode without problems Feilong Jiang has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into rv32-zero - adjust SYS_futex define for RISCV32 - Add support for 32-bit risc-v zero - Changes: - all: https://git.openjdk.java.net/jdk/pull/8284/files - new: https://git.openjdk.java.net/jdk/pull/8284/files/dae310f9..2b6158f8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8284=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8284=01-02 Stats: 7355 lines in 832 files changed: 3683 ins; 1056 del; 2616 mod Patch: https://git.openjdk.java.net/jdk/pull/8284.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8284/head:pull/8284 PR: https://git.openjdk.java.net/jdk/pull/8284
Re: RFR: 8284949: riscv: Add Zero support for the 32-bit RISC-V architecture [v2]
On Tue, 19 Apr 2022 11:22:37 GMT, Thomas Stuefe wrote: > LGTM > > Cheers, Thomas > > P.S. I assume you did not run the whole gamut of jtreg tests, right? Seems to > me there are a number of tests which would need to get adapted for riscv32. Thanks for the review, Thomas. Currently, we only test jtreg tier1 tests under test/hotspot and test/jdk. I assume it would be enough for adding riscv32 zero support? There are some tests adaptions to be done for riscv32 like [1] [2]. I guess it might be better to fix them in another PR. [1] https://github.com/openjdk/jdk/blob/72726c41829b33fd2baf5b3604cab49d39489dd2/test/lib/jdk/test/lib/Platform.java#L200-L202 [2] https://github.com/openjdk/jdk/blob/72726c41829b33fd2baf5b3604cab49d39489dd2/test/lib-test/jdk/test/lib/TestMutuallyExclusivePlatformPredicates.java#L46-L49 - PR: https://git.openjdk.java.net/jdk/pull/8284
Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v5]
> Created a test called NullCallerGetResource to test
> Module::getResourceAsStream and Class::getResourceAsStream from the native
> level.
>
> At the java level the test builds a test module called 'n' which opens the
> package 'open' to everyone. There is also a package 'closed' which is neither
> opened or exported. Both packages have a text file called 'test.txt'. The
> open package has a class called OpenResources and the closed package has a
> class called ClosedResources. The native test is launched with the test
> module n added.
>
> At the native level the test tries to read both the open and closed resource
> from both the classes and the module. It performs the equivalent of the
> following java operations:
>
> Class c = open.OpenResources.fetchClass();
> InputStream in = c.getResourceAsStream("test.txt");
> assert(in != null); in.close();
>
> Module n = c.getModule();
> in = n.getResourceAsStream("open/test.txt");
> assert(in != null); in.close();
>
> Class closed = closed.ClosedResources.fetchClass();
> assert(closedsStream("test.txt") == null);
> assert(n.getResourceAsStream("closed/test.txt") == null);
>
> The test initially threw an NPE when trying to fetch the open resource. The
> Module class was fixed by removing the fragment with the (caller == null)
> test in getResourceAsStream, and changing the call to isOpen(String,Module)
> to use EVERYONE_MODULE if the caller module is null.
Tim Prinzing has updated the pull request incrementally with one additional
commit since the last revision:
some cleanup of the test
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8134/files
- new: https://git.openjdk.java.net/jdk/pull/8134/files/c5fef46b..15de2394
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8134=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8134=03-04
Stats: 62 lines in 4 files changed: 21 ins; 27 del; 14 mod
Patch: https://git.openjdk.java.net/jdk/pull/8134.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8134/head:pull/8134
PR: https://git.openjdk.java.net/jdk/pull/8134
RFR: 8285093: Introduce UTIL_ARG_WITH
Analogous to `UTIL_ARG_ENABLE`, we need a `UTIL_ARG_WITH` which wraps ´AC_ARG_WITH`, provides a declarative rather than programmatic way of handling configure arguments. It can also make sure that all edge cases are covered, and that we treat arguments consistently. This PR contains the implementation of `UTIL_ARG_WITH`, and an example conversation of the calls to `AC_ARG_WITH` in basic_tools.m4. There are some 120-odd more places where `AC_ARG_WITH` is called that need to be converted, but that is out of scope for this PR. Getting the m4 logic for `AC_ARG_WITH` was hard enough for this PR, and verifying a whole bunch of configure arguments is mind-numbingly boring. So I intend to attack those piecewise, fixing them in large enough batches at a time, later on. I also fixed a bug and a documentation issue in `UTIL_ARG_ENABLE`. - Commit messages: - Use new default value in basic_tools.m4 - Better default for RESULT. - Fix UTIL_ARG_WITH in basic_tools.m4 - Create UTIL_ARG_WITH Changes: https://git.openjdk.java.net/jdk/pull/8306/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8306=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285093 Stats: 382 lines in 2 files changed: 349 ins; 5 del; 28 mod Patch: https://git.openjdk.java.net/jdk/pull/8306.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8306/head:pull/8306 PR: https://git.openjdk.java.net/jdk/pull/8306
Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables [v3]
> This patch examines and optimizes `Wrapper` lookups. > > First wrote a few simple microbenchmarks to verify there are actual speedups > from using perfect hash tables in `sun.invoke.util.Wrapper` compared to > simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ a > speed-up for the case of `char` -> `Wrapper`, but not when mapping from > `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case > didn't use the `FROM_CHAR` table for some reason, which is remedied. > > Micros show benefits across the board for warmed up case: > > > Baseline, OOTB > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op > Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op > Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op > Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op > > Patch, OOTB > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op > Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op > Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op > Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op > > > For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when > spinning up of MHs) there are decent or even great wins in all cases but > `forPrimitiveType` - which was changed from a simple switch to use the hash > lookup. Since the interpreter penalty is small in absolute terms and the win > on JITed code is significant this seems like a reasonable trade-off: > > > Baseline, -Xint > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op > Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op > Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op > Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op > > Patch, -Xint > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op > Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op > Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op > Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op Claes Redestad has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'wrappers' of https://github.com/cl4es/jdk into wrappers - Copyrights - Changes: - all: https://git.openjdk.java.net/jdk/pull/8242/files - new: https://git.openjdk.java.net/jdk/pull/8242/files/97eec9d3..792c37e2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8242=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8242=01-02 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8242.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8242/head:pull/8242 PR: https://git.openjdk.java.net/jdk/pull/8242
Integrated: 8284880: Re-examine sun.invoke.util.Wrapper hash tables
On Thu, 14 Apr 2022 11:19:04 GMT, Claes Redestad wrote: > This patch examines and optimizes `Wrapper` lookups. > > First wrote a few simple microbenchmarks to verify there are actual speedups > from using perfect hash tables in `sun.invoke.util.Wrapper` compared to > simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ a > speed-up for the case of `char` -> `Wrapper`, but not when mapping from > `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case > didn't use the `FROM_CHAR` table for some reason, which is remedied. > > Micros show benefits across the board for warmed up case: > > > Baseline, OOTB > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op > Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op > Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op > Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op > > Patch, OOTB > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op > Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op > Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op > Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op > > > For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when > spinning up of MHs) there are decent or even great wins in all cases but > `forPrimitiveType` - which was changed from a simple switch to use the hash > lookup. Since the interpreter penalty is small in absolute terms and the win > on JITed code is significant this seems like a reasonable trade-off: > > > Baseline, -Xint > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op > Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op > Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op > Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op > > Patch, -Xint > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op > Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op > Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op > Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op This pull request has now been integrated. Changeset: 5df8bd6b Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/5df8bd6b4e15686aa7d72b3f5a977eb51b0befc3 Stats: 259 lines in 3 files changed: 158 ins; 59 del; 42 mod 8284880: Re-examine sun.invoke.util.Wrapper hash tables Reviewed-by: erikj, mchung - PR: https://git.openjdk.java.net/jdk/pull/8242
Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables [v2]
On Thu, 14 Apr 2022 13:59:57 GMT, Claes Redestad wrote: >> This patch examines and optimizes `Wrapper` lookups. >> >> First wrote a few simple microbenchmarks to verify there are actual speedups >> from using perfect hash tables in `sun.invoke.util.Wrapper` compared to >> simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ >> a speed-up for the case of `char` -> `Wrapper`, but not when mapping from >> `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case >> didn't use the `FROM_CHAR` table for some reason, which is remedied. >> >> Micros show benefits across the board for warmed up case: >> >> >> Baseline, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op >> Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op >> Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op >> Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op >> >> Patch, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op >> Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op >> Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op >> Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op >> >> >> For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when >> spinning up of MHs) there are decent or even great wins in all cases but >> `forPrimitiveType` - which was changed from a simple switch to use the hash >> lookup. Since the interpreter penalty is small in absolute terms and the win >> on JITed code is significant this seems like a reasonable trade-off: >> >> >> Baseline, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op >> Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op >> Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op >> Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op >> >> Patch, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op >> Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op >> Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op >> Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add line break in make/test/BuildMicrobenchmark.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> Thanks for reviews! - PR: https://git.openjdk.java.net/jdk/pull/8242
Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v4]
On Mon, 11 Apr 2022 00:48:34 GMT, Tim Prinzing wrote:
>> Created a test called NullCallerGetResource to test
>> Module::getResourceAsStream and Class::getResourceAsStream from the native
>> level.
>>
>> At the java level the test builds a test module called 'n' which opens the
>> package 'open' to everyone. There is also a package 'closed' which is
>> neither opened or exported. Both packages have a text file called
>> 'test.txt'. The open package has a class called OpenResources and the closed
>> package has a class called ClosedResources. The native test is launched with
>> the test module n added.
>>
>> At the native level the test tries to read both the open and closed resource
>> from both the classes and the module. It performs the equivalent of the
>> following java operations:
>>
>> Class c = open.OpenResources.fetchClass();
>> InputStream in = c.getResourceAsStream("test.txt");
>> assert(in != null); in.close();
>>
>> Module n = c.getModule();
>> in = n.getResourceAsStream("open/test.txt");
>> assert(in != null); in.close();
>>
>> Class closed = closed.ClosedResources.fetchClass();
>> assert(closedsStream("test.txt") == null);
>> assert(n.getResourceAsStream("closed/test.txt") == null);
>>
>> The test initially threw an NPE when trying to fetch the open resource. The
>> Module class was fixed by removing the fragment with the (caller == null)
>> test in getResourceAsStream, and changing the call to isOpen(String,Module)
>> to use EVERYONE_MODULE if the caller module is null.
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> requested changes
Marked as reviewed by mchung (Reviewer).
test/jdk/java/lang/module/exeNullCallerGetResource/exeNullCallerGetResource.c
line 95:
> 93: jclass class_OpenResources = (*env)->FindClass(env,
> "open/OpenResources");
> 94: assert(class_OpenResources != NULL);
> 95: jmethodID mid_OpenResources_fetchClass =
> (*env)->GetStaticMethodID(env, class_OpenResources, "fetchClass",
> "()Ljava/lang/Class;" );
It seems that invoking `fetchClass` isn't necessary since you can simply use
`class_OpenResources`. Similarly for `class_ClosedResources`
test/jdk/java/lang/module/exeNullCallerGetResource/exeNullCallerGetResource.c
line 183:
> 181: exit(-1);
> 182: }
> 183: assert(in == NULL);
assert is typically used for sanity test. As part of the test validation,
e.g. in this case `in == NULL` or `in != NULL` in line 157, it may be clearer
if it's an explicit check and throw exception to indicate test failure
especially in case `#undef NDEBUG` may not be set in the test.
-
PR: https://git.openjdk.java.net/jdk/pull/8134
Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables [v2]
On Thu, 14 Apr 2022 13:59:57 GMT, Claes Redestad wrote: >> This patch examines and optimizes `Wrapper` lookups. >> >> First wrote a few simple microbenchmarks to verify there are actual speedups >> from using perfect hash tables in `sun.invoke.util.Wrapper` compared to >> simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ >> a speed-up for the case of `char` -> `Wrapper`, but not when mapping from >> `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case >> didn't use the `FROM_CHAR` table for some reason, which is remedied. >> >> Micros show benefits across the board for warmed up case: >> >> >> Baseline, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op >> Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op >> Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op >> Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op >> >> Patch, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op >> Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op >> Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op >> Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op >> >> >> For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when >> spinning up of MHs) there are decent or even great wins in all cases but >> `forPrimitiveType` - which was changed from a simple switch to use the hash >> lookup. Since the interpreter penalty is small in absolute terms and the win >> on JITed code is significant this seems like a reasonable trade-off: >> >> >> Baseline, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op >> Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op >> Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op >> Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op >> >> Patch, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op >> Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op >> Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op >> Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add line break in make/test/BuildMicrobenchmark.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> Looks good.The copyright header end-year needs update before you push. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8242
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
On Tue, 19 Apr 2022 15:54:02 GMT, Michael McMahon wrote:
>> test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 44:
>>
>>> 42: StandardProtocolFamily fam = args[0].equals("ipv4") ? INET :
>>> INET6;
>>> 43: System.out.println("Family = " + fam);
>>> 44: testDatagramChannel(args, fam);
>>
>> Shouldn't there be a testcase for when DatagramChannel is opened using the
>> no arg factory method `DatagramChannel.open()`?
>
> I'm not sure there is value in testing all of these permutations.
> Distinguishing DatagramChannel and DatagramSocket probably made sense, but
> it's all the same implementation under the hood.
1. `DatagramChannel.open()` => opens a dual socket unless
`-Djava.net.preferIPv4Stack=true`, in which case it should be equivalent to
`DatagramChannel.open(StandardProtocolFamily.INET)`
2. `DatagramChannel.open(StandardProtocolFamily.INET)` => opens an IPv4 socket
3. `DatagramChannel.open(StandardProtocolFamily.INET6)` => opens an IPv6 socket
So I believe it makes sense to test the no-arg constructor since that's the
only way to open a dual socket.
-
PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v6]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: typo in windows native code - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/509c3f81..1e08ee9a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=04-05 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
On Tue, 19 Apr 2022 14:50:57 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> fix whitespace
>
> src/jdk.net/windows/native/libextnet/WindowsSocketOptions.c line 112:
>
>> 110: return optval;
>> 111: }
>> 112: handleError(env, rv, "get option IP_DONTFRAGMENT failed");
>
> Is there some indentation issue here?
Yes, there is.
> test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 44:
>
>> 42: StandardProtocolFamily fam = args[0].equals("ipv4") ? INET :
>> INET6;
>> 43: System.out.println("Family = " + fam);
>> 44: testDatagramChannel(args, fam);
>
> Shouldn't there be a testcase for when DatagramChannel is opened using the no
> arg factory method `DatagramChannel.open()`?
I'm not sure there is value in testing all of these permutations.
Distinguishing DatagramChannel and DatagramSocket probably made sense, but it's
all the same implementation under the hood.
-
PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
On Tue, 19 Apr 2022 14:47:01 GMT, Michael McMahon wrote:
>> Hi,
>>
>> Could I get the following PR review please? It adds a new JDK specific
>> extended socket option
>> called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4
>> and IPv6
>> UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF
>> (Dont Fragment) bit
>> in the IP header. There is no equivalent in the IPv6 packet header as
>> fragmentation is implemented
>> exclusively by the sending and receiving nodes.
>>
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix whitespace
src/jdk.net/windows/native/libextnet/WindowsSocketOptions.c line 112:
> 110: return optval;
> 111: }
> 112: handleError(env, rv, "get option IP_DONTFRAGMENT failed");
Is there some indentation issue here?
test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 44:
> 42: StandardProtocolFamily fam = args[0].equals("ipv4") ? INET :
> INET6;
> 43: System.out.println("Family = " + fam);
> 44: testDatagramChannel(args, fam);
Shouldn't there be a testcase for when DatagramChannel is opened using the no
arg factory method `DatagramChannel.open()`?
test/jdk/jdk/net/ExtendedSocketOption/DontFragmentTest.java line 47:
> 45: try (DatagramSocket c = new DatagramSocket()) {
> 46: testDatagramSocket(c);
> 47: }
Can't you test `MulticastSocket` in exactly the same way? Why is there a
specific test method for `MulticastSocket`?
-
PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v5]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: fix whitespace - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/5f1d87eb..509c3f81 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284890: Support for Do not fragment IP socket options [v4]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: updates - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/e90aa7c3..5f1d87eb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=02-03 Stats: 42 lines in 2 files changed: 13 ins; 10 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Integrated: 8284539: Configure --with-source-date=version fails on MacOS
On Thu, 14 Apr 2022 16:13:59 GMT, Andrew Leonard wrote: > JDK-8282769 added support for more ISO-8601 formats, but remove handling of > just a date "-MM-DD" being present, which is the case for a configure > using --with-source-date=version which uses the date string from > version-numbers.conf. > Also, the first date parse had an invalid format string "%FZ %TZ", with too > many Zs. > This PR corrects the first date parse to parse a standard ISO-8601 Zulu > date: "%FT%TZ" > Then it adds the final check for no time being specified. > > Signed-off-by: Andrew Leonard This pull request has now been integrated. Changeset: da3d8b1d Author:Andrew Leonard URL: https://git.openjdk.java.net/jdk/commit/da3d8b1d1ea132e670d5629af3e98d958f2b56f7 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod 8284539: Configure --with-source-date=version fails on MacOS Reviewed-by: erikj, ihse - PR: https://git.openjdk.java.net/jdk/pull/8247
Integrated: 8284999: Remove remaining files in src/samples
On Tue, 19 Apr 2022 10:41:07 GMT, Magnus Ihse Bursie wrote: > JEP 298 was about removing demos and samples. Unfortunately, > [JDK-8173801](https://bugs.openjdk.java.net/browse/JDK-8173801) which should > have removed all files in src/samples, left a few non-source files (key > stores and images). These should be removed. This pull request has now been integrated. Changeset: 595c8b85 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/595c8b859890b5b439069a5aac6664b96b444580 Stats: 0 lines in 10 files changed: 0 ins; 0 del; 0 mod 8284999: Remove remaining files in src/samples Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/8294
Re: RFR: 8284999: Remove remaining files in src/samples
On Tue, 19 Apr 2022 10:41:07 GMT, Magnus Ihse Bursie wrote: > JEP 298 was about removing demos and samples. Unfortunately, > [JDK-8173801](https://bugs.openjdk.java.net/browse/JDK-8173801) which should > have removed all files in src/samples, left a few non-source files (key > stores and images). These should be removed. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8294
Re: RFR: 8284890: Support for Do not fragment IP socket options [v3]
> Hi, > > Could I get the following PR review please? It adds a new JDK specific > extended socket option > called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 > and IPv6 > UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF > (Dont Fragment) bit > in the IP header. There is no equivalent in the IPv6 packet header as > fragmentation is implemented > exclusively by the sending and receiving nodes. > > Thanks, > Michael Michael McMahon has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 25 additional commits since the last revision: - windows update - test update - Merge branch 'master' into mtu - builds in github action now - fix whitespace errors - minor spec update - windows 2016 issue - windows issue - windows update - test update - ... and 15 more: https://git.openjdk.java.net/jdk/compare/3b3bec01...e90aa7c3 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8245/files - new: https://git.openjdk.java.net/jdk/pull/8245/files/14c776b1..e90aa7c3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8245=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8245=01-02 Stats: 148852 lines in 1314 files changed: 108219 ins; 7158 del; 33475 mod Patch: https://git.openjdk.java.net/jdk/pull/8245.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8245/head:pull/8245 PR: https://git.openjdk.java.net/jdk/pull/8245
Re: Why we use specific compiler versions - was: Re: JDK-8284772 - was RE: [jdk17] RFR: 8269148: Update minor GCC version in GitHub Actions pipeline
On 2022-04-14 19:42, Andrew Hughes wrote: On 12:57 Wed 13 Apr , Magnus Ihse Bursie wrote: I disagree completely. We had it this way in mainline originally, but it was fixed in https://bugs.openjdk.java.net/browse/JDK-8256393. Prior to this patch, it seems there were no GCC version requirements. That's not what I'm suggesting. What I'm suggesting is that we replace gcc-10=10.2.0-5ubuntu1~20.04 (or whatever it is now) with just gcc-10. Is it possible to set the requirement to 10.2? That'd be okay for me, since it matches how we check for gcc versions in configure (currently it is at least "6.0"), and how we have traditionally described our requirements. Or are the only two options "just major version" or "major, minor, patch and ubuntu release"? :( /Magnus That still selects a specific major version of GCC. It is between major versions that we see new optimisations introduced, deprecations, etc. I would certainly not suggest that we allow it to be switched from e.g. GCC 10 to G11 behind our backs. I have dealt with such transitions in Fedora and know the changes they bring. What I don't see the advantage of is requiring a very specific package version. If this was OpenJDK, it would be like asking to build 8u with specifically 8u332-b05 rather than just 8u. I can't see that we would do that without a very good reason. I don't see such a good reason for requiring the same of GCC. These two are handled differently on the GCC development side too. Breakage is expected with the move to a new major version. This is why three stable versions are currently being maintained [0]. If breakage were to happen between minor releases of a stable version, however, that would be a bug. I've yet to see a case of it happening, though of course it's possible. As you might know, I'm not too fond of the GHA solution, since we can't debug issues with Github's hosts. Nevertheless, many users look at the GHA results as a way to sanity check their code. Any and all spurious build failures is a problem then, since it will present a red marker -- even if the new code in the PR is okay. This specific versioning is producing precisely these spurious failures. The reason I started digging into this was because my PR failed on Linux x86_64. There were no code changes in the PR (I was backporting the GHA setup to our Shenandoah fork of 8u). Having only just added support to 8u mainline, I found this very odd. It turns out it failed because it could no longer download the specific version of GCC. [1] I agree GHA can be painful to debug - it took me weeks to get 8u working in full - but it is useful for testing on architectures and operating systems one doesn't have easy access to. The less control we have over the build platform, the greater the chance for odd and non-reproducible build failures. Selecting a specific version of the compiler is a way to guarantee reproducible build results. If the build succeeds in mainline, and I submit correct code, chances are higher that the build also succeeds in my PR. In contrast, if the gcc version suddenly were changed behind my back, the mainline build might succeed, and my PR build fail, not due to anything I've done wrong, but just due to the fact that the compiler was switched by the Ubuntu team in the meantime. Yes, it means a bit more annoyance when upgrading the compiler, but that also means it is a conscious (and hopefully well tested) choice. I'll take that any day over re-introducing more uncertainty into an already-unstable testing procedure. As I say, I'm not suggesting we don't select a specific version, just that we are not *too* specific to the point we are constantly changing the version specification every time the Ubuntu maintainer fixes a typo. /Magnus [0] https://gcc.gnu.org/ [1] https://github.com/openjdk-bots/shenandoah-jdk8u/runs/5889665932?check_suite_focus=true Thanks
Re: RFR: 8284890: Support for Do not fragment IP socket options [v2]
On Fri, 15 Apr 2022 11:49:29 GMT, Michael McMahon wrote: >> Hi, >> >> Could I get the following PR review please? It adds a new JDK specific >> extended socket option >> called IP_DONTFRAGMENT, which disables IP packet fragmentation in both IPv4 >> and IPv6 >> UDP sockets (NIO DatagramChannels). For IPv4 in particular, it sets the DF >> (Dont Fragment) bit >> in the IP header. There is no equivalent in the IPv6 packet header as >> fragmentation is implemented >> exclusively by the sending and receiving nodes. >> >> Thanks, >> Michael > > Michael McMahon has updated the pull request incrementally with one > additional commit since the last revision: > > builds in github action now Build changes look OK. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8245
Re: RFR: 8284539: Configure --with-source-date=version fails on MacOS
On Thu, 14 Apr 2022 16:13:59 GMT, Andrew Leonard wrote: > JDK-8282769 added support for more ISO-8601 formats, but remove handling of > just a date "-MM-DD" being present, which is the case for a configure > using --with-source-date=version which uses the date string from > version-numbers.conf. > Also, the first date parse had an invalid format string "%FZ %TZ", with too > many Zs. > This PR corrects the first date parse to parse a standard ISO-8601 Zulu > date: "%FT%TZ" > Then it adds the final check for no time being specified. > > Signed-off-by: Andrew Leonard Marked as reviewed by ihse (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8247
Re: RFR: 8284949: riscv: Add Zero support for the 32-bit RISC-V architecture [v2]
On Tue, 19 Apr 2022 08:47:18 GMT, Feilong Jiang wrote: >> This patch adds Zero support for the 32-bit RISC-V architecture. >> >> Additional tests: >> >> - [x] Linux zero RISCV32 cross-compilation >> - [x] Resulting binaries run on QEMU User mode without problems > > Feilong Jiang has updated the pull request incrementally with one additional > commit since the last revision: > > adjust SYS_futex define for RISCV32 Marked as reviewed by ihse (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8284
Re: RFR: 8284949: riscv: Add Zero support for the 32-bit RISC-V architecture [v2]
On Tue, 19 Apr 2022 08:47:18 GMT, Feilong Jiang wrote: >> This patch adds Zero support for the 32-bit RISC-V architecture. >> >> Additional tests: >> >> - [x] Linux zero RISCV32 cross-compilation >> - [x] Resulting binaries run on QEMU User mode without problems > > Feilong Jiang has updated the pull request incrementally with one additional > commit since the last revision: > > adjust SYS_futex define for RISCV32 LGTM Cheers, Thomas P.S. I assume you did not run the whole gamut of jtreg tests, right? Seems to me there are a number of tests which would need to get adapted for riscv32. - Marked as reviewed by stuefe (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8284
RFR: 8284999: Remove remaining files in src/samples
JEP 298 was about removing demos and samples. Unfortunately, [JDK-8173801](https://bugs.openjdk.java.net/browse/JDK-8173801) which should have removed all files in src/samples, left a few non-source files (key stores and images). These should be removed. - Commit messages: - 8284999: Remove remaining files in src/samples Changes: https://git.openjdk.java.net/jdk/pull/8294/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8294=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284999 Stats: 0 lines in 10 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8294.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8294/head:pull/8294 PR: https://git.openjdk.java.net/jdk/pull/8294
Re: RFR: 8284949: riscv: Add Zero support for the 32-bit RISC-V architecture [v2]
> This patch adds Zero support for the 32-bit RISC-V architecture. > > Additional tests: > > - [x] Linux zero RISCV32 cross-compilation > - [x] Resulting binaries run on QEMU User mode without problems Feilong Jiang has updated the pull request incrementally with one additional commit since the last revision: adjust SYS_futex define for RISCV32 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8284/files - new: https://git.openjdk.java.net/jdk/pull/8284/files/a26633b2..dae310f9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8284=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8284=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8284.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8284/head:pull/8284 PR: https://git.openjdk.java.net/jdk/pull/8284
Re: RFR: 8284949: riscv: Add Zero support for the 32-bit RISC-V architecture [v2]
On Mon, 18 Apr 2022 15:27:07 GMT, Yadong Wang wrote: >> Feilong Jiang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> adjust SYS_futex define for RISCV32 > > src/hotspot/os/linux/waitBarrier_linux.cpp line 44: > >> 42: #ifndef SYS_futex >> 43: #if defined(RISCV32) && defined(SYS_futex_time64) >> 44: #define SYS_futex SYS_futex_time64 > > Should it be better to check SYS_futex and SYS_futex_time64 when RISCV32 > defined? Fixed. - PR: https://git.openjdk.java.net/jdk/pull/8284
Re: RFR: 8284539: Configure --with-source-date=version fails on MacOS
On Fri, 15 Apr 2022 12:32:50 GMT, Erik Joelsson wrote: >> JDK-8282769 added support for more ISO-8601 formats, but remove handling of >> just a date "-MM-DD" being present, which is the case for a configure >> using --with-source-date=version which uses the date string from >> version-numbers.conf. >> Also, the first date parse had an invalid format string "%FZ %TZ", with too >> many Zs. >> This PR corrects the first date parse to parse a standard ISO-8601 Zulu >> date: "%FT%TZ" >> Then it adds the final check for no time being specified. >> >> Signed-off-by: Andrew Leonard > > make/autoconf/util.m4 line 243: > >> 241: # BSD date >> 242: # ISO-8601 date in Zulu 'date'T'time'Z >> 243: timestamp=$($DATE -u -j -f "%FT%TZ" "$2" "+%s" 2> /dev/null) > > You are removing the space between FT and TZ, I'm just curious why and if > that is significant. > EDIT: Never mind me, this looks good. yes, this is supposed to be parsing an ISO8601 eg.2022-02-09T14:47:36Z, the previous JDK-8282769 had an error in this string - PR: https://git.openjdk.java.net/jdk/pull/8247
Re: RFR: 8284539: Configure --with-source-date=version fails on MacOS
On Thu, 14 Apr 2022 16:13:59 GMT, Andrew Leonard wrote: > JDK-8282769 added support for more ISO-8601 formats, but remove handling of > just a date "-MM-DD" being present, which is the case for a configure > using --with-source-date=version which uses the date string from > version-numbers.conf. > Also, the first date parse had an invalid format string "%FZ %TZ", with too > many Zs. > This PR corrects the first date parse to parse a standard ISO-8601 Zulu > date: "%FT%TZ" > Then it adds the final check for no time being specified. > > Signed-off-by: Andrew Leonard @magicus let me know when you're good for this one? and i'll then integrate - PR: https://git.openjdk.java.net/jdk/pull/8247
