Re: The new optimized version of Dual-Pivot Quicksort

[Laurent Bourgès]

Hi,

I am currently testing many sort algorithms to improve the Marlin renderer
(AA 2D shape rasterizer), I integrated since OpenJDK9 and am still
improving for OpenJDK12 .

I created my MergeSort (top down, check for sorted parts, array / buffer
swap to minimize moves, isort on small sub arrays) that sort 2 int[]:
crossing + edge indices.
 It is critical as edge crossings are sorted at every scanline, data are
almost sorted/reversed, not really random.

3 questions:
- why is this patch dormant ? I checked in openjdk12 repo and your changes
were not integrated.
- I need a sort() method that works with 2 arrays: data + indices (pointer
like). Such sorted indices are useful to use for lookups c[idx[k]] or to
perform other array permutations...
Would you agree adding such new sort(a[], low, high, indices)
- Marlin uses a ThreadLocal context to avoid any allocation. JDK sort()
impl do not accept parameters to give any buffer[] and avoid allocations.
Would you agree adding such optional parameters (like workbase[]) ?

I will experiment adapting the DualPivotQuickSort in Marlin renderer and
perform array sort race & rendering benchmarks.

Cheers,
Laurent

Le ven. 19 janv. 2018 à 14:38, Vladimir Yaroslavskiy  a
écrit :

> Hi team,
>
> In Sept 2009 Josh Bloch, Jon Bentley and I introduced new sorting
> algorithm, Dual-Pivot Quicksort, for primitives in JDK 7 and later
> I suggested several improvements of Dual-Pivot Quicksort, which
> were integrated into JDK 8.
>
> Now I have more optimized and faster version of Dual-Pivot Quicksort.
> I have been working on it for the last 5 years. Please, find the
> summary of changes below and sources / diff at webrev [1].
>
> All tests and benchmarking were run on the most recent build of JDK 10,
> jdk-10-ea+39. The new version shows the better performance on different
> inputs and guarantees n*log(n) on any data.
>
> The new implementation of Dual-Pivot Quicksort is 'all-in-one' version:
> it contains one code for both parallel and sequential sorting algorithms.
>
> Suggested version is 10-20% faster on random data, 1.5-4 times faster
> on nearly structured arrays, 1.5-2 times faster on period inputs.
>
> Parallel Dual-Pivot Quicksort is 1.5-3 times faster than current
> algorithm based on merge sort.
>
> Benchmarking on the test suite, suggested by Jon Bentley, shows the
> boost of performance in 1.4 times. This test suite contains several
> types of inputs, such as random data, nearly structured arrays, data
> with period and so on. Also there are several modifications of inputs
> and parameters which are used in data building. We run sorting on all
> combinations to compare two algorithms.
>
> Please let me know if you have any questions / comments.
>
> Summary of changes:
>
> DualPivotQuicksort class
> 
> * Pivots are chosen with another step, the 1-st and 5-th candidates
>are taken as pivots instead of 2-nd and 4-th.
> * Splitting into parts is related to the golden ratio
> * Pivot candidates are sorted by combination of 5-element
>network sorting + insertion sort
> * New backwards partitioning is simpler and more efficient
> * Quicksort tuning parameters were updated
> * Merging sort is invoked on each iteration from Quicksort
> * Additional steps for float/double were fully rewritten
> * Heap sort is invoked on the leftmost part
> * Heap sort is used as a guard against quadratic time
> * Parallel sorting is based on Dual-Pivot Quicksort
>instead of merge sort
>
> SortingAlgorithms class
> ---
> * Merging sort and pair insertion sort were moved from
>DualPivotQuicksort class
> * Pair insertion sort was simplified and optimized
> * New nano insertion sort was introduced for tiny arrays
> * Merging sort was fully rewritten
> * Optimized merging partitioning is used
> * Merging parameters were updated
> * Merging of runs was fully rewritten
> * Fast version of heap sort was introduced
> * Parallel merging sort was also provided
>
> Sorting / ParallelSorting classes
> -
> * New test cases were added
> * Sources of these classes were unified
>
> Arrays class
> 
> * Calls of Dual-Pivot Quicksort were updated
> * Parallel sorting of primitives was switched to parallel
>Dual-Pivot Quicksort
> * Javadoc was modified
>
> ArraysParallelSortHelpers class
> ---
> * Old implementation of parallel sorting for primitives was removed
> * Javadoc was updated
>
> Thank you,
> Vladimir
>
> 
> [1] http://cr.openjdk.java.net/~alanb/DualPivotSortUpdate/webrev.01/
> 
>

Re: [12] RFR of JDK-8213576: Make test AsyncCloseChannel.java run in othervm

[Amy Lu]

Thank you David for the quick review.

Pushed.

Thanks,
Amy

On 2018/11/9 12:04 PM, David Holmes wrote:

Okay. Worth a try.

Thanks Amy.

David

On 9/11/2018 1:18 PM, Amy Lu wrote:

java/nio/channels/SocketChannel/AsyncCloseChannel.java

Please review this trivial fix to make this test run in othervm to 
bring Mach 5 back to green.

bug: https://bugs.openjdk.java.net/browse/JDK-8213576

This test fails repeatedly in Mach 5 on osx-x64 (JDK-8213235) 
recently, and the issue JDK-8213235is still under investigation.


Thanks,
Amy


--- 
old/test/jdk/java/nio/channels/SocketChannel/AsyncCloseChannel.java 
2018-11-09 10:40:02.0 +0800
+++ 
new/test/jdk/java/nio/channels/SocketChannel/AsyncCloseChannel.java 
2018-11-09 10:40:01.0 +0800

@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2006, 2008, Oracle and/or its affiliates. All 
rights reserved.
+ * Copyright (c) 2006, 2018, Oracle and/or its affiliates. All 
rights reserved.

   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
   *
   * This code is free software; you can redistribute it and/or 
modify it

@@ -24,13 +24,16 @@
  /* @test
   * @bug 6285901 6501089
   * @summary Check no data is written to wrong socket channel during 
async closing.

- * @author Xueming Shen
+ * @run main/othervm AsyncCloseChannel
   */

