RFR (XS) 8148730: Add @since tags in new String concat APIs

[Aleksey Shipilev]

Hi,

Please review this tiny update, that puts @since 9 tags over new Indify
String Concat JDK APIs:
  http://cr.openjdk.java.net/~shade/8148730/webrev.01/

Cheers,
-Aleksey

Re: [PING] PoC for JDK-4347142: Need method to set Password protection to Zip entries

[KUBOTA Yuji]

Hi Sherman and all,

Could you please let know your thought and the past case about AES?

Thanks,
Yuji


2016-01-08 0:01 GMT+09:00 KUBOTA Yuji :
> Hi Sherman,
>
> Thank you for sharing!
>
> 2016-01-07 4:04 GMT+09:00 Xueming Shen :
>> The reason that I'm not convinced that we really need a public interface of
>> ZipCryption here
>> is I don't know how useful/helpful/likely it would be going forward that
>> someone might really
>> use this interface to implement other encryption(s), especially the pkware
>> proprietary one,
>> I doubt it might be not that straightforward.
>
> In this proposal, we aim to support "traditional" because most people need it
> in secure environment. BTW, could you please share the reason why you did
> not support WinZip AES? Do you have a plan to support in the future?
>
> If you can share the reason, we want to decide the way of implementation with
> consideration for your information. I think we can implement by two
> way as below.
>
> 1. Implementing by reference to
> http://cr.openjdk.java.net/~sherman/zipmisc/ZipFile.java
> This is good simply API. If we need to implement other encryption(s),
> try to refactor it.
>
> 2. Implementing with a package private interface of ZipCryption for next step.
> This has two problems as your advice.
>
> We agree with that the "encryption" and "compression" should be
> separated logically.
> However, current implementation compress the encrypted data, and buffering it.
> It is too tightly-coupled, so we need refactoring to separate the
> managing buffer
> processing and the stream processing of InflaterInputStream /
> DeflaterOutputStream.
>
> About "push back the bytes belong to next entry", we think
> InflaterInputStream.originBuf
> of our PoC do not required the needed info. Do this implements have problem?
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.00/src/java.base/share/classes/java/util/zip/InflaterInputStream.java.cdiff.html
>
> Thanks,
> Yuji
>
>> In fact I did have a draft implementation that supports WinZip AES about 5-6
>> years ago :-)
>> (which also supports compression methods bzip and lzma, btw)  Here is the
>> top class, It appears
>> a general interface might not be that helpful and it might be ideal to
>> simply implement it inside
>> the JDK, as what is proposed here, when it's really desired.
>>
>> http://cr.openjdk.java.net/~sherman/zipmisc/ZipFile.java
>>
>> It is a ZipFile based implementation, so it does not have the headache that
>> ZipInputStream has,
>> such as to push back the bytes belong to next entry, since the loc might not
>> have the needed
>> info regarding the size/csize in stream mode.
>>
>> From abstract point of view. The "encryption" and "compression" are
>> different layers, it would
>> be ideal to have them in separate classes logically, instead of mixing the
>> encryption into
>> compression. Sure, it might be convenient and probably may have better
>> performance to mix
>> them in certain use scenario, but the "encryption" should never appear in
>> the public interface
>> of those compression classes. Package private interface should be fine, if
>> have to.
>>
>> -Sherman
>>
>>
>>>
>>> 2016-01-06 7:10 GMT+09:00 Xueming Shen :

 it appears that instead of adding "password" specific method to these
 classes directly, it might be more appropriate to extend the ZipEntry
 class
 for such "password" functionality. For example, with a pair of new
 methods

 boolean ZipEntry.isTraditionalEncryption().
 void ZipEntry.setTraditionalEncryption(String password);
>>>
>>> Thanks advice, I agree. We should re-design the API to extend the
>>> ZipEntry class.
>>>
 The encryption support should/can be added naturally/smoothly with
 ZipFile.getInputStream(e), ZipInputstream and
 ZipOutputStream.putNextEntry(e),
 with no extra new method in these two classes. The implementation checks
 the flag (bit0, no bit 6) first and then verifies the password, as an
 implementation details.
>>>
>>> Agree. For this proposal, we aim to support only traditional
>>> encryption. So I think we should also check bit 6.
>>>
 For ZipFile and ZipInputStream, we can add note to the api doc to force
 the
 invoker to check if the returned ZipEntry indicates it's an encrypted
 entry.
 If yes, it must to set the appropriate password to the returned ZipEntry
 via
 ZipEntry.setTraditionalEncryption(password); before reading any byte from
 the input stream.
>>>
>>> Yes, we have to add note the flow of codes to the JavaDoc.
>>>
 Again, we should not have any "encryption" related public field/method in
 DeflaterOutputStream/InflaterInputStream. Ideally these two classes
 really
 should not be aware of "encryption" at all.
>>>
>>> Agree, but I think we might be faced technical difficulty about a
>>> processing between zlib and the internal buffer of InflaterInputStream
>>> / DeflaterOutputStream. Please give us time to implemen

Re: RFR 9: 8146773 java/lang/ref/CleanerTest.java CleanerTest.testRefSubtypes() fails

[Daniel Fuchs]

Hi Roger,

Looks good to me - but I'd suggest to call fullGC at least once
before waiting on the semaphore (e.g. add a call to whitebox.fullGC()
when entering checkCleaned() before line 285).
That might maximize the chances that the referred object will be
GC'ed before you start waiting on the semaphore.

Also I wonder whether you should raise the timeout in tryAcquire, 10ms
is not much - and you want to give enough time so that the cleaner's
thread is scheduled and calls the cleanup action...
Maybe you could make that timeout depending on the timeoutFactor
as well.

best regards,

-- daniel

On 29/01/16 19:47, Roger Riggs wrote:

Please review a fix for two test issues.  When run with -Xcomp and other
switches that change
GC behavior; the CleanerTest was not giving enough to time to see the
result of cleaning.
Also, added an adjustment of the number of cycles by the timeoutFactor.

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-cleanertest-8146773/

8146773: java/lang/ref/CleanerTest.java CleanerTest.testRefSubtypes() fails
8148352: CleanerTest fails: Cleanable should have been freed

Thanks, Roger

Re: RFR (XS) 8148730: Add @since tags in new String concat APIs

[Andrej Golovnin]

Hi Aleksej,

is it really needed to add the "@since 9" tag to the
methods/constructors when the whole class is since JDK 9? As far as I
know the @since tag should be only added to class members introduced
in a later version than the class.

Best regards,
Andrej Golovnin

On Mon, Feb 1, 2016 at 10:21 AM, Aleksey Shipilev
 wrote:
> Hi,
>
> Please review this tiny update, that puts @since 9 tags over new Indify
> String Concat JDK APIs:
>   http://cr.openjdk.java.net/~shade/8148730/webrev.01/
>
> Cheers,
> -Aleksey
>

Re: RFR (XS) 8148730: Add @since tags in new String concat APIs

[Aleksey Shipilev]

Hi,

I don't know, you tell me -- you have the tools that enforce
@since-ness! Certainly, we can put @since on classes only, if that is
acceptable.

-Aleksey

On 02/01/2016 12:32 PM, Andrej Golovnin wrote:
> Hi Aleksej,
> 
> is it really needed to add the "@since 9" tag to the
> methods/constructors when the whole class is since JDK 9? As far as I
> know the @since tag should be only added to class members introduced
> in a later version than the class.
> 
> Best regards,
> Andrej Golovnin
> 
> On Mon, Feb 1, 2016 at 10:21 AM, Aleksey Shipilev
>  wrote:
>> Hi,
>>
>> Please review this tiny update, that puts @since 9 tags over new Indify
>> String Concat JDK APIs:
>>   http://cr.openjdk.java.net/~shade/8148730/webrev.01/
>>
>> Cheers,
>> -Aleksey
>>

Re: RFR (XS) 8148730: Add @since tags in new String concat APIs

[Alan Bateman]

On 01/02/2016 09:35, Aleksey Shipilev wrote:

Hi,

I don't know, you tell me -- you have the tools that enforce
@since-ness! Certainly, we can put @since on classes only, if that is
acceptable.

-Aleksey

@since 9 on the class should be sufficient here. If it becomes necessary 
to add methods in Java SE 10 then the new methods will get @since.


-Alan

Re: RFR (XS) 8148730: Add @since tags in new String concat APIs

[Andrej Golovnin]

Hi Aleksej,

> I don't know, you tell me -- you have the tools that enforce
> @since-ness! Certainly, we can put @since on classes only, if that is
> acceptable.

I don't have such tools. But when I write JavaDocs I try to follow the
recommendations from this article:

http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html

And here is the excerpt from the article regarding the @since tag:

When a class (or interface) is introduced, specify one @since tag in
its class description and no @since tags in the members. Add an @since
tag only to members added in a later version than the class. This
minimizes the number of @since tags.

Best regards,
Andrej Golovnin

Re: RFR (XS) 8148730: Add @since tags in new String concat APIs

[Aleksey Shipilev]

On 02/01/2016 12:44 PM, Andrej Golovnin wrote:
>> I don't know, you tell me -- you have the tools that enforce
>> @since-ness! Certainly, we can put @since on classes only, if that is
>> acceptable.
> 
> I don't have such tools. 

Apologies, I confused you with another Andrey :)

Sure, let's do class-only:
  http://cr.openjdk.java.net/~shade/8148730/webrev.02/

Cheers,
-Aleksey

Re: RFR (XS) 8148730: Add @since tags in new String concat APIs

[Alan Bateman]

On 01/02/2016 09:56, Aleksey Shipilev wrote:

:

Sure, let's do class-only:
   http://cr.openjdk.java.net/~shade/8148730/webrev.02/


Looks good.

Re: RFR (XS) 8148730: Add @since tags in new String concat APIs

[Andrej Golovnin]

> Apologies, I confused you with another Andrey :)
>
> Sure, let's do class-only:
>   http://cr.openjdk.java.net/~shade/8148730/webrev.02/

No problem, Aleksey. Looks much better now. :)

Best regards,
Andrej Golovnin

Re: RFR (XS) 8148730: Add @since tags in new String concat APIs

[Aleksey Shipilev]

On 02/01/2016 01:03 PM, Alan Bateman wrote:
> On 01/02/2016 09:56, Aleksey Shipilev wrote:
>> :
>>
>> Sure, let's do class-only:
>>http://cr.openjdk.java.net/~shade/8148730/webrev.02/
>>
> Looks good.

Thanks guys, pushed.

-Aleksey

Re: RFR 9: 8146773 java/lang/ref/CleanerTest.java CleanerTest.testRefSubtypes() fails

[Roger Riggs]

Hi Daniel,

