Re: [PATCH] Docfix for Unsafe regarding fence methods

[Hans Boehm]

On Fri, Aug 28, 2020 at 1:21 PM Ian Graves  wrote:
>
> Looks like the patch attachment was stripped. I’ll include it inline:
> —Ian
>
> ...
> @@ -3458,6 +3462,12 @@
>  /**
>   * Ensures that stores before the fence will not be reordered with
>   * stores after the fence.
> + *
> + * This method is operationally equivalent to {@link #storeFence()}
> + * as most hardware instructions that provide a StoreStore barrier
> + * provide a LoadStore barrier for free.
> + *
> + * @since 9
>   */
>  public final void storeStoreFence() {
>  storeFence();
>
>
This seems a little misleading, since ARM has "dmb ishst", which orders
only stores?

Re: [PATCH] Docfix for Unsafe regarding fence methods

[Ian Graves]

Looks like the patch attachment was stripped. I’ll include it inline:
—Ian


diff -r 3e9d813ff918 src/java.base/share/classes/jdk/internal/misc/Unsafe.java
--- a/src/java.base/share/classes/jdk/internal/misc/Unsafe.java Thu Aug 27 
20:20:39 2020 +0200
+++ b/src/java.base/share/classes/jdk/internal/misc/Unsafe.java Fri Aug 28 
15:21:01 2020 -0500
@@ -3412,9 +3412,8 @@
  * Corresponds to C11 atomic_thread_fence(memory_order_acquire)
  * (an "acquire fence").
  *
- * A pure LoadLoad fence is not provided, since the addition of LoadStore
- * is almost always desired, and most current hardware instructions that
- * provide a LoadLoad barrier also provide a LoadStore barrier for free.
+ * Provides a LoadLoad barrier followed by a LoadStore barrier.
+ *
  * @since 1.8
  */
 @HotSpotIntrinsicCandidate
@@ -3427,9 +3426,8 @@
  * Corresponds to C11 atomic_thread_fence(memory_order_release)
  * (a "release fence").
  *
- * A pure StoreStore fence is not provided, since the addition of LoadStore
- * is almost always desired, and most current hardware instructions that
- * provide a StoreStore barrier also provide a LoadStore barrier for free.
+ * Provides a StoreStore barrier followed by a LoadStore barrier.
+ *
  * @since 1.8
  */
 @HotSpotIntrinsicCandidate
@@ -3450,6 +3448,12 @@
 /**
  * Ensures that loads before the fence will not be reordered with
  * loads after the fence.
+ *
+ * This method is operationally equivalent to {@link #loadFence()}
+ * as most hardware instructions that provide a LoadLoad barrier
+ * provide a LoadStore barrier for free.
+ *
+ * @since 9
  */
 public final void loadLoadFence() {
 loadFence();
@@ -3458,6 +3462,12 @@
 /**
  * Ensures that stores before the fence will not be reordered with
  * stores after the fence.
+ *
+ * This method is operationally equivalent to {@link #storeFence()}
+ * as most hardware instructions that provide a StoreStore barrier
+ * provide a LoadStore barrier for free.
+ *
+ * @since 9
  */
 public final void storeStoreFence() {
 storeFence();


> On Aug 28, 2020, at 3:12 PM, Ian Graves  wrote:
> 
> Hello,
> 
> When reviewing some of the documentation in jdk.internal.misc.Unsafe, it 
> appears that there is some dated information regarding the availability of 
> “pure barrier” methods (i.e. pure StoreStoreFence sans a LoadStore barrier). 
> When the storeFence() and loadFence() methods were implemented the pure 
> variations (loadLoadFence(), storeStoreFence()) were not implemented and the 
> documentation indicated as much. This is no longer the case. 
> 
> I’ve included a patch with this email that updates the docs to reflect the 
> new state of affairs here as well as indicate some implementation details as 
> to how the pure variants utilize the impure original methods.
> 
> Thanks!
> 
> Ian
> 

[PATCH] Docfix for Unsafe regarding fence methods

[Ian Graves]

Hello,

When reviewing some of the documentation in jdk.internal.misc.Unsafe, it 
appears that there is some dated information regarding the availability of 
“pure barrier” methods (i.e. pure StoreStoreFence sans a LoadStore barrier). 
When the storeFence() and loadFence() methods were implemented the pure 
variations (loadLoadFence(), storeStoreFence()) were not implemented and the 
documentation indicated as much. This is no longer the case. 

I’ve included a patch with this email that updates the docs to reflect the new 
state of affairs here as well as indicate some implementation details as to how 
the pure variants utilize the impure original methods.

Thanks!

Ian

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Hai-May Chao]

JarSigner.java #953: The output debug message can be removed from the code.
JavaUtilZipFileAccess.java #44: Change posixPerms to extraAttrs.
ZipFile.java #661: Suggest to keep the comment and update it with the 
additional 4 bits for symlink.

The rest of code changes and CSR look good.

Thanks,
Hai-May


> On Aug 28, 2020, at 7:17 AM, Seán Coffey  wrote:
> 
> I've been poking around the zip internals and am now able to locate the 16 
> bits of interest. The position of these actual bits does appear to move 
> around from one test run to another. For now, I guess it's sufficient to look 
> for the pattern of interest in the signed zip file. New testcase added.
> 
> http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/ 
> 
> regards,
> Sean.
> 
> On 27/08/2020 15:58, Weijun Wang wrote:
>>> Looks like it was a conscious design decision to only allow recording of 
>>> POSIX permission bits for this field (& 0xFFF). I don't see anything about 
>>> symlink support in zipfs docs.
>> As long as that *byte* is there and it’s not difficult to locate, we can 
>> manually add the *bit* for symlink and see if jarsigner can keep it.
>> 
>> —Max
>> 

RFR: 8249694 - [TestBug] java/lang/StringBuffer/HugeCapacity.java and j/l/StringBuilder/HugeCapacity.java tests shouldn't be @ignore-d

[Fernando Guallini]

Hi, 

May I please get reviews and a sponsor for this trivial change: 

webrev: http://cr.openjdk.java.net/~fguallini/8249694/webrev.00/ 
Testbug: https://bugs.openjdk.java.net/browse/JDK-8249694 

Tests do not need to have ‘@ignore' because with @requires os.maxMemory is 
enough to ensure they will not be executed if memory requirements are not 
satisfied. They run in Mach5 with no issues. 

Thanks 

-Fernando

Re: RFR(M): 8248188: [PATCH] Add HotSpotIntrinsicCandidate and API for Base64 decoding

[Roger Riggs]

Hi Corey,

A few comments on core-libs side...

The naming convention for methods that end in '0' is usually to indicate
they are the bottom-most method or a native method.
So I think you can/should rename the methods to make the most sense
as to their function.

Comparing with the way that the Base64 encoder was intrinsified, the
method that is intrinsified should have a method body that does
the same function, so it is interchangable.  That likely will just shift
the "fast path" code into the decodeBlock method.
Keeping the symmetry between encoder and decoder will
make it easier to maintain the code.

Given intrinsic only handles 2 of the three cases, and the java code handles
all three, I would add an extra arg to decodeBlock to reflect the isMime 
case

and have the intrinsic take an early exit until it was implemented.


It is unfortunate that taking advantage of vectorization has to be hand 
coded.

If/when the Vector API is ready (JEP 338 https://openjdk.java.net/jeps/338)
the java code should be replaced to use the Vector API and then it would
work for a new hardware without specific coding for each platform.
"Just" implement the Vector API.  There's a lot more bang for the buck
going for that approach.

Thanks, Roger


On 8/24/20 9:21 PM, Corey Ashford wrote:
Here's a revised webrev which includes a JMH benchmark for the decode 
operation.


http://cr.openjdk.java.net/~mhorie/8248188/webrev.03/

The added benchmark tries to be "fair" in that it doesn't prefer a 
large buffer size, which would favor the intrinsic.  It 
pseudo-randomly (but reproducibly) chooses a buffer size between 8 and 
20k+8 bytes, and fills it with random data to encode and decode.  As 
part of the TearDown of an invocation, it also checks the decoded 
output data for correctness.


Example runs on the Power9-based machine I use for development shows a 
3X average improvement across these random buffer sizes. Here's an 
excerpt of the output when run with -XX:-UseBASE64Intrinsics :


Iteration   1: 70795.623 ops/s
Iteration   2: 71070.607 ops/s
Iteration   3: 70867.544 ops/s
Iteration   4: 71107.992 ops/s
Iteration   5: 71048.281 ops/s

And here's the output with the intrinsic enabled:

Iteration   1: 208794.022 ops/s
Iteration   2: 208630.904 ops/s
Iteration   3: 208238.822 ops/s
Iteration   4: 208714.967 ops/s
Iteration   5: 209060.894 ops/s

Taking the best of the two runs: 209060/71048 = 2.94

From other experiments where the benchmark uses a fixed-size, larger 
buffer, the performance ratio rises to about 4.0.


Power10 should have a slightly higher ratio due to several factors, 
but I have not yet benchmarked on Power10.


Other arches ought to be able to do at least this well, if not better, 
because of wider vector registers (> 128 bits) being available.  Only 
a Power9/10 implementation is included in this webrev, however.


Regards,

- Corey


On 8/19/20 11:20 AM, Roger Riggs wrote:

Hi Corey,

For changes obviously performance motivated, it is conventional to 
run a JMH perf test to demonstate

the improvement and prove it is worthwhile to add code complexity.

I don't see any existing Base64 JMH tests but they would be in the 
repo below or near:

 test/micro/org/openjdk/bench/java/util/

Please contribute a JMH test and results to show the difference.

Regards, Roger



On 8/19/20 2:10 PM, Corey Ashford wrote:
Michihiro Horie posted up a new iteration of this webrev for me.  
This time the webrev includes a complete implementation of the 
intrinsic for Power9 and Power10.


You can find it here: 
http://cr.openjdk.java.net/~mhorie/8248188/webrev.02/


Changes in webrev.02 vs. webrev.01:

  * The method header for the intrinsic in the Base64 code has been 
rewritten using the Javadoc style.  The clarity of the comments has 
been improved and some verbosity has been removed. There are no 
additional functional changes to Base64.java.


  * The code needed to martial and check the intrinsic parameters 
has been added, using the base64 encodeBlock intrinsic as a guideline.


  * A complete intrinsic implementation for Power9 and Power10 is 
included.


  * Adds some Power9 and Power10 assembler instructions needed by 
the intrinsic which hadn't been defined before.


The intrinsic implementation in this patch accelerates the decoding 
of large blocks of base64 data by a factor of about 3.5X on Power9.


I'm attaching two Java test cases I am using for testing and 
benchmarking.  The TestBase64_VB encodes and decodes randomly-sized 
buffers of random data and checks that original data matches the 
encoded-then-decoded data.  TestBase64Errors encodes a 48K block of 
random bytes, then corrupts each byte of the encoded data, one at a 
time, checking to see if the decoder catches the illegal byte.


Any comments/suggestions would be appreciated.

Thanks,

- Corey

On 7/27/20 6:49 PM, Corey Ashford wrote:
Michihiro Horie uploaded a new revision of the Base64 decodeBlock 
intrinsic API for me:


http://cr.openjdk.java

Re: RFR 8252248: __SIGRTMAX is not declared in musl libc

[Thomas Stüfe]

Looks good to me. Thanks for checking.

Cheers, Thomas

On Fri, Aug 28, 2020 at 1:25 PM Alexander Scherbatiy <
alexander.scherba...@bell-sw.com> wrote:

> Hello,
>
> Could you review the updated fix:
>http://cr.openjdk.java.net/~alexsch/8252248/webrev.01/
>
>   - moving shared code to net_util_md.h is avoided
>   - INTERRUPT_SIGNAL is defined as SIGRTMAX - 2 instead of __SIGRTMAX -
> 2 for Linux in NativeThread.c
>   - "#include " is moved out from "#ifdef" in NativeThread.c
>   - sigWakeup is changed to "#define WAKEUP_SIGNAL (SIGRTMAX - 2)" in
> linux_close.c
>
> At least test/jdk/java/net/Socket/asyncClose/AsyncClose.java touches all
> four methods where the signal changes are used:
> NativeThread.c
> - Java_sun_nio_ch_NativeThread_init(JNIEnv *env, jclass cl)
> - Java_sun_nio_ch_NativeThread_signal(JNIEnv *env, jclass cl, jlong thread)
> linux_close.c
> -  __attribute((constructor)) init()
> - closefd(int fd1, int fd2)
>
> I run test/jdk/java/net tests on Ubuntu 18.04.4 and Alpine Linux 3.11.6
> with musl libc 1.1.24.
> There are 10 failed tests but they fail without the fix as well. I
> believe that it is because of some network settings of  my machine.
>
>java/net/MulticastSocket/IPMulticastIF.java
>java/net/MulticastSocket/NoSetNetworkInterface.java
>java/net/MulticastSocket/Promiscuous.java
>java/net/MulticastSocket/PromiscuousIPv6.java
>java/net/MulticastSocket/SetGetNetworkInterfaceTest.java
>java/net/MulticastSocket/SetLoopbackMode.java
>java/net/MulticastSocket/SetOutgoingIf.java
>java/net/MulticastSocket/Test.java
>java/net/NetworkInterface/NetworkInterfaceRetrievalTests.java
>java/net/ipv6tests/UdpTest.java
>
> Thanks,
> Alexander.
>
> On 26.08.2020 13:42, Alan Bateman wrote:
> > On 25/08/2020 18:00, Alexander Scherbatiy wrote:
> >>
> >> Hello,
> >>
> >> Could your review the fix for the issue:
> >>   Bug: https://bugs.openjdk.java.net/browse/JDK-8252248
> >>   Fix: http://cr.openjdk.java.net/~alexsch/8252248/webrev.00/
> >>
> >> :
> >>
> >> The fix has been discussed on the portola-dev alias [2] where it was
> >> pointed out that the fix can be reviewed in the mainline and it was
> >> suggested to rename the INTERRUPT_SIGNAL and move its definition to
> >> net_util_md.h.
> >>
> > The xxx_close.c sources are for the old SocketImpl and
> > DatagramSocketImpl implementations; they are aren't used by default
> > and will eventually go away. Maybe an option for the musl port is to
> > just not compile in the old socket implementations? It might be best
> > to start a discussion on nio-dev and net-dev about this as we
> > shouldn't be changing NativeThread.c (which is used for signalling in
> > the FileChannel implementation) to include net_util.h.
> >
> > -Alan
>

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Weijun Wang]

Everything looks fine to me.

I’ve added myself as the CSR reviewer. In the Solution section, it probably 
should me “The existing warning introduced in JDK-8218021 is expanded to 
include symlink”. It is not a new warning.

Thanks,
Max

> On Aug 28, 2020, at 12:05 PM, Seán Coffey  wrote:
> 
> 
> On 28/08/2020 16:18, Weijun Wang wrote:
>> 1. Add a comment on how to generate ZIPBYTES in the test. Not the byte[] 
>> declaration but how the original ZIP file is generated.
> I'll add a comment block to the test:
>> /*
>>  * Created using the createByteArray utility method.
>>  * The zipfile itself was created via this example:
>>  * $ ls -l z
>>  * lrwxrwxrwx 1 test test 4 Aug 27 18:33 z -> ../z
>>  * $ zip -ry test.zip z
>>  */
> 
>> 
>> 2. Does this require a CSR? The POSIX permission one had one.
> 
> Fair point. I've logged one, just to be safe.
> 
> regards,
> Sean.
> 
>> 
>> Thanks,
>> Max
>> 
>>> On Aug 28, 2020, at 10:17 AM, Seán Coffey  wrote:
>>> 
>>> I've been poking around the zip internals and am now able to locate the 16 
>>> bits of interest. The position of these actual bits does appear to move 
>>> around from one test run to another. For now, I guess it's sufficient to 
>>> look for the pattern of interest in the signed zip file. New testcase added.
>>> 
>>> http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/
>>> 
>>> regards,
>>> Sean.
>>> 
>>> On 27/08/2020 15:58, Weijun Wang wrote:
> Looks like it was a conscious design decision to only allow recording of 
> POSIX permission bits for this field (& 0xFFF). I don't see anything 
> about symlink support in zipfs docs.
> 
 As long as that *byte* is there and it’s not difficult to locate, we can 
 manually add the *bit*
  for symlink and see if jarsigner can keep it.
 
 —Max
 
 

Re: RFR 8251989: Hex encoder and decoder utility

[Raffaello Giulietti]

Hi Roger,

I'm reading this version

http://cr.openjdk.java.net/~rriggs/webrev-hex-formatter-8251989-1/src/java.base/share/classes/java/util/Hex.java



On 2020-08-28 16:22, Roger Riggs wrote:

Hi Raffaello,

Missed sending this yesterday, the changes are in the updated webrev.

On 8/23/20 10:42 AM, Raffaello Giulietti wrote:

...

(3) The expressions preLen > origLen - sufLen on L.492 and sufLen >
origLen - preLen on L.496 are equivalent. The latter is thus useless on
L.496 because it is always false when execution reaches here.

true; updated to optimize the case of empty prefix and/or suffix.





While the lines are now numbered L.552 and L.556, this note still holds. 
In other words, L.556-557

if (sufLen > origLen - preLen ||
!suffix.contentEquals(cs.subSequence(origLen - sufLen, origLen))) {

could be replaced with the simpler
if (!suffix.contentEquals(cs.subSequence(origLen - sufLen, origLen))) {




(7) On L.561, delimiter.isEmpty() could be removed as checkDelimiter()
is not invoked when the delimiter is empty. But it's perhaps safer to
have it.

It could be an assert, but asserts aren't enabled always


It could be an explicit throw new AssertionError(...), but I'm not sure 
it's worth the change.




Another quick note.
Complementing toHex(int value), what about exposing the additional

public char toHighHex(int value) {
return  (char)digits[value >> 4 & 0xf];
}

It would useful both to replace the various ">> 4" shifts in the code 
itself and externally if one isn't willing to pay for the allocation of 
the String returned by toHexPair() and prefers to deal with the two 
individual chars.





Thanks  for the comments,

Roger



Has been a pleasure


Greetings
Raffaello

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

On 28/08/2020 16:18, Weijun Wang wrote:

1. Add a comment on how to generate ZIPBYTES in the test. Not the byte[] 
declaration but how the original ZIP file is generated.

I'll add a comment block to the test:

    /*
 * Created using the createByteArray utility method.
 * The zipfile itself was created via this example:
 * $ ls -l z
 * lrwxrwxrwx 1 test test 4 Aug 27 18:33 z -> ../z
 * $ zip -ry test.zip z
 */




2. Does this require a CSR? The POSIX permission one had one.


Fair point. I've logged one, just to be safe.

regards,
Sean.



Thanks,
Max


On Aug 28, 2020, at 10:17 AM, Seán Coffey  wrote:

I've been poking around the zip internals and am now able to locate the 16 bits 
of interest. The position of these actual bits does appear to move around from 
one test run to another. For now, I guess it's sufficient to look for the 
pattern of interest in the signed zip file. New testcase added.

http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/

regards,
Sean.

On 27/08/2020 15:58, Weijun Wang wrote:

Looks like it was a conscious design decision to only allow recording of POSIX 
permission bits for this field (& 0xFFF). I don't see anything about symlink 
support in zipfs docs.


As long as that *byte* is there and it’s not difficult to locate, we can 
manually add the *bit*
  for symlink and see if jarsigner can keep it.

—Max

Re: RFR 8252248: __SIGRTMAX is not declared in musl libc

[Vyom Tiwari]

+1
Vyom

On Fri, Aug 28, 2020 at 8:24 PM Alexander Scherbatiy <
alexander.scherba...@bell-sw.com> wrote:

> On 28.08.2020 17:40, Alan Bateman wrote:
>
> > On 28/08/2020 15:32, Alexander Scherbatiy wrote:
> >>
> >>   I run the java/nio/channels tests with the fix. There are five
> >> failed tests that fail with and without the fix:
> >> java/nio/channels/DatagramChannel/AdaptorMulticasting.java
> >> java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java
> >> java/nio/channels/DatagramChannel/PromiscuousIPv6.java
> >> java/nio/channels/DatagramChannel/Loopback.java
> >> java/nio/channels/FileChannel/FileExtensionAndMap.java
> >>
> >> The FileExtensionAndMap.java has clear fail message: "Error. Test
> >> ignored: This test has huge disk space requirements".
> > The DatagramChannel failures are probably because of firewall or
> > iptables issues too.  The FileExtensionAndMap test has @ignore so it
> > will be skipped, maybe you are running jtreg directly and aren't use
> > -ignore:quiet?
>Yes, I run the jtreg tests directly without the -ignore:quiet option.
>
>Thanks,
>Alexander.
> >
> > In any case, I think the changes look okay.
> >
> > -Alan
>


-- 
Thanks,
Vyom

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Weijun Wang]

1. Add a comment on how to generate ZIPBYTES in the test. Not the byte[] 
declaration but how the original ZIP file is generated.

2. Does this require a CSR? The POSIX permission one had one.

Thanks,
Max

> On Aug 28, 2020, at 10:17 AM, Seán Coffey  wrote:
> 
> I've been poking around the zip internals and am now able to locate the 16 
> bits of interest. The position of these actual bits does appear to move 
> around from one test run to another. For now, I guess it's sufficient to look 
> for the pattern of interest in the signed zip file. New testcase added.
> 
> http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/
> 
> regards,
> Sean.
> 
> On 27/08/2020 15:58, Weijun Wang wrote:
>>> Looks like it was a conscious design decision to only allow recording of 
>>> POSIX permission bits for this field (& 0xFFF). I don't see anything about 
>>> symlink support in zipfs docs.
>>> 
>> As long as that *byte* is there and it’s not difficult to locate, we can 
>> manually add the *bit*
>>  for symlink and see if jarsigner can keep it.
>> 
>> —Max
>> 
>> 

Re: RFR 8252248: __SIGRTMAX is not declared in musl libc

[Alexander Scherbatiy]

On 28.08.2020 17:40, Alan Bateman wrote:


On 28/08/2020 15:32, Alexander Scherbatiy wrote:


  I run the java/nio/channels tests with the fix. There are five 
failed tests that fail with and without the fix:

    java/nio/channels/DatagramChannel/AdaptorMulticasting.java
java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java
    java/nio/channels/DatagramChannel/PromiscuousIPv6.java
    java/nio/channels/DatagramChannel/Loopback.java
    java/nio/channels/FileChannel/FileExtensionAndMap.java

The FileExtensionAndMap.java has clear fail message: "Error. Test 
ignored: This test has huge disk space requirements".
The DatagramChannel failures are probably because of firewall or 
iptables issues too.  The FileExtensionAndMap test has @ignore so it 
will be skipped, maybe you are running jtreg directly and aren't use 
-ignore:quiet?

  Yes, I run the jtreg tests directly without the -ignore:quiet option.

  Thanks,
  Alexander.


In any case, I think the changes look okay.

-Alan

Re: RFR 8252248: __SIGRTMAX is not declared in musl libc

[Alan Bateman]

On 28/08/2020 15:32, Alexander Scherbatiy wrote:


  I run the java/nio/channels tests with the fix. There are five 
failed tests that fail with and without the fix:

    java/nio/channels/DatagramChannel/AdaptorMulticasting.java
java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java
    java/nio/channels/DatagramChannel/PromiscuousIPv6.java
    java/nio/channels/DatagramChannel/Loopback.java
    java/nio/channels/FileChannel/FileExtensionAndMap.java

The FileExtensionAndMap.java has clear fail message: "Error. Test 
ignored: This test has huge disk space requirements".
The DatagramChannel failures are probably because of firewall or 
iptables issues too.  The FileExtensionAndMap test has @ignore so it 
will be skipped, maybe you are running jtreg directly and aren't use 
-ignore:quiet?


In any case, I think the changes look okay.

-Alan

Re: RFR 8252248: __SIGRTMAX is not declared in musl libc

[Alexander Scherbatiy]

On 28.08.2020 15:55, Alan Bateman wrote:


On 28/08/2020 12:25, Alexander Scherbatiy wrote:

Hello,

Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/8252248/webrev.01/

 - moving shared code to net_util_md.h is avoided
 - INTERRUPT_SIGNAL is defined as SIGRTMAX - 2 instead of __SIGRTMAX 
- 2 for Linux in NativeThread.c

 - "#include " is moved out from "#ifdef" in NativeThread.c
 - sigWakeup is changed to "#define WAKEUP_SIGNAL (SIGRTMAX - 2)" in 
linux_close.c


At least test/jdk/java/net/Socket/asyncClose/AsyncClose.java touches 
all four methods where the signal changes are used:

NativeThread.c
- Java_sun_nio_ch_NativeThread_init(JNIEnv *env, jclass cl)
- Java_sun_nio_ch_NativeThread_signal(JNIEnv *env, jclass cl, jlong 
thread)

linux_close.c
-  __attribute((constructor)) init()
- closefd(int fd1, int fd2)
NativeThread current/signal will be exercised by the tests in 
java/nio/channels.


  I run the java/nio/channels tests with the fix. There are five failed 
tests that fail with and without the fix:

    java/nio/channels/DatagramChannel/AdaptorMulticasting.java
    java/nio/channels/DatagramChannel/MulticastSendReceiveTests.java
    java/nio/channels/DatagramChannel/PromiscuousIPv6.java
    java/nio/channels/DatagramChannel/Loopback.java
    java/nio/channels/FileChannel/FileExtensionAndMap.java

The FileExtensionAndMap.java has clear fail message: "Error. Test 
ignored: This test has huge disk space requirements".


Thanks,
Alexander.

The code in linux_close.c is only used with the old (and not used by 
default) SocketImpl and DatagramSocketImpl implementation. Some of the 
tests jdk_net test group will re-run the tests with the old 
implementation so you should be okay.




I run test/jdk/java/net tests on Ubuntu 18.04.4 and Alpine Linux 
3.11.6 with musl libc 1.1.24.
There are 10 failed tests but they fail without the fix as well. I 
believe that it is because of some network settings of  my machine.
Sometimes there is will firewall or iptables configuration need to 
allow multicast datagrams to be received.


-Alan

Re: RFR: JDK-8249699: java/io/ByteArrayOutputStream/MaxCapacity.java should use @requires instead of @ignore

[Seán Coffey]

Apologies. Meant to reply yesterday. Your edit looks fine to me.

regards,
Sean.

On 27/08/2020 16:41, Fernando Guallini wrote:

Thanks Sean, updated webrev here: 
http://cr.openjdk.java.net/~fguallini/8249699/webrev.01/

Regards,
Fernando
- Original Message -
From: sean.cof...@oracle.com
To: fernando.guall...@oracle.com, core-libs-dev@openjdk.java.net
Sent: Wednesday, 26 August, 2020 7:39:25 PM GMT +00:00 GMT Britain, Ireland, 
Portugal
Subject: Re: RFR: JDK-8249699: java/io/ByteArrayOutputStream/MaxCapacity.java 
should use @requires instead of @ignore

test/jdk/java/util/Base64/TestEncodingDecodingLength.java is an example
of another test using -Xmx8g. Do you want to push the os.maxMemory
requirement up to 10g perhaps ? It might avoid border line resource
failures. Also I think it might need a "sun.arch.data.model == "64" "
requirement :

@requires (sun.arch.data.model == "64" & os.maxMemory >= 10g)

regards,
Sean.

On 26/08/2020 18:17, Fernando Guallini wrote:

Hi,

Could I please get reviews and a sponsor for:

webrev: http://cr.openjdk.java.net/~fguallini/8249699/webrev.00/ 

Testbug: https://bugs.openjdk.java.net/browse/JDK-8249699 


The test was ignored due to ‘huge memory requirements’, but with the jtreg 
current version, tests can be only run when system requirements are satisfied 
as opposed to being always excluded. It runs and passes in Mach5


Thanks

-Fernando

Re: RFR 8251989: Hex encoder and decoder utility

[Roger Riggs]

Hi Raffaello,

Missed sending this yesterday, the changes are in the updated webrev.

On 8/23/20 10:42 AM, Raffaello Giulietti wrote:

...

Hi Roger,

here are my notes about the Decoder.

(1) The only serious issue I could find is on L.548. It should read
L.548   CharBuffer cb = CharBuffer.wrap(chars, index, index + length);
as method wrap() takes a start and an end.
(It's quite confusing and error prone to have both (start, length) and
(start, end) conventions on ranges all over the OpenJDK :-( Nobody's
fault, just annoying.)

Well spotted, fixed.



The rest are minor notes.

(2) While I'm pretty sure that a JIT compiler can remove the index range
check on L.465 when len is replaced by bytes.length on L.463, I'm less
sure that it can do it as currently coded, despite len and bytes.length
having the same value. Readability wouldn't suffer and len could be 
removed.

The JIT doesn't have trouble following those values.



(3) The expressions preLen > origLen - sufLen on L.492 and sufLen >
origLen - preLen on L.496 are equivalent. The latter is thus useless on
L.496 because it is always false when execution reaches here.

true; updated to optimize the case of empty prefix and/or suffix.



(4) The code starting at L.588 assumes that delimiter.length() > 0,
which is ensured by the optimization in L.585-586.
Without this assumption, however, neither L.509 nor L.512 are correct
when delimiter.length() == 0.
To make the code to work even when, for some reason, delimiter.length()
== 0) the lines can be replaced by the slightly more involved yet more
robust
L.509   if ((subcs.length() - 2) % stride != 0)
and
L.512   final int len = (subcs.length() - 2) / stride + 1;

fixed



(5) I guess that the rationale for not failing fast on illegal
characters in the loop on L.516-521, and accumulate and postpone the
check only after the whole cs has been processed, is that the code
optimistically assumes that cs is legal, so it would be wasteful to
check at each iteration.
But since each iteration already needs a quite more expensive check for
the delimiter, I wonder if the postponing makes much economical sense.
The check could be directly on v before assignment to bytes and the var
illegal could be removed.

The code is easier to read with the inline checs and throws.



(6) What about moving L.517 at the end of the loop body so that memory
accesses are issued with strictly increasing and hardware predictable
addresses? This move would also better reveal the "left-to-right"
processing intent.
ok, though with a lot of reordering and caching in hardware it won't 
make a difference.





(7) On L.561, delimiter.isEmpty() could be removed as checkDelimiter()
is not invoked when the delimiter is empty. But it's perhaps safer to
have it.

It could be an assert, but asserts aren't enabled always



(8) You may want to wrap escapeNL() around the message on L.567-569

ok



(9) fromHexPair() and fromHex() can be declared static.


(10) equals() and hashCode() are missing despite the class being
value-based. This holds for Encoder as well.

Added



I look forward for the next iteration of the code.


Thanks  for the comments,

Roger




Greetings
Raffaello

Re: RFR: 8250968: Symlinks attributes not preserved when using jarsigner on zip files

[Seán Coffey]

I've been poking around the zip internals and am now able to locate the 
16 bits of interest. The position of these actual bits does appear to 
move around from one test run to another. For now, I guess it's 
sufficient to look for the pattern of interest in the signed zip file. 
New testcase added.


http://cr.openjdk.java.net/~coffeys/webrev.8250968.v4/webrev/

regards,
Sean.

On 27/08/2020 15:58, Weijun Wang wrote:

Looks like it was a conscious design decision to only allow recording of POSIX 
permission bits for this field (& 0xFFF). I don't see anything about symlink 
support in zipfs docs.

As long as that*byte*  is there and it’s not difficult to locate, we can 
manually add the*bit*  for symlink and see if jarsigner can keep it.

—Max

Re: [PATCH] optimization opportunity regarding misuse of BufferedInputStream

[Florian Weimer]

* Сергей Цыпанов:

> Hi,
>
>> Isn't this InputStream::readAllBytes?
>
> thanks for pointing this out! Indeed, InputStream::readAllBytes() allows to 
> save even more memory:
>
>   Mode  Cnt   Score Error   Units
> read  avgt   50 227.054 ±   1.354   us/op
> read:·gc.alloc.rate.norm  avgt   50  138605.638 ±  20.778B/op
> readNoVerify  avgt   50 226.606 ±   1.748   us/op
> readNoVerify:·gc.alloc.rate.norm  avgt   50  137810.392 ±   7.020B/op
>
> Would you sponsor the changes?

Sorry, I don't have that kind of role on the OpenJDK project.

Florian

Re: [PATCH] optimization opportunity regarding misuse of BufferedInputStream

[Сергей Цыпанов]

Hi,

> Isn't this InputStream::readAllBytes?

thanks for pointing this out! Indeed, InputStream::readAllBytes() allows to 
save even more memory:

  Mode  Cnt   Score Error   Units
read  avgt   50 227.054 ±   1.354   us/op
read:·gc.alloc.rate.norm  avgt   50  138605.638 ±  20.778B/op
readNoVerify  avgt   50 226.606 ±   1.748   us/op
readNoVerify:·gc.alloc.rate.norm  avgt   50  137810.392 ±   7.020B/op

Would you sponsor the changes?

Regards,
Sergey Tsypanov

28.08.2020, 12:08, "Florian Weimer" :
> * Сергей Цыпанов:
>
>>  @@ -105,12 +105,8 @@
>>   private byte[] getBytes(InputStream is)
>>   throws IOException
>>   {
>>  - byte[] buffer = new byte[8192];
>>   ByteArrayOutputStream baos = new ByteArrayOutputStream(2048);
>>  - int n;
>>  - while ((n = is.read(buffer, 0, buffer.length)) != -1) {
>>  - baos.write(buffer, 0, n);
>>  - }
>>  + is.transferTo(baos);
>>   return baos.toByteArray();
>>   }
>
> Isn't this InputStream::readAllBytes?
>
> Thanks,
> Florian

Re: RFR 8252248: __SIGRTMAX is not declared in musl libc

[Alan Bateman]

On 28/08/2020 12:25, Alexander Scherbatiy wrote:

Hello,

Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/8252248/webrev.01/

 - moving shared code to net_util_md.h is avoided
 - INTERRUPT_SIGNAL is defined as SIGRTMAX - 2 instead of __SIGRTMAX - 
2 for Linux in NativeThread.c

 - "#include " is moved out from "#ifdef" in NativeThread.c
 - sigWakeup is changed to "#define WAKEUP_SIGNAL (SIGRTMAX - 2)" in 
linux_close.c


At least test/jdk/java/net/Socket/asyncClose/AsyncClose.java touches 
all four methods where the signal changes are used:

NativeThread.c
- Java_sun_nio_ch_NativeThread_init(JNIEnv *env, jclass cl)
- Java_sun_nio_ch_NativeThread_signal(JNIEnv *env, jclass cl, jlong 
thread)

linux_close.c
-  __attribute((constructor)) init()
- closefd(int fd1, int fd2)
NativeThread current/signal will be exercised by the tests in 
java/nio/channels.


The code in linux_close.c is only used with the old (and not used by 
default) SocketImpl and DatagramSocketImpl implementation. Some of the 
tests jdk_net test group will re-run the tests with the old 
implementation so you should be okay.





I run test/jdk/java/net tests on Ubuntu 18.04.4 and Alpine Linux 
3.11.6 with musl libc 1.1.24.
There are 10 failed tests but they fail without the fix as well. I 
believe that it is because of some network settings of  my machine.
Sometimes there is will firewall or iptables configuration need to allow 
multicast datagrams to be received.


-Alan

Re: RFR 8252248: __SIGRTMAX is not declared in musl libc

[Alexander Scherbatiy]

Hello,

Could you review the updated fix:
  http://cr.openjdk.java.net/~alexsch/8252248/webrev.01/

 - moving shared code to net_util_md.h is avoided
 - INTERRUPT_SIGNAL is defined as SIGRTMAX - 2 instead of __SIGRTMAX - 
2 for Linux in NativeThread.c

 - "#include " is moved out from "#ifdef" in NativeThread.c
 - sigWakeup is changed to "#define WAKEUP_SIGNAL (SIGRTMAX - 2)" in 
linux_close.c


At least test/jdk/java/net/Socket/asyncClose/AsyncClose.java touches all 
four methods where the signal changes are used:

NativeThread.c
- Java_sun_nio_ch_NativeThread_init(JNIEnv *env, jclass cl)
- Java_sun_nio_ch_NativeThread_signal(JNIEnv *env, jclass cl, jlong thread)
linux_close.c
-  __attribute((constructor)) init()
- closefd(int fd1, int fd2)

I run test/jdk/java/net tests on Ubuntu 18.04.4 and Alpine Linux 3.11.6 
with musl libc 1.1.24.
There are 10 failed tests but they fail without the fix as well. I 
believe that it is because of some network settings of  my machine.


  java/net/MulticastSocket/IPMulticastIF.java
  java/net/MulticastSocket/NoSetNetworkInterface.java
  java/net/MulticastSocket/Promiscuous.java
  java/net/MulticastSocket/PromiscuousIPv6.java
  java/net/MulticastSocket/SetGetNetworkInterfaceTest.java
  java/net/MulticastSocket/SetLoopbackMode.java
  java/net/MulticastSocket/SetOutgoingIf.java
  java/net/MulticastSocket/Test.java
  java/net/NetworkInterface/NetworkInterfaceRetrievalTests.java
  java/net/ipv6tests/UdpTest.java

Thanks,
Alexander.

On 26.08.2020 13:42, Alan Bateman wrote:

On 25/08/2020 18:00, Alexander Scherbatiy wrote:


Hello,

Could your review the fix for the issue:
  Bug: https://bugs.openjdk.java.net/browse/JDK-8252248
  Fix: http://cr.openjdk.java.net/~alexsch/8252248/webrev.00/

:

The fix has been discussed on the portola-dev alias [2] where it was 
pointed out that the fix can be reviewed in the mainline and it was 
suggested to rename the INTERRUPT_SIGNAL and move its definition to 
net_util_md.h.


The xxx_close.c sources are for the old SocketImpl and 
DatagramSocketImpl implementations; they are aren't used by default 
and will eventually go away. Maybe an option for the musl port is to 
just not compile in the old socket implementations? It might be best 
to start a discussion on nio-dev and net-dev about this as we 
shouldn't be changing NativeThread.c (which is used for signalling in 
the FileChannel implementation) to include net_util.h.


-Alan

Re: [PATCH] optimization opportunity regarding misuse of BufferedInputStream

[Florian Weimer]

* Сергей Цыпанов:

> @@ -105,12 +105,8 @@
>  private byte[] getBytes(InputStream is)
>  throws IOException
>  {
> -byte[] buffer = new byte[8192];
>  ByteArrayOutputStream baos = new ByteArrayOutputStream(2048);
> -int n;
> -while ((n = is.read(buffer, 0, buffer.length)) != -1) {
> -baos.write(buffer, 0, n);
> -}
> +is.transferTo(baos);
>  return baos.toByteArray();
>  }

Isn't this InputStream::readAllBytes?

Thanks,
Florian