-import java.io.*;
-import java.nio.*;
-import java.nio.channels.*;
-import java.net.*;
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.nio.ByteBuffer;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.SocketChannel;

  public class AsyncCloseChannel {
  static volatile boolean failed = false;

Re: [12] RFR of JDK-8213576: Make test AsyncCloseChannel.java run in othervm

[David Holmes]

Okay. Worth a try.

Thanks Amy.

David

On 9/11/2018 1:18 PM, Amy Lu wrote:

java/nio/channels/SocketChannel/AsyncCloseChannel.java

Please review this trivial fix to make this test run in othervm to bring 
Mach 5 back to green.

bug: https://bugs.openjdk.java.net/browse/JDK-8213576

This test fails repeatedly in Mach 5 on osx-x64 (JDK-8213235) recently, 
and the issue JDK-8213235is still under investigation.


Thanks,
Amy


--- old/test/jdk/java/nio/channels/SocketChannel/AsyncCloseChannel.java 
2018-11-09 10:40:02.0 +0800
+++ new/test/jdk/java/nio/channels/SocketChannel/AsyncCloseChannel.java 
2018-11-09 10:40:01.0 +0800

@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2006, 2008, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2006, 2018, Oracle and/or its affiliates. All rights 
reserved.

   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
   *
   * This code is free software; you can redistribute it and/or modify it
@@ -24,13 +24,16 @@
  /* @test
   * @bug 6285901 6501089
   * @summary Check no data is written to wrong socket channel during 
async closing.

- * @author Xueming Shen
+ * @run main/othervm AsyncCloseChannel
   */

-import java.io.*;
-import java.nio.*;
-import java.nio.channels.*;
-import java.net.*;
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.nio.ByteBuffer;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.SocketChannel;

  public class AsyncCloseChannel {
  static volatile boolean failed = false;

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[David Holmes]

Hi Vicente,

On 9/11/2018 2:39 AM, Vicente Romero wrote:

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features?


I changed only the tests for which the new ASM was complaining about a 
particular API available only for ASM7


I could not understand how this could be if the tests and other code 
were unchanged, so I applied the ASM-only patch and investigated the 
first failure running nasgen. And of course the problem is the 
NestHost/NestMembers attributes! We modified our internal version of ASM 
to add nestmate support, but of course this update removes that and 
replaces it with the official support. But the official support is only 
enabled for ASMv7 so we must update all users of ASM to request version 7.


Thanks,
David
-

If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with ASM5).


I have made two webrevs to make the review easier [1], contain only the 
changes to the internal asm and [2] contains the changes to the clients 
plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/ 



On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal version 
in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

[12] RFR of JDK-8213576: Make test AsyncCloseChannel.java run in othervm

[Amy Lu]

java/nio/channels/SocketChannel/AsyncCloseChannel.java

Please review this trivial fix to make this test run in othervm to bring 
Mach 5 back to green.

bug: https://bugs.openjdk.java.net/browse/JDK-8213576

This test fails repeatedly in Mach 5 on osx-x64 (JDK-8213235) recently, 
and the issue JDK-8213235is still under investigation.


Thanks,
Amy


--- old/test/jdk/java/nio/channels/SocketChannel/AsyncCloseChannel.java 
2018-11-09 10:40:02.0 +0800
+++ new/test/jdk/java/nio/channels/SocketChannel/AsyncCloseChannel.java 
2018-11-09 10:40:01.0 +0800

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2006, 2008, Oracle and/or its affiliates. All rights 
reserved.
+ * Copyright (c) 2006, 2018, Oracle and/or its affiliates. All rights 
reserved.

  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -24,13 +24,16 @@
 /* @test
  * @bug 6285901 6501089
  * @summary Check no data is written to wrong socket channel during 
async closing.

- * @author Xueming Shen
+ * @run main/othervm AsyncCloseChannel
  */

-import java.io.*;
-import java.nio.*;
-import java.nio.channels.*;
-import java.net.*;
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
+import java.nio.ByteBuffer;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.SocketChannel;

 public class AsyncCloseChannel {
 static volatile boolean failed = false;

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Brent Christian]

On 11/8/18 12:52 PM, Brian Burkhalter wrote:

Hi Daniel,


On Nov 8, 2018, at 1:50 AM, Daniel Fuchs  wrote:

So FWIW my point was that there's nothing that you can really
guarantee in InputSteam::skipFully() if a subclass implementation of
skip() uses negative number to e.g. signal abnormal conditions
(such as EOF?). And I wonder if that should warrant a disclaimer
in the API doc of InputStream::skipFully (maybe an @implSpec?)


I don’t know about having an explicit disclaimer; that seems a little strange 
to me. If the new method is defined in terms of skip() the I would think that 
it is implicit that misbehavior of the latter would cause problems.



My thinking is along the lines of Daniel's.

skipNBytes() should account for weird behavior on the part of a 
subclass's skip() method.  I'm also in favor of documenting which other 
InputStream methods might be called.


But some of the possible skip() behavior being described is not 
mentioned in InputStream, such as skipping MORE than n bytes, or calling 
skip() with negative values in order to navigate backwards.


Some sort of disclaimer could frame the behavior being described, even 
if only something like, "skip() makes a best effort to account for a 
subclass's overridden skip() method behaving unexpectedly"


-Brent

Re: RFR 8185496: Improve performance of system properties initialization in initPhase1

[Mandy Chung]

On 11/8/18 7:41 AM, Roger Riggs wrote:


Webrev updated in place:  (Only System.java is modified)

http://cr.openjdk.java.net/%7Erriggs/webrev-props-cleanup-8185496/index.html


Just notice this...

I wonder if System.setProperties(null) should call 
VM.saveAndRemoveProperties(props) to be consistent.  It's an existing 
issue.  You may consider refactoring and adding initSystemProperties 
method that will be called from initPhase1 and setProperties.


Otherwise looks good.

Mandy

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Brian Burkhalter]

Hi Daniel,

> On Nov 8, 2018, at 1:50 AM, Daniel Fuchs  wrote:
> 
> So FWIW my point was that there's nothing that you can really
> guarantee in InputSteam::skipFully() if a subclass implementation of
> skip() uses negative number to e.g. signal abnormal conditions
> (such as EOF?). And I wonder if that should warrant a disclaimer
> in the API doc of InputStream::skipFully (maybe an @implSpec?)

I don’t know about having an explicit disclaimer; that seems a little strange 
to me. If the new method is defined in terms of skip() the I would think that 
it is implicit that misbehavior of the latter would cause problems.

Thanks,

Brian

Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath - enhance some IOExceptions with path causing the issue

[Sean Mullan]

On 11/7/18 3:52 AM, Baesken, Matthias wrote:

Sorry, I haven't had time to look at this in more detail yet. But, let's
take a step back first. Can you or Matthias explain in more detail why
this fix is necessary? What are the use cases and motivation?


Hello,
 adding paths  (or maybe more details)  to exception messages just makes 
analyzing errors easier.
You do not get just "Bad path",  but also the real bad path which gives you a 
hint where to look and analyze further .


File paths are, in general, always something that demands extra scrutiny 
as it can be the source of security issues (privacy leaks, traversal 
attacks, etc). It's not just me that thinks that way, you can do a 
search on the Internet and find lots of references.



That's why we introduced it in our JVM ages ago.
I have to agree that additionally  printing cwd / user.dir is a bit special,  
so I omit that from this revision of the patch.
This makes the patch more simple.
New revision :

http://cr.openjdk.java.net/~mbaesken/webrevs/8211752.1/


Unfortunately the usage of sun.security.util.SecurityProperties  (which was 
considered)  in the  static { ... }
class initializers (e.g. UnixFileSystem.java) just does not work.
It fails with already in the build (!) with :