On 2/1/2016 4:24 AM, Daniel Fuchs wrote:

Hi Roger,

Looks good to me - but I'd suggest to call fullGC at least once
before waiting on the semaphore (e.g. add a call to whitebox.fullGC()
when entering checkCleaned() before line 285).
That might maximize the chances that the referred object will be
GC'ed before you start waiting on the semaphore.

Sure, I moved the GC to the beginning of the loop


Also I wonder whether you should raise the timeout in tryAcquire, 10ms
is not much - and you want to give enough time so that the cleaner's
thread is scheduled and calls the cleanup action...
Maybe you could make that timeout depending on the timeoutFactor
as well.
The number of cycles was scaled by the timeoutFactor but it is more 
intuitive to

scale the timeout itself.

checkCleaned is invoked for cases where the cleanup function will not be 
called

so increasing the timeout will just increase the running time of the test.
The refactoring was done to allow more time in cases where the cleaning 
is expected
and not wait too long in cases where it is not expected.  But the 
overall running time

of the test is < 10s.

Webrev updated in place.

Thanks, Roger




best regards,

-- daniel

On 29/01/16 19:47, Roger Riggs wrote:

Please review a fix for two test issues. When run with -Xcomp and other
switches that change
GC behavior; the CleanerTest was not giving enough to time to see the
result of cleaning.
Also, added an adjustment of the number of cycles by the timeoutFactor.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-cleanertest-8146773/

8146773: java/lang/ref/CleanerTest.java CleanerTest.testRefSubtypes() 
fails

8148352: CleanerTest fails: Cleanable should have been freed

Thanks, Roger

Re: RFR 9: 8146773 java/lang/ref/CleanerTest.java CleanerTest.testRefSubtypes() fails

[Daniel Fuchs]

On 01/02/16 15:47, Roger Riggs wrote:

The number of cycles was scaled by the timeoutFactor but it is more
intuitive to
scale the timeout itself.

checkCleaned is invoked for cases where the cleanup function will not be
called
so increasing the timeout will just increase the running time of the test.
The refactoring was done to allow more time in cases where the cleaning
is expected
and not wait too long in cases where it is not expected.  But the
overall running time
of the test is < 10s.

Webrev updated in place.


Hi Roger,

Looks good to me.

-- daniel

RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Roger Riggs]

Please review an API addition to handle signals such as SIGINT, SIGHUP, 
and SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use 
case for

interactive applications that need to handle Control-C and other signals.

The new java.util.Signal class provides a settable primary signal 
handler and a default
signal handler.  The primary signal handler can be unregistered and 
handling is restored
to the default signal handler.  System initialization registers default 
signal handlers

to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API requires
a permission if a SecurityManager is set.

The sun.misc.Signal implementation is modified to be layered on a common
thread and dispatch mechanism. The VM handling of native signals is not 
affected.
The command option to reduce signal use by the runtime with -Xrs is 
unmodified.


The changes to hotspot are minimal to rename the hardcoded callback to 
the Java

Signal dispatcher.

Please review and comment on the API and implementation.

javadoc:
  http://cr.openjdk.java.net/~rriggs/signal-doc/

Webrev:
jdk:  http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8087286

JEP 260:
  https://bugs.openjdk.java.net/browse/JDK-8132928

Thanks, Roger

RFR 9: 8143016 : java/time/test/java/time/TestClock_System.java failed intermittently

[Roger Riggs]

Please review a test improvement to address an intermittent test failure in
the java.time System Clock test for microsecond support.

webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-micros-8143016/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8143016

Thanks, Roger

Re: RFR 9: 8143016 : java/time/test/java/time/TestClock_System.java failed intermittently

[Lance Andersen]

Looks OK Roger.
On Feb 1, 2016, at 11:41 AM, Roger Riggs  wrote:

> Please review a test improvement to address an intermittent test failure in
> the java.time System Clock test for microsecond support.
> 
> webrev:
>  http://cr.openjdk.java.net/~rriggs/webrev-micros-8143016/
> 
> Issue:
>  https://bugs.openjdk.java.net/browse/JDK-8143016
> 
> Thanks, Roger
> 



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Chris Hegarty]

Thank you for taking this on Roger.

An initial question to help my understanding. [ I think I need to
understand this, before I can comment further on the API ]

I'm a little confused about how the default handler is supposed to
work, so I looked at the implementation and become even more confused.
The consumer is registered on a per instance basis, and the instance
is added to the static map when it is created. So it appears that the
registered handler is not dependent on when the last register() is
called, but when the last instance of a Signal for a specific signal
is created. Is this intended?

-Chris.

On 01/02/16 16:02, Roger Riggs wrote:

Please review an API addition to handle signals such as SIGINT, SIGHUP,
and SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use
case for
interactive applications that need to handle Control-C and other signals.

The new java.util.Signal class provides a settable primary signal
handler and a default
signal handler.  The primary signal handler can be unregistered and
handling is restored
to the default signal handler.  System initialization registers default
signal handlers
to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API
requires
a permission if a SecurityManager is set.

The sun.misc.Signal implementation is modified to be layered on a common
thread and dispatch mechanism. The VM handling of native signals is not
affected.
The command option to reduce signal use by the runtime with -Xrs is
unmodified.

The changes to hotspot are minimal to rename the hardcoded callback to
the Java
Signal dispatcher.

Please review and comment on the API and implementation.

javadoc:
   http://cr.openjdk.java.net/~rriggs/signal-doc/

Webrev:
jdk:  http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8087286

JEP 260:
   https://bugs.openjdk.java.net/browse/JDK-8132928

Thanks, Roger

Re: RFR 9: 8143016 : java/time/test/java/time/TestClock_System.java failed intermittently

[Stephen Colebourne]

Fine by me
Stephen

On 1 February 2016 at 16:41, Roger Riggs  wrote:
> Please review a test improvement to address an intermittent test failure in
> the java.time System Clock test for microsecond support.
>
> webrev:
>   http://cr.openjdk.java.net/~rriggs/webrev-micros-8143016/
>
> Issue:
>   https://bugs.openjdk.java.net/browse/JDK-8143016
>
> Thanks, Roger
>

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

> On Jan 30, 2016, at 12:00 AM, Alan Bateman  wrote:
> 
> 
> On 29/01/2016 17:39, Paul Sandoz wrote:
>> :
>> Alan’s point is that traversing using entries()/stream() always returns the 
>> versioned entries (if any) rather than all entries, thus in a sense filters.
>> 
>> My assumption was the traversal should by default be consistent with a calls 
>> to getEntry, thus:
>> 
>>  jarFile.stream().forEach(e -> {
>> JarEntry je = jarFile.getJarEntry(e.getName());
>> assert e.equals(je);
>>  });
>> 
>> There might need to be another stream method that returns all entries.
>> 
> Right, I'm mostly just wondering if entries()/streams() should override the 
> entries in the stream with versioned entries and filter out the 
> META-INF/versions/ tree.

I don’t think so.  That kind of behavior might be difficult to understand.  
Returning all the entries provides some flexibility.  One can write code like 
this:

jarfile.stream().map(JarEntry::getName).filter(s -> 
!s.startsWith(“META-INF”)).map(JarFile::getJarEntry).etc

to get the versioned results for any version you specify when constructing the 
JarFile.

> 
> If I've gone to trouble of specifying the a Release then it seems the right 
> thing to do. On the other hand, it comes at a cost and there will be 
> use-cases like "get the names of all entries" that would be more efficient to 
> just build on the current entries()/stream(). I'm loath to suggest this might 
> need a new method but it might be one of the options to consider here. 
> Minimally there is a javadoc to specify on how these methods behave when the 
> JAR is multi-release and opened by specifying a release.

How’s this?

diff -r 68867430065b src/java.base/share/classes/java/util/jar/JarFile.java
--- a/src/java.base/share/classes/java/util/jar/JarFile.javaFri Jan 29 
12:34:44 2016 -0800
+++ b/src/java.base/share/classes/java/util/jar/JarFile.javaMon Feb 01 
09:48:05 2016 -0800
@@ -576,9 +576,11 @@
 }
 
 /**
- * Returns an enumeration of the jar file entries.
+ * Returns an enumeration of all the jar file entries.  Constructing this
+ * JarFile with the {@link JarFile#JarFile(File, boolean, int, Release)}
+ * constructor does not modify the behavior of this method.
  *
- * @return an enumeration of the jar file entries
+ * @return an enumeration of the all jar file entries
  * @throws IllegalStateException
  * may be thrown if the jar file has been closed
  */
@@ -587,11 +589,13 @@
 }
 
 /**
- * Returns an ordered {@code Stream} over the jar file entries.
+ * Returns an ordered {@code Stream} over all the jar file entries.
  * Entries appear in the {@code Stream} in the order they appear in
- * the central directory of the jar file.
+ * the central directory of the jar file.  Constructing this
+ * JarFile with the {@link JarFile#JarFile(File, boolean, int, Release)}
+ * constructor does not modify the behavior of this method.
  *
- * @return an ordered {@code Stream} of entries in this jar file
+ * @return an ordered {@code Stream} of all entries in this jar file
  * @throws IllegalStateException if the jar file has been closed
  * @since 1.8
  */

RFR: jsr166 jdk9 integration wave 4

[Martin Buchholz]

After much debate on what to do when CompleteableFuture.whenComplete
encounters an exception in both the source and the action, we chose
what was acceptable to the most people - add the action's exception to
the source exception as a suppressed exception.  And added usage
guidelines.  And gave handle "top billing" over whenComplete.

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

and the usual miscellaneous improvements.

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Gerard Ziemski]

hi Roger,

I love that we are adding this Signal object. I have several questions, but 
please do not take them as criticism, I’m just seeking clarification and 
providing feedback:


#1 Re: "Consumers for signals that are unknown or reserved by the Java 
implementation can not be registered.”

- Why don't we want to allow unknown signals? This will make us have to update 
our implementation if we want to support new or platform specific signals in 
the future. 


#2 Re: "java.util.Signal.raise()”

- That API raises the signal in the current process, but what about sending a 
signal to another process for interprocess communication?


#3 Re: "Signal.of("SIGINT”)”

- Is this a factory method that returns the same object if called more than 
once?


#4 Re: "public boolean unregister(Consumer consumer)”

- Why is this API returning a value? Wouldn’t having a Signal API like “public 
Consumer getConsumer()” be more flexible?


#5 Re: "public void registerDefault(Consumer consumer)”

- Do we really need this API? Can’t the same be achieved with the plain vanilla 
“public void register(Consumer consumer)” I guess I don’t really see 
what makes this API special.


cheers