Error occurred during initialization of boot layer
java.lang.ExceptionInInitializerError
Caused by: java.lang.NullPointerException

(seems it is too early in the game for SecurityProperties).
(btw. this is another  NOT very helpful exception error message)


So I unfortunately had to go back to using system properties.


Btw. another question regarding path output in exceptions  :
you seem to consider it a bad thing  to (unconditionally) print paths in the 
exception messages,
but then on the other hand we have  it  already in OpenJDK UnixFileSystem_md.c :

  269 JNIEXPORT jboolean JNICALL
  270 Java_java_io_UnixFileSystem_createFileExclusively(JNIEnv *env, jclass cls,
  271   jstring pathname)
  272 {
  ...
  277 /* The root directory always exists */
  278 if (strcmp (path, "/")) {
  279 fd = handleOpen(path, O_RDWR | O_CREAT | O_EXCL, 0666);
  280 if (fd < 0) {
  281 if (errno != EEXIST)
  282 JNU_ThrowIOExceptionWithLastError(env, path);
  283 } else {
  284 if (close(fd) == -1)
  285 JNU_ThrowIOExceptionWithLastError(env, path);


Why is it fine here for a long time , but considered harmful at the other 
places ?
If we want to be consistent, we should then  write  "Bad path" here (or  allow 
the path output at the other places too ).


It might be perfectly fine and your usage might be ok too. But I'll need 
some time to evaluate it further. I am not familiar with the code in 
this part of the JDK.


I would also see if you can think about the security issues as well. 
Where do these paths come from? What are the application use cases that 
invoke these lower methods? From application code or elsewhere? Are 
relative paths used? Are paths containing ".." canonicalized? Are they 
arbitrary paths or a fixed set of paths? Do they ever contain sensitive 
information like home directory user names, etc?


Once we understand if there are any security issues, then we can decide 
if allowing that via a system property is acceptable or not, and if so 
the security risks that we would have to document for that property.


Thanks,
Sean




Thanks, Matthias




-Original Message-
From: Sean Mullan 
Sent: Freitag, 12. Oktober 2018 17:19
To: Langer, Christoph ; Baesken, Matthias
; Alan Bateman ;
security-...@openjdk.java.net; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8211752: JNU_ThrowIOExceptionWithLastErrorAndPath -
enhance some IOExceptions with path causing the issue

On 10/12/18 10:33 AM, Langer, Christoph wrote:

Sean, what is your take on this?


Sorry, I haven't had time to look at this in more detail yet. But, let's
take a step back first. Can you or Matthias explain in more detail why
this fix is necessary? What are the use cases and motivation? The bug
report doesn't go into any detail about that and there isn't anything
in the initial RFR email that explains why this change is useful or
necessary. As a general guideline or advice, RFEs should include this
type of information so that Reviewers understand more of the context and
motivation behind the change.

Thanks,
Sean

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Vladimir Kozlov]

Thank you, Andrew, for sending changes

I am fine with intrinsics general wording in JEP. I 'reviewed' JEP.

I have several questions and issues with proposed changes in Hotspot which needs to be discussed 
during changes review. My main question is - should new nodes be treated as global memory barriers 
(not only RAW memory)?


One suggestion I can give is to introduce a feature flag in java code which will be set by VM if 
platform can support the feature. Similar to COMPACT_STRINGS in JEP 254 Compact Strings:


http://hg.openjdk.java.net/jdk/jdk/file/66a0e6b3ec1a/src/java.base/share/classes/java/lang/String.java#l195

Thanks,
Vladimir

On 11/8/18 1:10 AM, Andrew Dinn wrote:

On 07/11/18 17:12, Vladimir Kozlov wrote:

I am Lead for Hotspot [1]. Alan is Group Lead for core libs and he gave
review already.

I don't see any reference to Hotspot in JEP so I am not sure what to
review. Do you need any new optimizations/intrinsics in Hotspot for this
JEP?


Yes I do need some new intrinsics. I was not clear whether they needed
to be documented in the JEP. Perhaps you could advise?

n.b. If you need to know what is being proposed in order to answer that
I can point you at my prototype implementation. Details after the sig.


You need to ask Alan or Brian Goetz (as Area Lead) for endorsement
before 'Submitting' JEP [2].


Ok, will do once I know whether details of the intrinsics have to be
included. Thanks for your help.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

- 8<  8<  8<  8<  8<  8< ---

The basic operation to persist ByteBuffer changes is provided via a new
method of jdk.internal.misc.Unsafe which /is/ currently described in the
JEP:

   public void writebackMemory(long address, long length)

My prototype implements this method using 3 intrinsics, a pre-writeback
memory sync, a per-cache line force (executed in a loop) and a
post-writeback memory sync.

I also added a native method which allows the (cpu-specific) cache line
size to be retrieved at class init time.

Webrev:

   http://cr.openjdk.java.net/~adinn/pmem/webrev.04/

Unsafe changes:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/java.base/share/classes/jdk/internal/misc/Unsafe.java.udiff.html

In the underlying implementation of the intrinsics there are 3
corresponding new IR nodes, CacheWBNode, CacheWBPreSyncNode and
CacheWBPostSyncNode. They are matched by processor-specific ad file
rules to generate the required assembler

Intrinsics implementation:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/share/opto/memnode.hpp.udiff.html

http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/share/opto/library_call.cpp.udiff.html

Back end rules:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/aarch64/aarch64.ad.udiff.html


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/x86_64.ad.udiff.html

Assembler changes:

The assembler implementations are fairly straightforward. There is no
need for a pre sync on either AArch64 or x86_64. A post sync is always
needed on AArch64. It may not be needed on x86 depending on what type of
cache line flush the processor supports


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp.udiff.html


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/macroAssembler_x86.cpp.udiff.html

http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/assembler_x86.cpp.udiff.html

Re: RFR(JDK 12/NIO) 8202285: (fs) Add a method to Files for comparing file contents

[Joe Wang]

Checked in. Thanks all for all of the great help!

Best regards,
Joe

On 11/7/18, 11:39 AM, Ivan Gerasimov wrote:

Thank you Joe!

I like the last variant.

With kind regards,
Ivan

On 11/7/18 9:59 AM, Joe Wang wrote:

Thanks Ivan!

I agree that the upfront edge case checks aren't really necessary, 
after all, the great majority of the use cases won't hit the edge 
case (with the size being a factor of the buffer). We're therefore 
better off without the checks.


Here's an updated webrev: 
http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v08/


Best,
Joe

On 11/6/18, 4:21 PM, Ivan Gerasimov wrote:

Hi Joe!

I apologize, if it was already discussed before.

Why do you need to have 3 edge cases of nRead1 and nRead2 in lines 
1596-1605:  Was it to save a call to Arrays.mismatch()?


If I'm not mistaken, the code would work equally well, if lines 
1596-1615 were replaced with just subrange 1606-1614.


With kind regards,
Ivan

On 11/6/18 12:53 PM, Joe Wang wrote:
Thanks Andrew for pointing that out. It wasn't intentional, a 
result of copy n paste after removing the support class. It's fixed 
now:

http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v07/

-Joe

On 11/6/18, 10:27 AM, Andrew Luo wrote:
I'm not a reviewer, but just wanted to point out that the fully 
qualified Files.BUFFER_SIZE can be simplified to just 
BUFFER_SIZE.  Searching through the code in Files.java I don't see 
many other instances of using Files.* (although I do see a few).


Thanks
Andrew

-Original Message-
From: core-libs-dev On 
Behalf Of Roger Riggs

Sent: Tuesday, November 6, 2018 8:56 AM
To: core-libs-dev@openjdk.java.net
Subject: Re: RFR(JDK 12/NIO) 8202285: (fs) Add a method to Files 
for comparing file contents


+1

Though with 65 tests, I suspect that there are more cases than 
strictly needed to cover all the code flows.
For example, three cases for testing when it is the same file 
doesn't seem necessary.


Regards, Roger

On 11/05/2018 11:27 PM, Stuart Marks wrote:

On 10/19/18 11:26 AM, Joe Wang wrote:

Current version:
http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v06/

Hi Joe,

Thanks for updating the tests per my comments. Everything looks 
good now!


s'marks

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features?


I changed only the tests for which the new ASM was complaining about a 
particular API available only for ASM7


If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with ASM5).


I have made two webrevs to make the review easier [1], contain only the 
changes to the internal asm and [2] contains the changes to the clients 
plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/


On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal version 
in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

On 11/8/18 8:14 AM, Alan Bateman wrote:

On 07/11/2018 19:33, Igor Ignatyev wrote:

Hi Vicente,

I recall an (internal?) discussion about updating ASM somewhen in JDK 
11TF, and AFAIR it was decided not to update ASM b/c nothing in JDK 
needs that, has it been changed? put somewhat differently, why are we 
doing this?


in any case, I don't like the changes in mlvm tests. I understand 
that ClassWriter has been significantly changed in ASM 7.0, so 
ClassWriterExt can't disable CP entries caching (at least not in the 
way it used to), but removing setCache* calls from the tests changed 
them and in some cases made them invalid as they don't test that they 
supposed to. therefore I'd prefer to leave all calls setCache* as-is, 
change setCache* implementation to throw an exception (similarly to 
the fix in JDK-8194826 
) and mark all 
tests which throw this exception w/ '@ignore 8194951' jtreg tag.



ClassWriterExt the MLVM tests have come in previous upgrades too. Has 
there been any discussion Remi or others on ASM to make it easier for 
the JDK to upgrade?


I'm not aware of any such discussions.



-Alan


Vicente

Re: RFR 8185496: Improve performance of system properties initialization in initPhase1

[Roger Riggs]

Hi Claes,

yes, those template files bit me again.  And the tools didn't save me (yet).
I wonder if it is worth the trouble to make the generated source files 
be read-only...


Thanks, Roger


On 11/08/2018 10:50 AM, Claes Redestad wrote:

Hi Roger,

On 2018-11-08 16:41, Roger Riggs wrote:

Hi Mandy,

Webrev updated in place:  (Only System.java is modified)

http://cr.openjdk.java.net/%7Erriggs/webrev-props-cleanup-8185496/index.html 



Don't you need to update VersionProps.template.java, too?

/Claes

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Andrew Dinn]

Hi Roger,

On 08/11/18 14:51, Roger Riggs wrote:
> If to achieve the performance or functional goals of the JEP hotspot
> changes are needed
> they should be mentioned (no details needed) in the JEP.
> It helps the reader understand the scope and impact of the change.

Thanks. I have added a brief overview of the required Hotspot changes. I
also added a note about the target OS/CPU combinations.

Vladimir, could you please take a look at the revised JEP and comment.
The Hotspot-specifc details are at the end of the Description section
where details of proposed new method Unsafe.writebackMemory are provided.

Thanks for any feedback you can provide.

regards,


Andrew Dinn
---

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Andrew Dinn]

Hi Derek,

On 08/11/18 15:49, White, Derek wrote:

> Given that there is platform-specific code, it would be good to be
> clear which platforms you are intending to implement as part of this
> JEP, and which platforms will need others to step in to support.
> 
> I'm quite happy with your plan, but I'd like to see more JEPs being
> clear about platform support (CPU and OS).

The prototype implements this on Linux/x86_64 and Linux/AArch64. On
other platforms it will refuse to create a persistent MappedByteBuffer
by throwing an exception.

I believe it should be straightforward to migrate it to Windows/x86_64
but I don't know for sure. That depends on support for the mmap MAP_SYNC
mode being available.

I am not currently in a position to implement or test Windows/x86_64. I
really don't know what would work on other hardware or OSes. I'd be
happy to take advice but I'd like to get this included targeting the 2
OS/CPU configurations mentioned above and see those other platforms
supported as an upgrade.

I'll add a note to the JEP to record this.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Re: RFR 8185496: Improve performance of system properties initialization in initPhase1

[Claes Redestad]

Hi Roger,

On 2018-11-08 16:41, Roger Riggs wrote:

Hi Mandy,

Webrev updated in place:  (Only System.java is modified)

http://cr.openjdk.java.net/%7Erriggs/webrev-props-cleanup-8185496/index.html


Don't you need to update VersionProps.template.java, too?

/Claes

Re: RFR 8185496: Improve performance of system properties initialization in initPhase1

[Roger Riggs]

Hi Mandy,

Webrev updated in place:  (Only System.java is modified)

http://cr.openjdk.java.net/%7Erriggs/webrev-props-cleanup-8185496/index.html

On 11/07/2018 07:19 PM, Mandy Chung wrote:

Hi Roger

On 11/6/18 8:17 AM, Roger Riggs wrote:
While working to reduce startup time initializing properties, a pair 
of improvements are proposed.


8185496: Improve performance of system properties initialization in 
initPhase1 [1]

8213424: VersionProps duplicate initialization [2]

1) The overhead of providing default values in native is reduced by 
applying the defaults

    when first used and leaving the properties undefined unless there is
    an OS supplied value or a -D command line argument
2) Two tests for properties are combined into a more complete test

webrev:

http://cr.openjdk.java.net/~rriggs/webrev-props-cleanup-8185496/



This is a good cleanup.  Some comments:

Is "user.timezone" system property not set by default?  Before the 
change, System.getProperty("user.timezone") always returns non-null.   
With this patch, if it returns null, it may need a CSR for this 
behavioral change.

ok, https://bugs.openjdk.java.net/browse/JDK-8213551

Please review when it is convenient.


System.java
  804 initProperties(props);
  805 System.props = props;
  806 VersionProps.init();

This seems worth refactoring of existing code.