> On Feb 1, 2016, at 10:02 AM, Roger Riggs  wrote:
> 
> Please review an API addition to handle signals such as SIGINT, SIGHUP, and 
> SIGTERM.
> This JEP 260 motivated alternative to sun.misc.Signal supports the use case 
> for
> interactive applications that need to handle Control-C and other signals.
> 
> The new java.util.Signal class provides a settable primary signal handler and 
> a default
> signal handler.  The primary signal handler can be unregistered and handling 
> is restored
> to the default signal handler.  System initialization registers default 
> signal handlers
> to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API requires
> a permission if a SecurityManager is set.
> 
> The sun.misc.Signal implementation is modified to be layered on a common
> thread and dispatch mechanism. The VM handling of native signals is not 
> affected.
> The command option to reduce signal use by the runtime with -Xrs is 
> unmodified.
> 
> The changes to hotspot are minimal to rename the hardcoded callback to the 
> Java
> Signal dispatcher.
> 
> Please review and comment on the API and implementation.
> 
> javadoc:
>  http://cr.openjdk.java.net/~rriggs/signal-doc/
> 
> Webrev:
> jdk:  http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
> hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/
> 
> Issue:
>   https://bugs.openjdk.java.net/browse/JDK-8087286
> 
> JEP 260:
>  https://bugs.openjdk.java.net/browse/JDK-8132928
> 
> Thanks, Roger
> 
> 

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Roger Riggs]

Hi Chris,

On 2/1/2016 11:47 AM, Chris Hegarty wrote:

Thank you for taking this on Roger.

An initial question to help my understanding. [ I think I need to
understand this, before I can comment further on the API ]

I'm a little confused about how the default handler is supposed to
work, so I looked at the implementation and become even more confused.
The consumer is registered on a per instance basis, and the instance
is added to the static map when it is created. So it appears that the
registered handler is not dependent on when the last register() is
called, but when the last instance of a Signal for a specific signal
is created. Is this intended?

The Signal instance delegates to a singleton SignalImpl instance
(based on the name/number).  I chose to delegate from Signal
to SignalImpl to be able to put SignalImpl in a separate package that
could be exported to the implementation of sun.misc.Signal as needed.
Otherwise, java.util.Signal would only be able to export only its public 
interface

and s.m.Signal needs more functions to be available.

The current and default consumers are stored in the singleton SignalImpl 
objects

that are used for the dispatch and calling of the consumer.

The SignalImpl instances are create on first use.  Each call to register
re-registers the SignalImpl with the VM to ensure it will be the current 
handler.


Roger




-Chris.

On 01/02/16 16:02, Roger Riggs wrote:

Please review an API addition to handle signals such as SIGINT, SIGHUP,
and SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use
case for
interactive applications that need to handle Control-C and other 
signals.


The new java.util.Signal class provides a settable primary signal
handler and a default
signal handler.  The primary signal handler can be unregistered and
handling is restored
to the default signal handler.  System initialization registers default
signal handlers
to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API
requires
a permission if a SecurityManager is set.

The sun.misc.Signal implementation is modified to be layered on a common
thread and dispatch mechanism. The VM handling of native signals is not
affected.
The command option to reduce signal use by the runtime with -Xrs is
unmodified.

The changes to hotspot are minimal to rename the hardcoded callback to
the Java
Signal dispatcher.

Please review and comment on the API and implementation.

javadoc:
   http://cr.openjdk.java.net/~rriggs/signal-doc/

Webrev:
jdk:  http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8087286

JEP 260:
   https://bugs.openjdk.java.net/browse/JDK-8132928

Thanks, Roger

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Xueming Shen]

On 02/01/2016 09:58 AM, Steve Drach wrote:

On Jan 30, 2016, at 12:00 AM, Alan Bateman  wrote:


On 29/01/2016 17:39, Paul Sandoz wrote:

:
Alan’s point is that traversing using entries()/stream() always returns the 
versioned entries (if any) rather than all entries, thus in a sense filters.

My assumption was the traversal should by default be consistent with a calls to 
getEntry, thus:

  jarFile.stream().forEach(e ->  {
 JarEntry je = jarFile.getJarEntry(e.getName());
 assert e.equals(je);
  });

There might need to be another stream method that returns all entries.


Right, I'm mostly just wondering if entries()/streams() should override the 
entries in the stream with versioned entries and filter out the 
META-INF/versions/ tree.

I don’t think so.  That kind of behavior might be difficult to understand.  
Returning all the entries provides some flexibility.  One can write code like 
this:

jarfile.stream().map(JarEntry::getName).filter(s ->  
!s.startsWith(“META-INF”)).map(JarFile::getJarEntry).etc

to get the versioned results for any version you specify when constructing the 
JarFile.


The current specification treats those class files under meta-inf/releases like
kind of "metadata" of those base entries. Ideally those files should not even
be individual "files", but part of their corresponding entries. The consumer of
the MR-Jar should not need to be aware of these version-ed entries at all to use
this MR-jar file to load classes/resources. From this point of view, these 
entries
probably should be "invisible" from entries()/stream(), when the jar file is 
opened
with "version-enabled". And all returned entries should have all their "data"
(size, csize, timestamps, crc ...) pointed to the corresponding version-ed 
entries,
withe the only exception is the "name".

On the other hand it might be desired to keep JarFile.entries()/stream() asis to
match their "zip file" perspective, to return "all" raw entries. Then it might 
also
be desired to have an alternative "versioned streamVersion()" ...

something like

public Stream stream(Release r); ?

-sherman





If I've gone to trouble of specifying the a Release then it seems the right thing to do. 
On the other hand, it comes at a cost and there will be use-cases like "get the 
names of all entries" that would be more efficient to just build on the current 
entries()/stream(). I'm loath to suggest this might need a new method but it might be one 
of the options to consider here. Minimally there is a javadoc to specify on how these 
methods behave when the JAR is multi-release and opened by specifying a release.

How’s this?

diff -r 68867430065b src/java.base/share/classes/java/util/jar/JarFile.java
--- a/src/java.base/share/classes/java/util/jar/JarFile.javaFri Jan 29 
12:34:44 2016 -0800
+++ b/src/java.base/share/classes/java/util/jar/JarFile.javaMon Feb 01 
09:48:05 2016 -0800
@@ -576,9 +576,11 @@
  }

  /**
- * Returns an enumeration of the jar file entries.
+ * Returns an enumeration of all the jar file entries.  Constructing this
+ * JarFile with the {@link JarFile#JarFile(File, boolean, int, Release)}
+ * constructor does not modify the behavior of this method.
   *
- * @return an enumeration of the jar file entries
+ * @return an enumeration of the all jar file entries
   * @throws IllegalStateException
   * may be thrown if the jar file has been closed
   */
@@ -587,11 +589,13 @@
  }

  /**
- * Returns an ordered {@code Stream} over the jar file entries.
+ * Returns an ordered {@code Stream} over all the jar file entries.
   * Entries appear in the {@code Stream} in the order they appear in
- * the central directory of the jar file.
+ * the central directory of the jar file.  Constructing this
+ * JarFile with the {@link JarFile#JarFile(File, boolean, int, Release)}
+ * constructor does not modify the behavior of this method.
   *
- * @return an ordered {@code Stream} of entries in this jar file
+ * @return an ordered {@code Stream} of all entries in this jar file
   * @throws IllegalStateException if the jar file has been closed
   * @since 1.8
   */

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Roger Riggs]

Hi Gerard,

On 2/1/2016 1:58 PM, Gerard Ziemski wrote:

hi Roger,

I love that we are adding this Signal object. I have several questions, but 
please do not take them as criticism, I’m just seeking clarification and 
providing feedback:


#1 Re: "Consumers for signals that are unknown or reserved by the Java 
implementation can not be registered.”

- Why don't we want to allow unknown signals? This will make us have to update 
our implementation if we want to support new or platform specific signals in 
the future.

The statement was aimed primarily at the Java Signal API; there is quite 
a bit of detail
oriented code in the VM to initialize and handle signals. Most of it is 
agnostic to the signal number
and would just pass it through.  If a signal is not supported by the OS 
(think SIGHUP on Windows)
that should bubble up as being not available.  The 'cannot be 
registered' might be re-worded to say

it throws an exception, as the method javadoc does.

The set of signals is a pretty slow moving target so updating 
implementations should not be a big issue.

#2 Re: "java.util.Signal.raise()”

- That API raises the signal in the current process, but what about sending a 
signal to another process for interprocess communication?
I left that for a separate issue but would be a straight-forward 
addition to java.lang.ProcessHandle/Process.



#3 Re: "Signal.of("SIGINT”)”

- Is this a factory method that returns the same object if called more than 
once?

Maybe, maybe not, why would it matter.
The real state is encapsulate is in the SignalImpl instances which are 
singletons per signal.
I was trying to keep the Signal object stateless to allow it to be a 
value-type and lighter weight

some day.



#4 Re: "public boolean unregister(Consumer consumer)”

- Why is this API returning a value? Wouldn’t having a Signal API like “public 
Consumer getConsumer()” be more flexible?


The return value reports whether it unregistered the specific consumer.  
If it was not
the concurrently registered the caller might want to know it was 
currently registered.

I expect the return would mostly be ignored.

The getConsumer()/unregister consumer pair would be vulnerable to race 
conditions

and require some external locking to get predictable behavior.




#5 Re: "public void registerDefault(Consumer consumer)”

- Do we really need this API? Can’t the same be achieved with the plain vanilla 
“public void register(Consumer consumer)” I guess I don’t really see 
what makes this API special.
The java runtime currently registers termination handler to cleanly 
shutdown if someone types control-c.
It is useful to be able to remove the application provided signal 
handler and be able to restore the

system defaults.
This API could be hidden as a pure implementation detail.  So 
unregistering the signal handler

would always restore the appropriate system ones.

Thanks for the review and comments, Roger




cheers


On Feb 1, 2016, at 10:02 AM, Roger Riggs  wrote:

Please review an API addition to handle signals such as SIGINT, SIGHUP, and 
SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use case for
interactive applications that need to handle Control-C and other signals.

The new java.util.Signal class provides a settable primary signal handler and a 
default
signal handler.  The primary signal handler can be unregistered and handling is 
restored
to the default signal handler.  System initialization registers default signal 
handlers
to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API requires
a permission if a SecurityManager is set.

The sun.misc.Signal implementation is modified to be layered on a common
thread and dispatch mechanism. The VM handling of native signals is not 
affected.
The command option to reduce signal use by the runtime with -Xrs is unmodified.

The changes to hotspot are minimal to rename the hardcoded callback to the Java
Signal dispatcher.

Please review and comment on the API and implementation.

javadoc:
  http://cr.openjdk.java.net/~rriggs/signal-doc/