initProperties(props) and VersionProps.init() are called in initPhase1.
I think VersionProps.init can be called right after initProperties(props)
in initPhase1.  So it'd be cleaner to have a java method to call
initProperties(props) and add the version-related system properties
to the given props (i.e. VersionProps.init could take Properties object).

ok, will do

jvm.cpp
    The other place to filter the property is Arguments::add_property 
or maybe in Arguments::parse_argument. The runtime team can advise 
where is the preferred place.

Its cleaner to keep it in the jvm_initProperties function and not pollute
the general Arguments parsing code with this exception case.


As you mentioned the future cleanup, the VM is using system properties 
as the internal interface with the library code and in some cases the 
library code removes those system properties after startup 
(VM.saveAndRemoveProperties).  As we discussed, one approach would be 
to define a native JVM interface to fetch the values in a batch but we 
have to measure the performance to determine if this is better.

yep, that's next.

Thanks, Roger



Mandy

Re: RFR: 8148188: Enhance the security libraries to record events of interest

[Erik Gahlin]

Hi Sean,

I think we could still call the event 
"jdk.SecurityPropertyModification", but add a @Description that explains 
that events are only emitted for the JDK due to security concerns. If we 
at a later stage want to include user events, we could add those and 
remove the @Description, possibly with a setting where you can specify 
scope, or perhaps a new event all together.


Perhaps we could find a better name for the field validationEventNumber? 
It sounds like we have an event number, which is not really the case. We 
have a counter for the validation id.


I noticed that you use hashCode as an id. I assume this is to simplify 
the implementation? That is probably OK, the risk of a collision is 
perhaps low, but could you make the field a long, so we in the future 
could add something more unique (Flight Recorder uses compressed 
integers, so a long uses the same amount of disk space as an int). Could 
you also rename the field and the annotation, perhaps certificationId 
could work, so we don't leak how the id was implemented.


I could not find the file: 
test/lib/jdk/test/lib/security/TestJdkSecurityPropertyModification.java


Thanks
Erik

With JDK-8203629 now pushed, I've re-based my patch on latest jdk/jdk 
code.


Some modifications also made based on off-thread suggestions :

src/java.base/share/classes/java/security/Security.java

* Only record JDK security properties for now (i.e. those in 
java.security conf file)
  - we can consider a new event to record non-JDK security events if 
demand comes about

  - new event renamed to *Jdk*SecurityPropertyModificationEvent

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

* Use hashCode() equality test for storing certs in List.

Tests updated to capture these changes

src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java

* Verify that self signed certs exercise the modified code paths

I've added new test functionality to test use of self signed cert.

webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v7/webrev/

Regards,
Sean.
On 17/10/18 11:25, Seán Coffey wrote:
I'd like to re-boot this review. I've been working with Erik off 
thread who's been helping with code and test design.


Some of the main changes worthy of highlighting :

* Separate out the tests for JFR and Logger events
* Rename some events
* Normalize the data view for X509ValidationEvent (old event name : 
CertChainEvent)
* Introduce CertificateSerialNumber type to make use of the 
@Relational JFR annotation.
* Keep calls for JFR event operations local to call site to optimize 
jvm compilation


webrev : http://cr.openjdk.java.net/~coffeys/webrev.8148188.v6/webrev/

This code is dependent on the JDK-8203629 enhancement being integrated.

Regards,
Sean.

On 10/07/18 13:50, Seán Coffey wrote:

Erik,

After some trial edits, I'm not so sure if moving the event & logger 
commit code into the class where it's used works too well after all.


In the code you suggested, there's an assumption that calls such as 
EventHelper.certificateChain(..) are low cost. While that might be 
the case here, it's something we can't always assume. It's called 
once for the JFR operation and once for the Logger operation. That 
pattern multiplies itself further in other Events. i.e. set up and 
assign the variables for a JFR event without knowing whether they'll 
be needed again for the Logger call. We could work around it by 
setting up some local variables and re-using them in the Logger code 
but then, we're back to where we were. The current EventHelper 
design eliminates this problem and also helps to abstract the 
recording/logging functionality away from the functionality of the 
class itself.


It also becomes a problem where we record events in multiple 
different classes (e.g. SecurityPropertyEvent). While we could leave 
the code in EventHelper for this case, we then have a mixed design 
pattern.


Are you ok with leaving as is for now? A future wish might be one 
where JFR would handle Logger via its own framework/API in a future 
JDK release.


I've removed the variable names using underscore. Also optimized 
some variable assignments in X509Impl.commitEvent(..)


http://cr.openjdk.java.net/~coffeys/webrev.8148188.v5/webrev/

regards,
Sean.

On 09/07/2018 18:01, Seán Coffey wrote:

Erik,

Thanks for reviewing. Comments inline..

On 09/07/18 17:21, Erik Gahlin wrote:

Thanks Sean.

Some feedback on the code in the security libraries.

- We should use camel case naming convention for variables (not 
underscore).

Sure. I see two offending variable names which I'll fix up.


- Looking at sun/security/ssl/Finished.java,

I wonder if it wouldn't be less code and more easy to read, if we 
would commit the event in a local private function and then use 
the EventHelper class for common functionality.
I'm fine with your suggested approach. I figured that tucking the 
recording/logging logic away in a helper class also had benefits 
but I'l

Re: Stream Method Proposal: long count(Predicate predicate)

[Roger Riggs]

Hi Jacob,

Its hard to resist the urge to add convenience methods, they look nice 
and help a few developers.
However, they accumulate rapidly and end up obscuring the core 
functionality.

They can hurt comprehension since they fold different functions together
and the collective API surface area ends up impinging on every 
developers learning curve.


$.02, Roger


On 11/07/2018 08:00 PM, Jacob Glickman wrote:

  Hello!

I see myself having to often call count() as a terminal operation on a
Stream immediately after performing a filter operation. How feasible would
it be to add an overloaded count() method that accepts a Predicate, which
it uses as a filter before returning the count of elements in the Stream?
If this is supported, I'd gladly create the webrev & tests for it!

I suppose the method signature can be something along the lines of:

 long count(Predicate predicate)

It would also seem reasonable to give this method to IntStream,
DoubleStream, and LongStream, but allowing them to use IntPredicate,
DoublePredicate, and LongPredicate respectively.

Thanks,

Jacob Glickman

[8u] Request for Approval : Backport of 8171049 : Era.getDisplayName doesn't work with non-IsoChronology

[Deepak Kejriwal]

Hi,

 

Please approve the backport of JDK-8171049 to 8u-dev.

 

Master bug :  https://bugs.openjdk.java.net/browse/JDK-8171049

Webrev for [8u-dev]:  http://cr.openjdk.java.net/~rpatil/8171049/webrev.00/

JDK Review Thread: 
http://mail.openjdk.java.net/pipermail/i18n-dev/2018-November/002682.html

 

 

Master Bug Change set: http://hg.openjdk.java.net/jdk/jdk/rev/8dfed4387312

Master Review Thread : 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-August/049037.html

 

Summary:

 

The fix was clean backport but new test file "TestEraDisplayName.java" added as 
part of fix is modified as per jdk 8 release:-  

. All test written in jdk 10 are written to verify the CLDR resource. 
Since, in case of jdk 8 "CLDR" is not default locale providers, added 
Djava.locale.providers=CLDR as JVM parameter to test. 

. Some resource keys in jdk 8 does not exist in both CLDR and JRE due 
to which Era.getDisplayName() returns numeric value. Modified such test cases 
accordingly

 

All the related testing is done and is a pass.

 

Regards,

Deepak

 

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Roger Riggs]

Hi Andrew,

If to achieve the performance or functional goals of the JEP hotspot 
changes are needed

they should be mentioned (no details needed) in the JEP.
It helps the reader understand the scope and impact of the change.

Regards, Roger


On 11/08/2018 04:10 AM, Andrew Dinn wrote:

On 07/11/18 17:12, Vladimir Kozlov wrote:

I am Lead for Hotspot [1]. Alan is Group Lead for core libs and he gave
review already.

I don't see any reference to Hotspot in JEP so I am not sure what to
review. Do you need any new optimizations/intrinsics in Hotspot for this
JEP?

Yes I do need some new intrinsics. I was not clear whether they needed
to be documented in the JEP. Perhaps you could advise?

n.b. If you need to know what is being proposed in order to answer that
I can point you at my prototype implementation. Details after the sig.


You need to ask Alan or Brian Goetz (as Area Lead) for endorsement
before 'Submitting' JEP [2].

Ok, will do once I know whether details of the intrinsics have to be
included. Thanks for your help.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

- 8<  8<  8<  8<  8<  8< ---

The basic operation to persist ByteBuffer changes is provided via a new
method of jdk.internal.misc.Unsafe which /is/ currently described in the
JEP:

   public void writebackMemory(long address, long length)

My prototype implements this method using 3 intrinsics, a pre-writeback
memory sync, a per-cache line force (executed in a loop) and a
post-writeback memory sync.

I also added a native method which allows the (cpu-specific) cache line
size to be retrieved at class init time.

Webrev:

   http://cr.openjdk.java.net/~adinn/pmem/webrev.04/

Unsafe changes:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/java.base/share/classes/jdk/internal/misc/Unsafe.java.udiff.html

In the underlying implementation of the intrinsics there are 3
corresponding new IR nodes, CacheWBNode, CacheWBPreSyncNode and
CacheWBPostSyncNode. They are matched by processor-specific ad file
rules to generate the required assembler

Intrinsics implementation:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/share/opto/memnode.hpp.udiff.html

http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/share/opto/library_call.cpp.udiff.html

Back end rules:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/aarch64/aarch64.ad.udiff.html


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/x86_64.ad.udiff.html

Assembler changes:

The assembler implementations are fairly straightforward. There is no
need for a pre sync on either AArch64 or x86_64. A post sync is always
needed on AArch64. It may not be needed on x86 depending on what type of
cache line flush the processor supports


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp.udiff.html


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/macroAssembler_x86.cpp.udiff.html

http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/assembler_x86.cpp.udiff.html

Re: Stream Method Proposal: long count(Predicate predicate)

[Jacob Glickman]

There was a typo in my last e-mail:

`.count("Java::equals")` should be `.count("Java"::equals)`

Thanks,

Jacob Glickman

On Thu, Nov 8, 2018 at 9:02 AM Jacob Glickman  wrote:

> Tagir,
>
> Nothing is wrong with it, but I think the addition of the convenience
> method(s) would help to improve readability in some cases. Personally, I'd
> much rather have the option of writing `.count("Java::equals")` than
> `.filter("Java"::equals).count()`. As Zheka stated, this type of
> convenience method could also be useful for findFirst, in addition to
> findAny, distinct, etc.
>
> Thanks,
>
> Jacob Glickman
>
> On Thu, Nov 8, 2018 at 4:46 AM Tagir Valeev  wrote:
>
>> What's wrong with `filter(predicate).count()`? Saving nine characters?
>>
>> With best regards,
>> Tagir Valeev.
>> On Thu, Nov 8, 2018 at 8:02 AM Jacob Glickman 
>> wrote:
>> >
>> >  Hello!
>> >
>> > I see myself having to often call count() as a terminal operation on a
>> > Stream immediately after performing a filter operation. How feasible
>> would
>> > it be to add an overloaded count() method that accepts a Predicate,
>> which
>> > it uses as a filter before returning the count of elements in the
>> Stream?
>> > If this is supported, I'd gladly create the webrev & tests for it!
>> >
>> > I suppose the method signature can be something along the lines of:
>> >
>> > long count(Predicate predicate)
>> >
>> > It would also seem reasonable to give this method to IntStream,
>> > DoubleStream, and LongStream, but allowing them to use IntPredicate,
>> > DoublePredicate, and LongPredicate respectively.
>> >
>> > Thanks,
>> >
>> > Jacob Glickman
>>
>

Re: Stream Method Proposal: long count(Predicate predicate)

[Jacob Glickman]

Tagir,

Nothing is wrong with it, but I think the addition of the convenience
method(s) would help to improve readability in some cases. Personally, I'd
much rather have the option of writing `.count("Java::equals")` than
`.filter("Java"::equals).count()`. As Zheka stated, this type of
convenience method could also be useful for findFirst, in addition to
findAny, distinct, etc.

Thanks,

Jacob Glickman

On Thu, Nov 8, 2018 at 4:46 AM Tagir Valeev  wrote:

> What's wrong with `filter(predicate).count()`? Saving nine characters?
>
> With best regards,
> Tagir Valeev.
> On Thu, Nov 8, 2018 at 8:02 AM Jacob Glickman  wrote:
> >
> >  Hello!
> >
> > I see myself having to often call count() as a terminal operation on a
> > Stream immediately after performing a filter operation. How feasible
> would
> > it be to add an overloaded count() method that accepts a Predicate, which
> > it uses as a filter before returning the count of elements in the Stream?
> > If this is supported, I'd gladly create the webrev & tests for it!
> >
> > I suppose the method signature can be something along the lines of:
> >
> > long count(Predicate predicate)
> >
> > It would also seem reasonable to give this method to IntStream,
> > DoubleStream, and LongStream, but allowing them to use IntPredicate,
> > DoublePredicate, and LongPredicate respectively.
> >
> > Thanks,
> >
> > Jacob Glickman
>

Re: Filesystem case sensitive check and java.io.File#equals

[Jaikiran Pai]

Thanks for the clarification, Alan.

-Jaikiran