Webrev:
jdk:  http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8087286

JEP 260:
  https://bugs.openjdk.java.net/browse/JDK-8132928

Thanks, Roger

RFR (S) 8148787: StringConcatFactory exactness check produces bad bytecode when a non-arg concat is requested

[Aleksey Shipilev]

Hi,

Please review the fix for a corner case in StringConcatFactory exactness
check, which produces invalid bytecode:
  https://bugs.openjdk.java.net/browse/JDK-8148787

Note that this happens when all three things align:
  a) BSM is called directly, as Java method -- javac would never produce
such a String concat shape;
  b) BC_SB_SIZED_EXACT strategy is used, so exactness check can be applied;
  c) -Djava.lang.invoke.stringConcat.debug=true is set, forcing
exactness check to run;

The issue is with exactness debug check using a temporary local variable
when the local variable table is small (like it is in non-arg case). The
code can be reformulated without using temporary variables, with a
creative use of "swap" instruction.

Ironically, the bug is within the debugging code, so we don't care about
its performance at all:
  http://cr.openjdk.java.net/~shade/8148787/webrev.01/

Cheers,
-Aleksey

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

 Alan’s point is that traversing using entries()/stream() always returns 
 the versioned entries (if any) rather than all entries, thus in a sense 
 filters.
 
 My assumption was the traversal should by default be consistent with a 
 calls to getEntry, thus:
 
  jarFile.stream().forEach(e ->  {
 JarEntry je = jarFile.getJarEntry(e.getName());
 assert e.equals(je);
  });
 
 There might need to be another stream method that returns all entries.
 
>>> Right, I'm mostly just wondering if entries()/streams() should override the 
>>> entries in the stream with versioned entries and filter out the 
>>> META-INF/versions/ tree.
>> I don’t think so.  That kind of behavior might be difficult to understand.  
>> Returning all the entries provides some flexibility.  One can write code 
>> like this:
>> 
>> jarfile.stream().map(JarEntry::getName).filter(s ->  
>> !s.startsWith(“META-INF”)).map(JarFile::getJarEntry).etc
>> 
>> to get the versioned results for any version you specify when constructing 
>> the JarFile.
> 
> The current specification treats those class files under meta-inf/releases 
> like
> kind of "metadata" of those base entries. Ideally those files should not even
> be individual "files", but part of their corresponding entries. The consumer 
> of
> the MR-Jar should not need to be aware of these version-ed entries at all to 
> use
> this MR-jar file to load classes/resources. From this point of view, these 
> entries
> probably should be "invisible" from entries()/stream(), when the jar file is 
> opened
> with "version-enabled". And all returned entries should have all their "data"
> (size, csize, timestamps, crc ...) pointed to the corresponding version-ed 
> entries,
> withe the only exception is the "name".
> 
> On the other hand it might be desired to keep JarFile.entries()/stream() asis 
> to
> match their "zip file" perspective, to return "all" raw entries. Then it 
> might also
> be desired to have an alternative "versioned streamVersion()" …

It seems to that we have two reasonable alternatives: (1) return all entries, 
and (2) return all entries except those under the “META-INF/versions/“ 
directory and for any entries returned, return their versioned equivalent if it 
exists.  If we choose alternative 2, we can still get alternative 1 by asking 
for JarFile.super.entries() and JarFile.super.stream().

Or we can do it both ways, leaving entries()/stream() as is and adding two new 
methods, versionedEntries() and versionedStream().

> 
> something like
> 
>public Stream stream(Release r); ?

We should not parametrize the methods with a Release, because what does it mean 
if we construct the JarFile with one Release but specify a different Release 
for the stream argument.  Parameterizing methods with a Release object feels 
like we’re starting to slide down a slippery slope.

I think adding the two new methods is the “right” solution, but I’d like some 
consensus here.