On Thursday, November 8, 2018, Alan Bateman  wrote:
> On 08/11/2018 12:59, Jaikiran Pai wrote:
>>
>> A slightly related question - I used the example that Roger showed and
>> it mostly worked. However, Files.isSameFile throws a
>> java.nio.file.NoSuchFileException since the path2 doesn't exist (on a
>> case sensitive file system). I checked the javadoc of Files.isSameFile
>> and couldn't find a clear mention if this is expected. It does talk
>> about the existence checks that might be carried out in this
>> implementation, but doesn't mention that it will throw an exception in
>> the absence of either of the passed paths. Is this expected? Or should
>> the implementation internally catch this exception and return false?
>>
> IOException is correct, the methods in this API aren't specify all the
possible sub-classes. In a few places you will see specific exceptions
specified as "optional specific exception" where it make sense. We could
potentially improve the spec for the cannot access or does not exist cases
but hasn't been an issue to date.
>
> -Alan
>

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Alan Bateman]

On 07/11/2018 19:33, Igor Ignatyev wrote:

Hi Vicente,

I recall an (internal?) discussion about updating ASM somewhen in JDK 11TF, and 
AFAIR it was decided not to update ASM b/c nothing in JDK needs that, has it 
been changed? put somewhat differently, why are we doing this?

in any case, I don't like the changes in mlvm tests. I understand that ClassWriter 
has been significantly changed in ASM 7.0, so ClassWriterExt can't disable CP entries 
caching (at least not in the way it used to), but removing setCache* calls from the 
tests changed them and in some cases made them invalid as they don't test that they 
supposed to. therefore I'd prefer to leave all calls setCache* as-is, change 
setCache* implementation to throw an exception (similarly to the fix in JDK-8194826 
) and mark all tests which 
throw this exception w/ '@ignore 8194951' jtreg tag.


ClassWriterExt the MLVM tests have come in previous upgrades too. Has 
there been any discussion Remi or others on ASM to make it easier for 
the JDK to upgrade?


-Alan

Re: Filesystem case sensitive check and java.io.File#equals

[Alan Bateman]

On 08/11/2018 12:59, Jaikiran Pai wrote:

A slightly related question - I used the example that Roger showed and
it mostly worked. However, Files.isSameFile throws a
java.nio.file.NoSuchFileException since the path2 doesn't exist (on a
case sensitive file system). I checked the javadoc of Files.isSameFile
and couldn't find a clear mention if this is expected. It does talk
about the existence checks that might be carried out in this
implementation, but doesn't mention that it will throw an exception in
the absence of either of the passed paths. Is this expected? Or should
the implementation internally catch this exception and return false?

IOException is correct, the methods in this API aren't specify all the 
possible sub-classes. In a few places you will see specific exceptions 
specified as "optional specific exception" where it make sense. We could 
potentially improve the spec for the cannot access or does not exist 
cases but hasn't been an issue to date.


-Alan

Re: Filesystem case sensitive check and java.io.File#equals

[Jaikiran Pai]

A slightly related question - I used the example that Roger showed and
it mostly worked. However, Files.isSameFile throws a
java.nio.file.NoSuchFileException since the path2 doesn't exist (on a
case sensitive file system). I checked the javadoc of Files.isSameFile
and couldn't find a clear mention if this is expected. It does talk
about the existence checks that might be carried out in this
implementation, but doesn't mention that it will throw an exception in
the absence of either of the passed paths. Is this expected? Or should
the implementation internally catch this exception and return false?

For now, in my implementation, I have explicitly caught this
NoSuchFileException, around the Files.isSameFile statement and consider
it to imply the filesystem is case sensitive.

-Jaikiran


On 07/11/18 8:37 PM, Roger Riggs wrote:
> Hi Jaikiran,
>
> To check if two pathnames are the same file,
> java.nio.file.Files.isSameFile(path1, path2)
> is cleaner.  It uses the file system specific mechanisms to determine
> if the two paths
> refer to the identical file.  Traversing symbolic links, etc.
>
> Something like:
>             Path dir = ...;   // supply a directory to test a
> particular file system
>     Path path1 = Files.createTempFile(dir, "MixedCase", "");
>     Path path2 =
> Path.of(path1.toString().toLowerCase(Locale.US));
>     return Files.isSameFile(path1, path2);
>
> or similar...
>
> Roger
>
> On 11/07/2018 09:27 AM, Jaikiran Pai wrote:
>> Hi Alan,
>>
>> On 07/11/18 7:15 PM, Alan Bateman wrote:
>>> On 07/11/2018 13:13, Jaikiran Pai wrote:
 :


 My impression, based on that javadoc, was that the implementation of
 that API will use the underlying _filesystem_ to decide whether or not
 its case sensitive. However, my experiments on a macOS which is case
 insensitive and also a basic check of the implementation code in the
 JDK, shows that this uses a lexicographic check on the string
 representation of the paths. Is that what this javadoc means by
 "underlying system". I understand that there's a further sentence in
 that javadoc which says UNIX systems are case significant and Windows
 isn't. However, it wasn't fully clear to me that this API wouldn't
 delegate it to the underlying filesystem on the OS.
>>> Maybe the javadoc could be clearer but having an equals method
>>> delegate to the underlying file system would be very problematic
>>> (think URL equals or cases where the file path is relative or doesn't
>>> locate a file).
>> I see what you mean. About the javadoc, do you think it is worth
>> improving and if so, should I file an enhancement at bugs.java.com?
>>
 While we are at it, is there a better API that can be used to do
 such a
 case sensitivity check of the underlying filesystem? I checked the
 APIs
 in java.nio.file but couldn't find anything relevant. If there
 isn't any
 such API currently, is that something that can be introduced?

>>> This has been looked into a couple of times over the years,
>>> suggestions have included FileStore exposing a comparator that uses
>>> the rules of a specific underlying file system or volume. This turns
>>> out to be very complicated as it means looking beyond case sensitively
>>> issues and into topics such as case preservation, Unicode
>>> normalization forms, per volume case mapping tables, and handling of
>>> limits and truncation.
>> So I guess, that leaves us (the client applications) to rely on some
>> kind of file creation tricks like the one below to figure this out:
>>
>>      // create a temp file in a fresh directory
>>  final Path tmpDir = Files.createTempDirectory(null);
>>  final Path tmpFile = Files.createTempFile(tmpDir, null, null);
>>  tmpFile.toFile().deleteOnExit();
>>  tmpDir.toFile().deleteOnExit();
>>  // now check if a file with that same name but different
>> case is
>> considered to exist
>>  final boolean existsAsLowerCase =
>> Files.exists(Paths.get(tmpDir.toString(),
>> tmpFile.getFileName().toString().toLowerCase()));
>>  final boolean existsAsUpperCase =
>> Files.exists(Paths.get(tmpDir.toString(),
>> tmpFile.getFileName().toString().toUpperCase()));
>>  // if the temp file that we created is found to not exist in a
>> particular "case", then
>>  // the filesystem is case sensitive
>>  final boolean filesystemCaseSensitive = existsAsLowerCase ==
>> false || existsAsUpperCase == false;
>>
>> One final question - is there anywhere in the JDK code, where files are
>> auto created (for whatever purpose)? If such a construct exists, maybe
>> during the initialization of each FileStore implementation, it could do
>> such tricks (maybe in a much better and performant way) and figure out
>> this detail and then expose it some way? It feels hacky to do it in the
>> JDK, so I fully understand if such a construct won't be entertained.
>>
>>