> 
> -sherman
> 
> 
> 
> 
>>> If I've gone to trouble of specifying the a Release then it seems the right 
>>> thing to do. On the other hand, it comes at a cost and there will be 
>>> use-cases like "get the names of all entries" that would be more efficient 
>>> to just build on the current entries()/stream(). I'm loath to suggest this 
>>> might need a new method but it might be one of the options to consider 
>>> here. Minimally there is a javadoc to specify on how these methods behave 
>>> when the JAR is multi-release and opened by specifying a release.
>> How’s this?
>> 
>> diff -r 68867430065b src/java.base/share/classes/java/util/jar/JarFile.java
>> --- a/src/java.base/share/classes/java/util/jar/JarFile.java Fri Jan 29 
>> 12:34:44 2016 -0800
>> +++ b/src/java.base/share/classes/java/util/jar/JarFile.java Mon Feb 01 
>> 09:48:05 2016 -0800
>> @@ -576,9 +576,11 @@
>>  }
>> 
>>  /**
>> - * Returns an enumeration of the jar file entries.
>> + * Returns an enumeration of all the jar file entries.  Constructing 
>> this
>> + * JarFile with the {@link JarFile#JarFile(File, boolean, int, Release)}
>> + * constructor does not modify the behavior of this method.
>>   *
>> - * @return an enumeration of the jar file entries
>> + * @return an enumeration of the all jar file entries
>>   * @throws IllegalStateException
>>   * may be thrown if the jar file has been closed
>>   */
>> @@ -587,11 +589,13 @@
>>  }
>> 
>>  /**
>> - * Returns an ordered {@code Stream} over the jar file entries.
>> + * Returns an ordered {@code Stream} over all the jar file entries.
>>   * Entries appear in the {@code Stream} in the order they appear in
>> - * the central directory of the jar file.
>> + * the central directory of the jar file.  Constructing this
>> + * JarFile with the {@link JarFile#JarFile(File, boolean, int, Release)}
>> + * constructor does not modify the behavior of this meth

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Gerard Ziemski]

> On Feb 1, 2016, at 1:25 PM, Roger Riggs  wrote:
> 
> Hi Gerard,
> 
> On 2/1/2016 1:58 PM, Gerard Ziemski wrote:
>> hi Roger,
>> 
>> I love that we are adding this Signal object. I have several questions, but 
>> please do not take them as criticism, I’m just seeking clarification and 
>> providing feedback:
>> 
>> 
>> #1 Re: "Consumers for signals that are unknown or reserved by the Java 
>> implementation can not be registered.”
>> 
>> - Why don't we want to allow unknown signals? This will make us have to 
>> update our implementation if we want to support new or platform specific 
>> signals in the future. 
>> 
>> 
> The statement was aimed primarily at the Java Signal API; there is quite a 
> bit of detail
> oriented code in the VM to initialize and handle signals. Most of it is 
> agnostic to the signal number
> and would just pass it through.  If a signal is not supported by the OS 
> (think SIGHUP on Windows)
> that should bubble up as being not available.  The 'cannot be registered' 
> might be re-worded to say
> it throws an exception, as the method javadoc does.
> 
> The set of signals is a pretty slow moving target so updating implementations 
> should not be a big issue.

Right, but you don’t actually answer why we don’t allow unknown (to us at the 
moment) signals. Why have a limit in place, unless there is a good reason?


>> #2 Re: "java.util.Signal.raise()”
>> 
>> - That API raises the signal in the current process, but what about sending 
>> a signal to another process for interprocess communication?
>> 
> I left that for a separate issue but would be a straight-forward addition to 
> java.lang.ProcessHandle/Process.

The proposed Signal “feels” incomplete to me without this, though I understand 
that it meets the original goal. I would love to see at the very least a 
followup enhancement filed to address this.


>> 
>> 
>> #3 Re: "Signal.of("SIGINT”)”
>> 
>> - Is this a factory method that returns the same object if called more than 
>> once?
>> 
> Maybe, maybe not, why would it matter.  
> The real state is encapsulate is in the SignalImpl instances which are 
> singletons per signal.
> I was trying to keep the Signal object stateless to allow it to be a 
> value-type and lighter weight
> some day.

If it doesn’t matter, the why not just use constructor “Signal signal = new 
Signal(“SIGINT”)” ?


>> 
>> 
>> #4 Re: "public boolean unregister(Consumer consumer)”
>> 
>> - Why is this API returning a value? Wouldn’t having a Signal API like 
>> “public Consumer getConsumer()” be more flexible?
>> 
> 
> The return value reports whether it unregistered the specific consumer.  If 
> it was not
> the concurrently registered the caller might want to know it was currently 
> registered.
> I expect the return would mostly be ignored. 
> 
> The getConsumer()/unregister consumer pair would be vulnerable to race 
> conditions
> and require some external locking to get predictable behavior.  

Isn’t it also true for register/unregister?

> 
>> 
>> 
>> #5 Re: "public void registerDefault(Consumer consumer)”
>> 
>> - Do we really need this API? Can’t the same be achieved with the plain 
>> vanilla “public void register(Consumer consumer)” I guess I don’t 
>> really see what makes this API special.
>> 
> The java runtime currently registers termination handler to cleanly shutdown 
> if someone types control-c.
> It is useful to be able to remove the application provided signal handler and 
> be able to restore the
> system defaults.
> This API could be hidden as a pure implementation detail.  So unregistering 
> the signal handler
> would always restore the appropriate system ones.

I was hoping that behavior is all that’s needed.


> 
> Thanks for the review and comments, Roger

Thanks for all the work!


> 
>> 
>> 
>> cheers
>> 
>> 
>>> On Feb 1, 2016, at 10:02 AM, Roger Riggs 
>>>  wrote:
>>> 
>>> Please review an API addition to handle signals such as SIGINT, SIGHUP, and 
>>> SIGTERM.
>>> This JEP 260 motivated alternative to sun.misc.Signal supports the use case 
>>> for
>>> interactive applications that need to handle Control-C and other signals.
>>> 
>>> The new java.util.Signal class provides a settable primary signal handler 
>>> and a default
>>> signal handler.  The primary signal handler can be unregistered and 
>>> handling is restored
>>> to the default signal handler.  System initialization registers default 
>>> signal handlers
>>> to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API requires
>>> a permission if a SecurityManager is set.
>>> 
>>> The sun.misc.Signal implementation is modified to be layered on a common
>>> thread and dispatch mechanism. The VM handling of native signals is not 
>>> affected.
>>> The command option to reduce signal use by the runtime with -Xrs is 
>>> unmodified.
>>> 
>>> The changes to hotspot are minimal to rename the hardcoded callback to the 
>>> Java
>>> Signal dispatcher.
>>> 
>>> Please review and comment on the API and implementati

Re: RFR 9: 8143016 : java/time/test/java/time/TestClock_System.java failed intermittently

[Roger Riggs]

Hi,

I discovered a flaw in the fix; it needs to re-read the instant from the 
clock inside the inner loop.

Please review.

Thanks, Roger

On 2/1/2016 11:41 AM, Roger Riggs wrote:
Please review a test improvement to address an intermittent test 
failure in

the java.time System Clock test for microsecond support.

webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-micros-8143016/

Issue:
  https://bugs.openjdk.java.net/browse/JDK-8143016

Thanks, Roger

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

I’m sorry, I didn’t look at the code close enough before I started talking ;-)  
Right now entries()/stream() returns all entries and if the JarFile is 
constructed with a Release object != Release.BASE, the base entries that are 
returned are the versioned entries.  I think this behavior is a bit confusing 
and we should just return all entries without regard to versioning.  Then 
create the two new methods for specific versioned entries.

> On Feb 1, 2016, at 12:18 PM, Steve Drach  wrote:
> 
> Alan’s point is that traversing using entries()/stream() always returns 
> the versioned entries (if any) rather than all entries, thus in a sense 
> filters.
> 
> My assumption was the traversal should by default be consistent with a 
> calls to getEntry, thus:
> 
> jarFile.stream().forEach(e ->  {
>JarEntry je = jarFile.getJarEntry(e.getName());
>assert e.equals(je);
> });
> 
> There might need to be another stream method that returns all entries.
> 
 Right, I'm mostly just wondering if entries()/streams() should override 
 the entries in the stream with versioned entries and filter out the 
 META-INF/versions/ tree.
>>> I don’t think so.  That kind of behavior might be difficult to understand.  
>>> Returning all the entries provides some flexibility.  One can write code 
>>> like this:
>>> 
>>> jarfile.stream().map(JarEntry::getName).filter(s ->  
>>> !s.startsWith(“META-INF”)).map(JarFile::getJarEntry).etc
>>> 
>>> to get the versioned results for any version you specify when constructing 
>>> the JarFile.
>> 
>> The current specification treats those class files under meta-inf/releases 
>> like
>> kind of "metadata" of those base entries. Ideally those files should not even
>> be individual "files", but part of their corresponding entries. The consumer 
>> of
>> the MR-Jar should not need to be aware of these version-ed entries at all to 
>> use
>> this MR-jar file to load classes/resources. From this point of view, these 
>> entries
>> probably should be "invisible" from entries()/stream(), when the jar file is 
>> opened
>> with "version-enabled". And all returned entries should have all their "data"
>> (size, csize, timestamps, crc ...) pointed to the corresponding version-ed 
>> entries,
>> withe the only exception is the "name".
>> 
>> On the other hand it might be desired to keep JarFile.entries()/stream() 
>> asis to
>> match their "zip file" perspective, to return "all" raw entries. Then it 
>> might also
>> be desired to have an alternative "versioned streamVersion()" …
> 
> It seems to that we have two reasonable alternatives: (1) return all entries, 
> and (2) return all entries except those under the “META-INF/versions/“ 
> directory and for any entries returned, return their versioned equivalent if 
> it exists.  If we choose alternative 2, we can still get alternative 1 by 
> asking for JarFile.super.entries() and JarFile.super.stream().
> 
> Or we can do it both ways, leaving entries()/stream() as is and adding two 
> new methods, versionedEntries() and versionedStream().
> 
>> 
>> something like
>> 
>>   public Stream stream(Release r); ?
> 
> We should not parametrize the methods with a Release, because what does it 
> mean if we construct the JarFile with one Release but specify a different 
> Release for the stream argument.  Parameterizing methods with a Release 
> object feels like we’re starting to slide down a slippery slope.
> 
> I think adding the two new methods is the “right” solution, but I’d like some 
> consensus here.
> 
>> 
>> -sherman
>> 
>> 
>> 
>> 
 If I've gone to trouble of specifying the a Release then it seems the 
 right thing to do. On the other hand, it comes at a cost and there will be 
 use-cases like "get the names of all entries" that would be more efficient 
 to just build on the current entries()/stream(). I'm loath to suggest this 
 might need a new method but it might be one of the options to consider 
 here. Minimally there is a javadoc to specify on how these methods behave 
 when the JAR is multi-release and opened by specifying a release.
>>> How’s this?
>>> 
>>> diff -r 68867430065b src/java.base/share/classes/java/util/jar/JarFile.java
>>> --- a/src/java.base/share/classes/java/util/jar/JarFile.javaFri Jan 
>>> 29 12:34:44 2016 -0800
>>> +++ b/src/java.base/share/classes/java/util/jar/JarFile.javaMon Feb 
>>> 01 09:48:05 2016 -0800
>>> @@ -576,9 +576,11 @@
>>> }
>>> 
>>> /**
>>> - * Returns an enumeration of the jar file entries.
>>> + * Returns an enumeration of all the jar file entries.  Constructing 
>>> this
>>> + * JarFile with the {@link JarFile#JarFile(File, boolean, int, 
>>> Release)}
>>> + * constructor does not modify the behavior of this method.
>>>  *
>>> - * @return an enumeration of the jar file entries
>>> + * @return an enumeration of the all jar file entries
>>>  * @throws Illega

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Roger Riggs]

Hi,

On 2/1/2016 3:17 PM, Gerard Ziemski wrote:

On Feb 1, 2016, at 1:25 PM, Roger Riggs  wrote:

Hi Gerard,

On 2/1/2016 1:58 PM, Gerard Ziemski wrote:

hi Roger,

I love that we are adding this Signal object. I have several questions, but 
please do not take them as criticism, I’m just seeking clarification and 
providing feedback:


#1 Re: "Consumers for signals that are unknown or reserved by the Java 
implementation can not be registered.”

- Why don't we want to allow unknown signals? This will make us have to update 
our implementation if we want to support new or platform specific signals in 
the future.



The statement was aimed primarily at the Java Signal API; there is quite a bit 
of detail
oriented code in the VM to initialize and handle signals. Most of it is 
agnostic to the signal number
and would just pass it through.  If a signal is not supported by the OS (think 
SIGHUP on Windows)
that should bubble up as being not available.  The 'cannot be registered' might 
be re-worded to say
it throws an exception, as the method javadoc does.

The set of signals is a pretty slow moving target so updating implementations 
should not be a big issue.

Right, but you don’t actually answer why we don’t allow unknown (to us at the 
moment) signals. Why have a limit in place, unless there is a good reason?
There is no artificial limit; just the list of signal names.  The 
implementation should not be expected to

handle unknown inputs.





#2 Re: "java.util.Signal.raise()”

- That API raises the signal in the current process, but what about sending a 
signal to another process for interprocess communication?


I left that for a separate issue but would be a straight-forward addition to 
java.lang.ProcessHandle/Process.

The proposed Signal “feels” incomplete to me without this, though I understand 
that it meets the original goal. I would love to see at the very least a 
followup enhancement filed to address this.

how about:
4914493 (process) Cannot send arbitary signals to UNIX process 





#3 Re: "Signal.of("SIGINT”)”

- Is this a factory method that returns the same object if called more than 
once?


Maybe, maybe not, why would it matter.
The real state is encapsulate is in the SignalImpl instances which are 
singletons per signal.
I was trying to keep the Signal object stateless to allow it to be a value-type 
and lighter weight
some day.

If it doesn’t matter, the why not just use constructor “Signal signal = new 
Signal(“SIGINT”)” ?
Factory methods are preferred to constructors;  it allows greater 
flexibility in the implementation, current and future.





#4 Re: "public boolean unregister(Consumer consumer)”

- Why is this API returning a value? Wouldn’t having a Signal API like “public 
Consumer getConsumer()” be more flexible?


The return value reports whether it unregistered the specific consumer.  If it 
was not
the concurrently registered the caller might want to know it was currently 
registered.
I expect the return would mostly be ignored.

The getConsumer()/unregister consumer pair would be vulnerable to race 
conditions
and require some external locking to get predictable behavior.

Isn’t it also true for register/unregister?

Register is a strong takeover of responsibility for handling the signal.
Unregister is safer if the caller has to say which handler to remove and 
not remove one the caller

did not register.
In practice, it requires a higher level coordination to avoid 
interference between competing
interests in handling signals which makes it more complicated than 
necessary for the use case.




#5 Re: "public void registerDefault(Consumer consumer)”

- Do we really need this API? Can’t the same be achieved with the plain vanilla 
“public void register(Consumer consumer)” I guess I don’t really see 
what makes this API special.


The java runtime currently registers termination handler to cleanly shutdown if 
someone types control-c.
It is useful to be able to remove the application provided signal handler and 
be able to restore the
system defaults.
This API could be hidden as a pure implementation detail.  So unregistering the 
signal handler
would always restore the appropriate system ones.

I was hoping that behavior is all that’s needed.

Worth considering, will look for other comments on the subject.

Thanks, Roger





Thanks for the review and comments, Roger

Thanks for all the work!




cheers



On Feb 1, 2016, at 10:02 AM, Roger Riggs 
  wrote:

Please review an API addition to handle signals such as SIGINT, SIGHUP, and 
SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use case for
interactive applications that need to handle Control-C and other signals.

The new java.util.Signal class provides a settable primary signal handler and a 
default
signal handler.  The primary signal handler can be unregistered and handling is 
restored
to the default signal handler.  System initialization 

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Paul Benedict]

I believe the answer should be based on your viewpoint of what "is" a JAR.
Do you consider the JAR to be a dumb ZIP container or an artifact of the
Java runtime? If it's the former, then return everything because the
version folder is meaningless in this perspective. Otherwise, treat the JAR
according to how Java runtime would load it, which is the versioned
entries. Whatever your answer is, that should be the behavior for
entries()/stream().

Those who don't like the behavior should be given a second set of methods
to customize the return.

Cheers,
Paul

On Mon, Feb 1, 2016 at 2:29 PM, Steve Drach  wrote:

> I’m sorry, I didn’t look at the code close enough before I started talking
> ;-)  Right now entries()/stream() returns all entries and if the JarFile is
> constructed with a Release object != Release.BASE, the base entries that
> are returned are the versioned entries.  I think this behavior is a bit
> confusing and we should just return all entries without regard to
> versioning.  Then create the two new methods for specific versioned entries.
>
> > On Feb 1, 2016, at 12:18 PM, Steve Drach  wrote:
> >
> > Alan’s point is that traversing using entries()/stream() always
> returns the versioned entries (if any) rather than all entries, thus in a
> sense filters.
> >
> > My assumption was the traversal should by default be consistent with
> a calls to getEntry, thus:
> >
> > jarFile.stream().forEach(e ->  {
> >JarEntry je = jarFile.getJarEntry(e.getName());
> >assert e.equals(je);
> > });
> >
> > There might need to be another stream method that returns all
> entries.
> >
>  Right, I'm mostly just wondering if entries()/streams() should
> override the entries in the stream with versioned entries and filter out
> the META-INF/versions/ tree.
> >>> I don’t think so.  That kind of behavior might be difficult to
> understand.  Returning all the entries provides some flexibility.  One can
> write code like this:
> >>>
> >>> jarfile.stream().map(JarEntry::getName).filter(s ->
> !s.startsWith(“META-INF”)).map(JarFile::getJarEntry).etc
> >>>
> >>> to get the versioned results for any version you specify when
> constructing the JarFile.
> >>
> >> The current specification treats those class files under
> meta-inf/releases like
> >> kind of "metadata" of those base entries. Ideally those files should
> not even
> >> be individual "files", but part of their corresponding entries. The
> consumer of
> >> the MR-Jar should not need to be aware of these version-ed entries at
> all to use
> >> this MR-jar file to load classes/resources. From this point of view,
> these entries
> >> probably should be "invisible" from entries()/stream(), when the jar
> file is opened
> >> with "version-enabled". And all returned entries should have all their
> "data"
> >> (size, csize, timestamps, crc ...) pointed to the corresponding
> version-ed entries,
> >> withe the only exception is the "name".
> >>
> >> On the other hand it might be desired to keep
> JarFile.entries()/stream() asis to
> >> match their "zip file" perspective, to return "all" raw entries. Then
> it might also
> >> be desired to have an alternative "versioned streamVersion()" …
> >
> > It seems to that we have two reasonable alternatives: (1) return all
> entries, and (2) return all entries except those under the
> “META-INF/versions/“ directory and for any entries returned, return their
> versioned equivalent if it exists.  If we choose alternative 2, we can
> still get alternative 1 by asking for JarFile.super.entries() and
> JarFile.super.stream().
> >
> > Or we can do it both ways, leaving entries()/stream() as is and adding
> two new methods, versionedEntries() and versionedStream().
> >
> >>
> >> something like
> >>
> >>   public Stream stream(Release r); ?
> >
> > We should not parametrize the methods with a Release, because what does
> it mean if we construct the JarFile with one Release but specify a
> different Release for the stream argument.  Parameterizing methods with a
> Release object feels like we’re starting to slide down a slippery slope.
> >
> > I think adding the two new methods is the “right” solution, but I’d like
> some consensus here.
> >
> >>
> >> -sherman
> >>
> >>
> >>
> >>
>  If I've gone to trouble of specifying the a Release then it seems the
> right thing to do. On the other hand, it comes at a cost and there will be
> use-cases like "get the names of all entries" that would be more efficient
> to just build on the current entries()/stream(). I'm loath to suggest this
> might need a new method but it might be one of the options to consider
> here. Minimally there is a javadoc to specify on how these methods behave
> when the JAR is multi-release and opened by specifying a release.
> >>> How’s this?
> >>>
> >>> diff -r 68867430065b
> src/java.base/share/classes/java/util/jar/JarFile.java
> >>> --- a/src/java.base/share/classes/java/util/jar/JarFile.java
> Fri Jan 29 12:34:44 20

Re: RFR 9: 8143016 : java/time/test/java/time/TestClock_System.java failed intermittently

[Lance Andersen]

See the change, looks OK 
On Feb 1, 2016, at 3:26 PM, Roger Riggs  wrote:

> Hi,
> 
> I discovered a flaw in the fix; it needs to re-read the instant from the 
> clock inside the inner loop.
> Please review.
> 
> Thanks, Roger
> 
> On 2/1/2016 11:41 AM, Roger Riggs wrote:
>> Please review a test improvement to address an intermittent test failure in
>> the java.time System Clock test for microsecond support.
>> 
>> webrev:
>>  http://cr.openjdk.java.net/~rriggs/webrev-micros-8143016/
>> 
>> Issue:
>>  https://bugs.openjdk.java.net/browse/JDK-8143016
>> 
>> Thanks, Roger
>> 
> 



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[David Holmes]

Hi Roger,

Sorry I couldn't look into the code in extreme detail right now but 
would like to clarify the big picture ...


Can you please clarify the exact flow of control from when a signal is 
sent to the process and when the Java handler for it gets to run - what 
is handled by which thread where, and in what context (signal handling 
context or regular execution context)? I'm assuming no Java code is ever 
being executed in a signal handling context, but I'm not clear on the 
threading model being used here.


Is the VM still responsible for the initial receipt of all signals?

Howe do you manage a user-defined signal handler, for ctrl-C say, with 
the default behaviour for that signal (orderly shutdown) ?


Thanks,
David

On 2/02/2016 2:02 AM, Roger Riggs wrote:

Please review an API addition to handle signals such as SIGINT, SIGHUP,
and SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use
case for
interactive applications that need to handle Control-C and other signals.

The new java.util.Signal class provides a settable primary signal
handler and a default
signal handler.  The primary signal handler can be unregistered and
handling is restored
to the default signal handler.  System initialization registers default
signal handlers
to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API
requires
a permission if a SecurityManager is set.

The sun.misc.Signal implementation is modified to be layered on a common
thread and dispatch mechanism. The VM handling of native signals is not
affected.
The command option to reduce signal use by the runtime with -Xrs is
unmodified.

The changes to hotspot are minimal to rename the hardcoded callback to
the Java
Signal dispatcher.

Please review and comment on the API and implementation.

javadoc:
   http://cr.openjdk.java.net/~rriggs/signal-doc/

Webrev:
jdk:  http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8087286

JEP 260:
   https://bugs.openjdk.java.net/browse/JDK-8132928

Thanks, Roger

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[Roger Riggs]

HI David,


On 2/1/2016 4:26 PM, David Holmes wrote:

Hi Roger,

Sorry I couldn't look into the code in extreme detail right now but 
would like to clarify the big picture ...


Can you please clarify the exact flow of control from when a signal is 
sent to the process and when the Java handler for it gets to run - 
what is handled by which thread where, and in what context (signal 
handling context or regular execution context)? I'm assuming no Java 
code is ever being executed in a signal handling context, but I'm not 
clear on the threading model being used here.
The VM signal handling control flows are unchanged, I understand it 
roughly as the vm has a native signal handler
that atomically sets an entry in an array indexed by signal number.   
See os.cpp.


A separate dedicated thread waits to be woken up (signal_wait) and polls 
the array (os_linux.cpp:check_pending_signals).
It then does an upcall on the dedicated thread to call the 
java.util.Signal.dispatch method.
The java code takes it from there to find a registered java signal 
handler and then start

a new thread to call the signal handler.

The invocation of the java handler method is asynchronous to the native 
signal handler
and due to thread scheduling for the dedicated thread and the spawning 
of a new thread,
quite some time may pass before the registered java signal handler is 
invoked.




Is the VM still responsible for the initial receipt of all signals?

yes, no change.


How do you manage a user-defined signal handler, for ctrl-C say, with 
the default behaviour for that signal (orderly shutdown) ?
System initialization registers a default signal handler for SIGINT, 
SIGTERM, and SIGHUP (not windows).
If there is no registered signal handler, the default handler is invoked 
in the same manner.


Roger



Thanks,
David

On 2/02/2016 2:02 AM, Roger Riggs wrote:

Please review an API addition to handle signals such as SIGINT, SIGHUP,
and SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use
case for
interactive applications that need to handle Control-C and other 
signals.


The new java.util.Signal class provides a settable primary signal
handler and a default
signal handler.  The primary signal handler can be unregistered and
handling is restored
to the default signal handler.  System initialization registers default
signal handlers
to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API
requires
a permission if a SecurityManager is set.

The sun.misc.Signal implementation is modified to be layered on a common
thread and dispatch mechanism. The VM handling of native signals is not
affected.
The command option to reduce signal use by the runtime with -Xrs is
unmodified.

The changes to hotspot are minimal to rename the hardcoded callback to
the Java
Signal dispatcher.

Please review and comment on the API and implementation.

javadoc:
   http://cr.openjdk.java.net/~rriggs/signal-doc/

Webrev:
jdk:  http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8087286

JEP 260:
   https://bugs.openjdk.java.net/browse/JDK-8132928

Thanks, Roger

Re: RFR: 8072777: java/lang/ref/ReferenceEnqueuePending.java: some references aren't queued

[Kim Barrett]

> On Jan 31, 2016, at 4:19 AM, Peter Levart  wrote:
> 
> Hi Kim,

Hi Peter - Thanks for looking at this.

> This change will make it practically impossible to miss the enqueued 
> references.

Good.  That’s the goal.

> But this could be an opportunity to also clean-up the code a bit. The 
> checkResult method has an 'obj' parameter whose purpose is not immediately 
> obvious. […]
> 
> I don't know why such complications are necessary. Is the test supposed to 
> also verify the requirement that a Reference whose referent is kept strongly 
> reachable will not be enqueued?

I was wondering about the odd stuff around obj as well. I don't think
it is an attempt to "also verify the requirement that a Reference
whose referent is kept strongly reachable will not be enqueued".
Rather, I think it is a kludgy way to avoid having the final weaky be
sometimes enqueued and sometimes not, depending on compiler
optimizations.

Using Reference.reachabilityFence to keep obj and weaky live across
the checkResult as you suggested is one way to accomplish that, though
keeping only the final obj alive (as in the existing code) suffices to
keep the final weaky from being notified.  I think reachabilityFence
and a better comment would be clearer though, so changing accordingly.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8072777/webrev.01/
incr: http://cr.openjdk.java.net/~kbarrett/8072777/webrev.01.inc/

Re: RFR 9: 8087286 Need a way to handle control-C and possibly some other signals

[David Holmes]

Hi Roger,

On 2/02/2016 7:41 AM, Roger Riggs wrote:

HI David,


On 2/1/2016 4:26 PM, David Holmes wrote:

Hi Roger,

Sorry I couldn't look into the code in extreme detail right now but
would like to clarify the big picture ...

Can you please clarify the exact flow of control from when a signal is
sent to the process and when the Java handler for it gets to run -
what is handled by which thread where, and in what context (signal
handling context or regular execution context)? I'm assuming no Java
code is ever being executed in a signal handling context, but I'm not
clear on the threading model being used here.

The VM signal handling control flows are unchanged, I understand it
roughly as the vm has a native signal handler
that atomically sets an entry in an array indexed by signal number.
See os.cpp.

A separate dedicated thread waits to be woken up (signal_wait) and polls
the array (os_linux.cpp:check_pending_signals).
It then does an upcall on the dedicated thread to call the
java.util.Signal.dispatch method.
The java code takes it from there to find a registered java signal
handler and then start
a new thread to call the signal handler.

The invocation of the java handler method is asynchronous to the native
signal handler
and due to thread scheduling for the dedicated thread and the spawning
of a new thread,
quite some time may pass before the registered java signal handler is
invoked.


Ok. So this is primarily moving sun.misc.Signal to java.util.Signal with 
a supported API?




Is the VM still responsible for the initial receipt of all signals?

yes, no change.


I need to refresh myself as to how the set of acceptable signals is 
defined ie the sig masks.




How do you manage a user-defined signal handler, for ctrl-C say, with
the default behaviour for that signal (orderly shutdown) ?

System initialization registers a default signal handler for SIGINT,
SIGTERM, and SIGHUP (not windows).
If there is no registered signal handler, the default handler is invoked
in the same manner.


Does that mean that a user registered ctrl-C handler will prevent the VM 
from being terminated by ctrl-C? I assume so.


I'm dubious about the need for the average user to be able to provide 
Java level "signal" handlers - it is not as clear cut as it might seem. 
These are low-level system concepts that don't really fit in to the Java 
WORA principle in my opinion. We have provided lifecycle management of 
the VM by other means, including the use of ShutdownHooks.


I guess I should have been aware of JEP 260's intentions re 
sun.misc.Signal. I don't think a public supported API is appropriate for 
this and I think it will open a can of worms when it comes to VM 
lifecycle management.


Sorry.

David
-



Roger



Thanks,
David

On 2/02/2016 2:02 AM, Roger Riggs wrote:

Please review an API addition to handle signals such as SIGINT, SIGHUP,
and SIGTERM.
This JEP 260 motivated alternative to sun.misc.Signal supports the use
case for
interactive applications that need to handle Control-C and other
signals.

The new java.util.Signal class provides a settable primary signal
handler and a default
signal handler.  The primary signal handler can be unregistered and
handling is restored
to the default signal handler.  System initialization registers default
signal handlers
to terminate on SIGINT, SIGHUP, and SIGTERM.  Use of the Signal API
requires
a permission if a SecurityManager is set.

The sun.misc.Signal implementation is modified to be layered on a common
thread and dispatch mechanism. The VM handling of native signals is not
affected.
The command option to reduce signal use by the runtime with -Xrs is
unmodified.

The changes to hotspot are minimal to rename the hardcoded callback to
the Java
Signal dispatcher.

Please review and comment on the API and implementation.

javadoc:
http://cr.openjdk.java.net/~rriggs/signal-doc/

Webrev:
jdk: http://cr.openjdk.java.net/~rriggs/webrev-signal-8087286/
hotspot: http://cr.openjdk.java.net/~rriggs/webrev-hs-signal-8087286/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8087286

JEP 260:
https://bugs.openjdk.java.net/browse/JDK-8132928

Thanks, Roger

RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

[Iris Clark]

Hi, Hong.

Thanks again for looking closely at the regular expression and providing
comments.

> - The outer most quantifier in (((\.0)*\.[1-9][0-9]*)*)* is redundant
>   and a source of catastrophic backtracking. You only need 
>   ((\.0)*\.[1-9][0-9]*)*.
> - The outside - in PRE part (\-([a-zA-Z0-9]+))? doesn't need escaping.
>   Simplify it to (-([a-zA-Z0-9]+))?

Both done.

> - Do you want to allow only a plus without the number in the BUILD
?   part? Why do you capture the +? ((\+)(0|[1-9][0-9]*)?)?

I need to capture the plus to distinguish between cases where an 
empty build is allowed (e.g. "9+-foo") and when it is not ("9+").  
See code in Version.java, line 226-230 and in Basic.java, line 98, 
107-109.  (Note that we use the empty "+" to distinguish "9-foo" from
"9+-foo".)

> - Dot loses meaning in the character class, and hyphen loses 
> meaning at the beginning or at the end of the character class. You 
> can simplify the OPT part to (-([-a-zA-Z0-9.]+))?

Done.

> - You might want to consider using named-capturing groups instead 
> of numbered capturing groups.  Also, consider using non-capturing 
> groups for groups you don't need to extract.

Done.

This is the (hopefully) final version of the webrev and javadoc for
the initial implementation of this API:

  http://cr.openjdk.java.net/~iris/verona/8072379/webrev.3/index.html
  http://cr.openjdk.java.net/~iris/verona/8072379/doc.3/jdk/Version.html

I've filed the following bug to update the spec:

8148822: (spec) Regex in jdk.Version and JEP 223 should match
https://bugs.openjdk.java.net/browse/JDK-8148822

These are the corresponding changes to the JEP which I'll apply in the
next day or so:

$ diff jep-open-mr11.md jep-open-mr12.md
84c84
< `^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$`. The sequence may be of arbitrary
---
> `^[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*$`. The sequence may be of arbitrary
166c166
<   -  $OPT, matching `([-a-zA-Z0-9\.]+)` --- Additional
---
>   -  $OPT, matching `([-a-zA-Z0-9.]+)` --- Additional

Thanks again for the recommendations.

Regards,
iris

-Original Message-
From: Thanh Hong Dai [mailto:hdth...@tma.com.vn] 
Sent: Wednesday, January 13, 2016 7:53 PM
To: Iris Clark; Alan Bateman; core-libs-dev@openjdk.java.net; 
verona-...@openjdk.java.net
Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi,

Some comment on the regex (and also the JEP):

([1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*)(\-([a-zA-Z0-9]+))?((\+)(0|[1-9][0-9]*)?)?(-([\-a-zA-Z0-9\.]+))?

- The outer most quantifier in (((\.0)*\.[1-9][0-9]*)*)* is redundant and a 
source of catastrophic backtracking. You only need ((\.0)*\.[1-9][0-9]*)*.
- The outside - in PRE part (\-([a-zA-Z0-9]+))? doesn't need escaping. Simplify 
it to (-([a-zA-Z0-9]+))?
- Do you want to allow only a plus without the number in the BUILD part? Why do 
you capture the +? ((\+)(0|[1-9][0-9]*)?)?
- Dot loses meaning in the character class, and hyphen loses meaning at the 
beginning or at the end of the character class. You can simplify the OPT part 
to (-([-a-zA-Z0-9.]+))?
- You might want to consider using named-capturing groups instead of numbered 
capturing groups. Also, consider using non-capturing groups for groups you 
don't need to extract.

Best regards,
Hong Dai Thanh.

-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf 
Of Iris Clark
Sent: Thursday, 14 January, 2016 4:55 AM
To: Alan Bateman ; core-libs-dev@openjdk.java.net; 
verona-...@openjdk.java.net
Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi, Alan.

Thanks for looking at this (hopefully) one last time.

> It can't be java.base (see design principles in JEP 200). 
> If it's going into java.base temporarily then the top-level 
> modules.xml will need to be updated to export the "jdk" package.

This diff has been applied to modules.xml:

diff -r 6fefc5bce180 modules.xml
--- a/modules.xml   Wed Jan 13 13:56:19 2016 +
+++ b/modules.xml   Wed Jan 13 13:46:56 2016 -0800
@@ -205,6 +205,9 @@
   javax.security.cert
 
 
+  jdk
+
+
   jdk.net
 
 

It essentially reverts your 8049422 change [0] to that file. I will not re-add 
jdk to the javadoc bundle for javac trees API since that is not an appropriate 
location. I filed the following bug to track publication of jdk.Version:

8144069: Determine correct publication for jdk.Version API
https://bugs.openjdk.java.net/browse/JDK-8144069

When this came up earlier, I filed this bug to track finding a more appropriate 
module for jdk.Version:

8144062: Determine appropriate module for jdk.Version
https://bugs.openjdk.java.net/browse/JDK-8144062

Thanks,
iris

[0] http://hg.openjdk.java.net/jdk9/dev/rev/1bee5efa73e3

-Original Message-
From: Alan Bateman
Sent: Tuesday, January 12, 2016 7:41 AM
To: Iris Clark; core-libs-dev@openjdk.java.net; verona-...@openjdk.java.net
Subject: Re: RFR: 8072379: Implement jdk.Ve

Re: RFR: 8079136: Accessing a nested sublist leads to StackOverflowError

[Tagir F. Valeev]

Hello!

IG> It's misfortune that the spec of subList() doesn't match the spec of
IG> similar methods, like CharSequence.subSequence() or String.substring().
IG> It even contradicts the spec of List.subList(), which declares only IOOB
IG> to be thrown!

Yes, it's a pity. Isn't it the reason to change (fix) the spec making
List.subList and AbstractList.subList conformant?

>> AbstractList::rangeCheck could be replaced with Obejcts::checkIndex.
>> The same for ArrayList class.

IG> Right.
IG> Here's the updated webrev:
IG> http://cr.openjdk.java.net/~igerasim/8079136/06/webrev/

I have a doubt about replacing rangeCheckForAdd. What if size() ==
Integer.MAX_VALUE? This is not an issue for ArrayList as it's limited
by MAX_ARRAY_SIZE which is Integer.MAX_VALUE - 8. However for user
defined AbstractList having size = Integer.MAX_VALUE looks possible.
In this case range check would fail for any index.

With best regards,
Tagir Valeev.

IG> Sincerely yours,
IG> Ivan

>> Otherwise looks good to me.
>>
>> With best regards,
>> Tagir Valeev.
>>
>> IG> Hello everyone!
>>
>> IG> I'd like to respin the discussion.
>> IG> The previous attempt can be found here:
>> IG> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033159.html
>>
>> IG> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8079136
>> IG> WEBREV: http://cr.openjdk.java.net/~igerasim/8079136/05/webrev/
>>
>> IG> Here's the summary of the proposed changes:
>> IG> 1)
>> IG> Sublist of an AbstractList (AbstractList.SubList class) now maintains a
>> IG> link to the root AbstractList, and not only to the immediate parent list.
>> IG> This allows to perform modifications on the chain of sub-lists in a loop
>> IG> instead of using recursion (which, in particular, helps avoid the
>> IG> stack-overflow problem).
>> IG> The sublist is still backed by its parent list, in sense that all the
>> IG> modifications are correctly reflected in the backing list (as well as in
>> IG> all the grand parents the sublist, if any), so the fix does not violate
>> IG> the existing specification.
>>
>> IG> 2)
>> IG> It is proposed to update the spec of AbstractList.subList() in the
>> IG> @implSpec section by removing the words about private fields.
>> IG> With the fix, some of those private fields are removed.
>>
>> IG> 3)
>> IG> A similar change is proposed for the ArrayList.SubList class.
>>
>> IG> 4)
>> IG> Two regression tests are added:
>> IG> NestedSubList.java - demonstrates a stack-overflow problem when dealing
>> IG> with a long chain of sublists,
>> IG> SubList.java - tests basic functionality of sub-lists, which helps us
>> IG> make sure nothing is broken with the proposed change.
>>
>> IG> If people agree that the fix is good, I'll file a CCC request for
>> IG> changing the spec of AbstractList.subList().
>>
>> IG> Sincerely yours,
>> IG> Ivan
>>
>>

Re: RFR: 8079136: Accessing a nested sublist leads to StackOverflowError

[Ivan Gerasimov]

Thanks Tagir for the comments!


On 02.02.2016 9:19, Tagir F. Valeev wrote:

Hello!

IG> It's misfortune that the spec of subList() doesn't match the spec of
IG> similar methods, like CharSequence.subSequence() or String.substring().
IG> It even contradicts the spec of List.subList(), which declares only IOOB
IG> to be thrown!

Yes, it's a pity. Isn't it the reason to change (fix) the spec making
List.subList and AbstractList.subList conformant?

A request to fix that is already there:
https://bugs.openjdk.java.net/browse/JDK-4506427 (it's 15 years old!)

I'd rather keep it separate from this fix (JDK-8079136).



AbstractList::rangeCheck could be replaced with Obejcts::checkIndex.
The same for ArrayList class.

IG> Right.
IG> Here's the updated webrev:
IG> http://cr.openjdk.java.net/~igerasim/8079136/06/webrev/

I have a doubt about replacing rangeCheckForAdd. What if size() ==
Integer.MAX_VALUE? This is not an issue for ArrayList as it's limited
by MAX_ARRAY_SIZE which is Integer.MAX_VALUE - 8. However for user
defined AbstractList having size = Integer.MAX_VALUE looks possible.
In this case range check would fail for any index.


Ah, good point.
I'll revert that part that part of the change back.

Sincerely yours,
Ivan



With best regards,
Tagir Valeev.

IG> Sincerely yours,
IG> Ivan


Otherwise looks good to me.

With best regards,
Tagir Valeev.

IG> Hello everyone!

IG> I'd like to respin the discussion.
IG> The previous attempt can be found here:
IG> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033159.html

IG> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8079136
IG> WEBREV: http://cr.openjdk.java.net/~igerasim/8079136/05/webrev/

IG> Here's the summary of the proposed changes:
IG> 1)
IG> Sublist of an AbstractList (AbstractList.SubList class) now maintains a
IG> link to the root AbstractList, and not only to the immediate parent list.
IG> This allows to perform modifications on the chain of sub-lists in a loop
IG> instead of using recursion (which, in particular, helps avoid the
IG> stack-overflow problem).
IG> The sublist is still backed by its parent list, in sense that all the
IG> modifications are correctly reflected in the backing list (as well as in
IG> all the grand parents the sublist, if any), so the fix does not violate
IG> the existing specification.

IG> 2)
IG> It is proposed to update the spec of AbstractList.subList() in the
IG> @implSpec section by removing the words about private fields.
IG> With the fix, some of those private fields are removed.

IG> 3)
IG> A similar change is proposed for the ArrayList.SubList class.

IG> 4)
IG> Two regression tests are added:
IG> NestedSubList.java - demonstrates a stack-overflow problem when dealing
IG> with a long chain of sublists,
IG> SubList.java - tests basic functionality of sub-lists, which helps us
IG> make sure nothing is broken with the proposed change.

IG> If people agree that the fix is good, I'll file a CCC request for
IG> changing the spec of AbstractList.subList().

IG> Sincerely yours,
IG> Ivan

Re: RFR: 8079136: Accessing a nested sublist leads to StackOverflowError

[Martin Buchholz]

On Mon, Feb 1, 2016 at 10:19 PM, Tagir F. Valeev  wrote:

> I have a doubt about replacing rangeCheckForAdd. What if size() ==
> Integer.MAX_VALUE? This is not an issue for ArrayList as it's limited
> by MAX_ARRAY_SIZE which is Integer.MAX_VALUE - 8.

Actually, the limit is Integer.MAX_VALUE.  But it only grows beyond
MAX_ARRAY_SIZE if there's no choice.

Re: RFR (S) 8148787: StringConcatFactory exactness check produces bad bytecode when a non-arg concat is requested

[Aleksey Shipilev]

Anyone? This looks like a trivial fix.

-Aleksey

On 02/01/2016 10:47 PM, Aleksey Shipilev wrote:
> Hi,
> 
> Please review the fix for a corner case in StringConcatFactory exactness
> check, which produces invalid bytecode:
>   https://bugs.openjdk.java.net/browse/JDK-8148787
> 
> Note that this happens when all three things align:
>   a) BSM is called directly, as Java method -- javac would never produce
> such a String concat shape;
>   b) BC_SB_SIZED_EXACT strategy is used, so exactness check can be applied;
>   c) -Djava.lang.invoke.stringConcat.debug=true is set, forcing
> exactness check to run;
> 
> The issue is with exactness debug check using a temporary local variable
> when the local variable table is small (like it is in non-arg case). The
> code can be reformulated without using temporary variables, with a
> creative use of "swap" instruction.
> 
> Ironically, the bug is within the debugging code, so we don't care about
> its performance at all:
>   http://cr.openjdk.java.net/~shade/8148787/webrev.01/
> 
> Cheers,
> -Aleksey
> 

RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

[Thanh Hong Dai]

Dear Iris,

On closer look, there seems to be some conflicting definition of version string.

In JEP: http://openjdk.java.net/jeps/223
$VNUM(-$PRE)?(\+$BUILD)?(-$OPT)?

In Javadoc: 
http://cr.openjdk.java.net/~iris/verona/8072379/doc.3/jdk/Version.html
$VNUM(-$PRE)?(\+($BUILD)?(-$OPT)?)?

In implementation:
The regex follows JEP's definition.

The JEP & implementation allows -$OPT to be specified without +, but the 
Javadoc one does not allow that. For example, "9-pre-opt" is allowed by JEP, 
but disallowed by Javadoc.

> I need to capture the plus to distinguish between cases where an empty build 
> is allowed (e.g. "9+-foo") and when it is not ("9+").  
> See code in Version.java, line 226-230 and in Basic.java, line 98, 107-109.  
> (Note that we use the empty "+"  to distinguish "9-foo" from "9+-foo".)

Understood, but I didn't see any part of the JEP or the Javadoc explaining that 
+ is needed to make the parser recognize the text followed as options instead 
of pre-release identifier. It would be great if that is added.

Best regards,
Hong Dai Thanh.

-Original Message-
From: Iris Clark [mailto:iris.cl...@oracle.com] 
Sent: Tuesday, 2 February, 2016 12:51 PM
To: Thanh Hong Dai ; Alan Bateman 
; core-libs-dev@openjdk.java.net; 
verona-...@openjdk.java.net
Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi, Hong.

Thanks again for looking closely at the regular expression and providing 
comments.

> - The outer most quantifier in (((\.0)*\.[1-9][0-9]*)*)* is redundant
>   and a source of catastrophic backtracking. You only need 
>   ((\.0)*\.[1-9][0-9]*)*.
> - The outside - in PRE part (\-([a-zA-Z0-9]+))? doesn't need escaping.
>   Simplify it to (-([a-zA-Z0-9]+))?

Both done.

> - Do you want to allow only a plus without the number in the BUILD
?   part? Why do you capture the +? ((\+)(0|[1-9][0-9]*)?)?

I need to capture the plus to distinguish between cases where an empty build is 
allowed (e.g. "9+-foo") and when it is not ("9+").  
See code in Version.java, line 226-230 and in Basic.java, line 98, 107-109.  
(Note that we use the empty "+" to distinguish "9-foo" from
"9+-foo".)

> - Dot loses meaning in the character class, and hyphen loses meaning 
> at the beginning or at the end of the character class. You can 
> simplify the OPT part to (-([-a-zA-Z0-9.]+))?

Done.

> - You might want to consider using named-capturing groups instead of 
> numbered capturing groups.  Also, consider using non-capturing groups 
> for groups you don't need to extract.

Done.

This is the (hopefully) final version of the webrev and javadoc for the initial 
implementation of this API:

  http://cr.openjdk.java.net/~iris/verona/8072379/webrev.3/index.html
  http://cr.openjdk.java.net/~iris/verona/8072379/doc.3/jdk/Version.html

I've filed the following bug to update the spec:

8148822: (spec) Regex in jdk.Version and JEP 223 should match
https://bugs.openjdk.java.net/browse/JDK-8148822

These are the corresponding changes to the JEP which I'll apply in the next day 
or so:

$ diff jep-open-mr11.md jep-open-mr12.md
84c84
< `^[1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*$`. The sequence may be of arbitrary
---
> `^[1-9][0-9]*((\.0)*\.[1-9][0-9]*)*$`. The sequence may be of 
> arbitrary
166c166
<   -  $OPT, matching `([-a-zA-Z0-9\.]+)` --- Additional
---
>   -  $OPT, matching `([-a-zA-Z0-9.]+)` --- 
> Additional

Thanks again for the recommendations.

Regards,
iris

-Original Message-
From: Thanh Hong Dai [mailto:hdth...@tma.com.vn]
Sent: Wednesday, January 13, 2016 7:53 PM
To: Iris Clark; Alan Bateman; core-libs-dev@openjdk.java.net; 
verona-...@openjdk.java.net
Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi,

Some comment on the regex (and also the JEP):

([1-9][0-9]*(((\.0)*\.[1-9][0-9]*)*)*)(\-([a-zA-Z0-9]+))?((\+)(0|[1-9][0-9]*)?)?(-([\-a-zA-Z0-9\.]+))?

- The outer most quantifier in (((\.0)*\.[1-9][0-9]*)*)* is redundant and a 
source of catastrophic backtracking. You only need ((\.0)*\.[1-9][0-9]*)*.
- The outside - in PRE part (\-([a-zA-Z0-9]+))? doesn't need escaping. Simplify 
it to (-([a-zA-Z0-9]+))?
- Do you want to allow only a plus without the number in the BUILD part? Why do 
you capture the +? ((\+)(0|[1-9][0-9]*)?)?
- Dot loses meaning in the character class, and hyphen loses meaning at the 
beginning or at the end of the character class. You can simplify the OPT part 
to (-([-a-zA-Z0-9.]+))?
- You might want to consider using named-capturing groups instead of numbered 
capturing groups. Also, consider using non-capturing groups for groups you 
don't need to extract.

Best regards,
Hong Dai Thanh.

-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf 
Of Iris Clark
Sent: Thursday, 14 January, 2016 4:55 AM
To: Alan Bateman ; core-libs-dev@openjdk.java.net; 
verona-...@openjdk.java.net
Subject: RE: RFR: 8072379: Implement jdk.Version and jdk.OracleVersion

Hi, Alan.

Thanks for looking at