Re: RFR: 8212794 IBM-964 and IBM-29626C are required for AIX default charset

[Volker Simonis]

Hi Ichiroh,

sorry, but unfortunately, this change is way beyond my level of expertise :)

You should try to get a review from Sherman or Alan.

Regards,
Volker
On Fri, Oct 26, 2018 at 3:33 PM Ichiroh Takiguchi
 wrote:
>
> Hello.
>
> Could you review the fix ?
>
> Bug:https://bugs.openjdk.java.net/browse/JDK-8212794
> Change: https://cr.openjdk.java.net/~itakiguchi/8212794/webrev.00/
>
> I'd like to obtain a sponsor for this issue.
>
>
> IBM964 charset and IBM29626C charset are required for default charset
> on AIX zh_TW and ja_JP locale.
> OpenJDK already has IBM964, but it could not be configured for default
> charset.
> IBM29626C is new one.
> (IBM33722 extends IBM29626C class)
>
> I knew IBM charsets would need to move to somewhere.
> The discussion was started by "Adding new IBM extended charsets". [1]
> But it's related default charset issue.
> Please put them inside of OpenJDK.
>
> About IBM964,
> Bhaktavatsal started the discussion [2].
> Sherman said that [3]
>the new model (open/make/data/charetmapping), instead of hard-coding
> the map
>into the source code.
>
> This fix only has small sized hard-coded mapping,
> IBM964/IBM29626C/IBM33722 refer other charsets conversion table
> which are using the new model.
> And class file is smaller then before.
> Still I used SimpleEUCEncoder class because it's stable.
> I think we may re-write it by IBM charsets renewal.
>
> Thanks,
> Ichiroh Takiguchi
> IBM Japan, Ltd.
>
> [1]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-July/054248.html
> [2]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/053050.html
> [3]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-May/053096.html
>

Re: 6516099: InputStream.skipFully(int k) to skip exactly k bytes

[Daniel Fuchs]

Hi Brian,

On 07/11/2018 23:37, Brian Burkhalter wrote:

http://hg.openjdk.java.net/jdk/jdk/file/44f34d2c3243/src/java.desktop/share/classes/com/sun/media/sound/AudioFloatInputStream.java#l99


The above class is not a descendent of InputStream. In any case, as 
InputStream and all its “direct known subclasses” specify that skip() 
returns “the actual number of bytes skipped” the foregoing behavior 
would I think be a bug.


Damn. You're right. My mistake. But if I'm not mistaken
again it's called from a class that implements InputStream
(that's how I ended up looking at it):

http://hg.openjdk.java.net/jdk/jdk/file/44f34d2c3243/src/java.desktop/share/classes/com/sun/media/sound/AudioFloatFormatConverter.java#l117

That said, it's altogether possible that the condition on
which -1 is returned is dead code and actually never happens.
That's hard to tell.


Not sure there's anything that could be done in such cases, except
encouraging such subclasses to provide their own implementation
of skipNBytes?


That’s probably true.


So FWIW my point was that there's nothing that you can really
guarantee in InputSteam::skipFully() if a subclass implementation of
skip() uses negative number to e.g. signal abnormal conditions
(such as EOF?). And I wonder if that should warrant a disclaimer
in the API doc of InputStream::skipFully (maybe an @implSpec?)


best regards

-- daniel



Thanks,

Brian

Re: Stream Method Proposal: long count(Predicate predicate)

[Tagir Valeev]

What's wrong with `filter(predicate).count()`? Saving nine characters?

With best regards,
Tagir Valeev.
On Thu, Nov 8, 2018 at 8:02 AM Jacob Glickman  wrote:
>
>  Hello!
>
> I see myself having to often call count() as a terminal operation on a
> Stream immediately after performing a filter operation. How feasible would
> it be to add an overloaded count() method that accepts a Predicate, which
> it uses as a filter before returning the count of elements in the Stream?
> If this is supported, I'd gladly create the webrev & tests for it!
>
> I suppose the method signature can be something along the lines of:
>
> long count(Predicate predicate)
>
> It would also seem reasonable to give this method to IntStream,
> DoubleStream, and LongStream, but allowing them to use IntPredicate,
> DoublePredicate, and LongPredicate respectively.
>
> Thanks,
>
> Jacob Glickman

Re: RFR: 8207851 JEP Draft: Support ByteBuffer mapped over non-volatile memory

[Andrew Dinn]

On 07/11/18 17:12, Vladimir Kozlov wrote:
> I am Lead for Hotspot [1]. Alan is Group Lead for core libs and he gave
> review already.
> 
> I don't see any reference to Hotspot in JEP so I am not sure what to
> review. Do you need any new optimizations/intrinsics in Hotspot for this
> JEP?

Yes I do need some new intrinsics. I was not clear whether they needed
to be documented in the JEP. Perhaps you could advise?

n.b. If you need to know what is being proposed in order to answer that
I can point you at my prototype implementation. Details after the sig.

> You need to ask Alan or Brian Goetz (as Area Lead) for endorsement
> before 'Submitting' JEP [2].

Ok, will do once I know whether details of the intrinsics have to be
included. Thanks for your help.

regards,


Andrew Dinn
---
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

- 8<  8<  8<  8<  8<  8< ---

The basic operation to persist ByteBuffer changes is provided via a new
method of jdk.internal.misc.Unsafe which /is/ currently described in the
JEP:

  public void writebackMemory(long address, long length)

My prototype implements this method using 3 intrinsics, a pre-writeback
memory sync, a per-cache line force (executed in a loop) and a
post-writeback memory sync.

I also added a native method which allows the (cpu-specific) cache line
size to be retrieved at class init time.

Webrev:

  http://cr.openjdk.java.net/~adinn/pmem/webrev.04/

Unsafe changes:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/java.base/share/classes/jdk/internal/misc/Unsafe.java.udiff.html

In the underlying implementation of the intrinsics there are 3
corresponding new IR nodes, CacheWBNode, CacheWBPreSyncNode and
CacheWBPostSyncNode. They are matched by processor-specific ad file
rules to generate the required assembler

Intrinsics implementation:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/share/opto/memnode.hpp.udiff.html

http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/share/opto/library_call.cpp.udiff.html

Back end rules:


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/aarch64/aarch64.ad.udiff.html


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/x86_64.ad.udiff.html

Assembler changes:

The assembler implementations are fairly straightforward. There is no
need for a pre sync on either AArch64 or x86_64. A post sync is always
needed on AArch64. It may not be needed on x86 depending on what type of
cache line flush the processor supports


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp.udiff.html


http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/macroAssembler_x86.cpp.udiff.html

http://cr.openjdk.java.net/~adinn/pmem/webrev.04/src/hotspot/cpu/x86/assembler_x86.cpp.udiff.html