Re: RFR 8151486 : Class.forName causes memory leak

[David Holmes]

On 6/10/2016 9:19 AM, Mandy Chung wrote:



On Oct 5, 2016, at 4:02 PM, David Holmes  wrote:

On 6/10/2016 8:55 AM, Mandy Chung wrote:



Note that PD is the protection domain of an initiating class that resolves a 
target type T.  PD is kept in T’s class loader L.  It’s not the protection 
domains of the classes defined by L.  VM keeps its own cache of protection 
domains that have been validated with a resolved type.  VM does not depend on 
ClassLoader::domains field to keep it alive.  IIUC, it’s kept as a weak 
reference in VM which is what we want here.


Okay but this will still affect the lifecycle of the PDs because without the 
strong reference in L, those weak references in the VM will quickly be cleared. 
I'm unclear what the implications of this might be, but I doubt regular 
short-lived tests would expose any issues.


There is no issue there.

PD will be GC’ed together when the classes along with its class loader are 
unloaded. These class loaders may be one or more and they are L1, L2 …. != L.


Okay, thanks for walking me through this.

David


Mandy

Re: Proposal for adding O_DIRECT support into JDK 9

[Brian Burkhalter]

Hi Lucy,

On Oct 4, 2016, at 5:35 PM, Lu, Yingqi  wrote:

> Thank you very much for reviewing the patch and providing the detailed 
> error/warning messages. They are very helpful!

You are welcome.

> We will look into the issues and solve them in the next version of the patch. 
> Test cases for O_DIRECT and modified copyright year will be added in next 
> version too.

Given that the functionality of O_DIRECT on Linux appears to be supported by 
other interfaces on OS X, Solaris, and Windows, I wonder whether the patch will 
need to be refactored in some way to accommodate these other operating systems? 
For reference it looks as if direct I/O on OS X uses the F_NOCACHE command of 
fcntl(2) [1] (although per some online comments this might have some problems), 
Solaris uses the advice argument of directio(3c) [2], and Windows uses a 
combination of flags passed to CreateFile() [3, 4].

Thanks,

Brian

[1] 
https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man2/fcntl.2.html
[2] https://docs.oracle.com/cd/E26505_01/html/816-5168/directio-3c.html
[3] https://support.microsoft.com/en-us/kb/99794
[4] 
https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx

Re: RFR 8151486 : Class.forName causes memory leak

[Mandy Chung]

> On Oct 5, 2016, at 4:02 PM, David Holmes  wrote:
> 
> On 6/10/2016 8:55 AM, Mandy Chung wrote:
>> 
>> 
>> Note that PD is the protection domain of an initiating class that resolves a 
>> target type T.  PD is kept in T’s class loader L.  It’s not the protection 
>> domains of the classes defined by L.  VM keeps its own cache of protection 
>> domains that have been validated with a resolved type.  VM does not depend 
>> on ClassLoader::domains field to keep it alive.  IIUC, it’s kept as a weak 
>> reference in VM which is what we want here.
> 
> Okay but this will still affect the lifecycle of the PDs because without the 
> strong reference in L, those weak references in the VM will quickly be 
> cleared. I'm unclear what the implications of this might be, but I doubt 
> regular short-lived tests would expose any issues.

There is no issue there.

PD will be GC’ed together when the classes along with its class loader are 
unloaded. These class loaders may be one or more and they are L1, L2 …. != L.

Mandy

Re: RFR: JDK-8134373: explore potential uses of convenience factories within the JDK

[Stuart Marks]

On 10/5/16 12:32 PM, Stephen Colebourne wrote:

On 5 October 2016 at 17:07, Jonathan Bluett-Duncan
 wrote:

Stephen, thank you for getting back about DateTimeFormatter. It's not clear
to me what should be done with
TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
delete it; or do I change it to test that a null TemporalField param causes
a NullPointerException to be thrown; or do I do something else? May I have
your continued thoughts on this?


I think you should perform no change to DateTimeFormatter (other than
a comment) as part of this changeset. The behaviour of that
DateTimeFormatter method is subtle, and I now suspect that what we
have there might be the best option.


I had recommended removing the comment from DateTimeFormatter, but if Stephen 
wants the comment in, that's fine with me. In fact I'll defer to what Stephen 
(and Roger Riggs) want with this code, since they're the maintainers.


s'marks

Re: RFR 8151486 : Class.forName causes memory leak

[David Holmes]

On 6/10/2016 8:55 AM, Mandy Chung wrote:



On Oct 5, 2016, at 2:26 PM, David Holmes  wrote:

On 6/10/2016 6:19 AM, Mandy Chung wrote:



Since domains is not used and not intended to keep PD alive and VM maintains 
its own cache of these initiating PD for performance, removing domains field 
looks good to me.


What then is intended to keep PD alive? Or do we not need to keep them alive?

I have always assumed that domains was used to keep the PD alive for as long as 
the classloader is alive.



Note that PD is the protection domain of an initiating class that resolves a 
target type T.  PD is kept in T’s class loader L.  It’s not the protection 
domains of the classes defined by L.  VM keeps its own cache of protection 
domains that have been validated with a resolved type.  VM does not depend on 
ClassLoader::domains field to keep it alive.  IIUC, it’s kept as a weak 
reference in VM which is what we want here.


Okay but this will still affect the lifecycle of the PDs because without 
the strong reference in L, those weak references in the VM will quickly 
be cleared. I'm unclear what the implications of this might be, but I 
doubt regular short-lived tests would expose any issues.


David
-


Brent did the archeology and found that the initial fix introduced the domains 
field attempted to use it.  But the initial fix was back out and reimplemented 
and domains is not used.  In any case, this field is not used and depended by 
VM.

Mandy

Re: RFR 8151486 : Class.forName causes memory leak

[Mandy Chung]

> On Oct 5, 2016, at 2:26 PM, David Holmes  wrote:
> 
> On 6/10/2016 6:19 AM, Mandy Chung wrote:
>> 
>> 
>> Since domains is not used and not intended to keep PD alive and VM maintains 
>> its own cache of these initiating PD for performance, removing domains field 
>> looks good to me.
> 
> What then is intended to keep PD alive? Or do we not need to keep them alive?
> 
> I have always assumed that domains was used to keep the PD alive for as long 
> as the classloader is alive.


Note that PD is the protection domain of an initiating class that resolves a 
target type T.  PD is kept in T’s class loader L.  It’s not the protection 
domains of the classes defined by L.  VM keeps its own cache of protection 
domains that have been validated with a resolved type.  VM does not depend on 
ClassLoader::domains field to keep it alive.  IIUC, it’s kept as a weak 
reference in VM which is what we want here.

Brent did the archeology and found that the initial fix introduced the domains 
field attempted to use it.  But the initial fix was back out and reimplemented 
and domains is not used.  In any case, this field is not used and depended by 
VM. 

Mandy

Re: RFR 8151486 : Class.forName causes memory leak

[Brent Christian]

Yes!  Good catch, thanks.
-Brent

On 10/5/16 2:08 PM, Naoto Sato wrote:

Typo in ClassForNameLeak.java? At line 103, "change" -> "chance"?

Naoto

On 10/5/16 12:38 PM, Brent Christian wrote:

Hi,

Please review my fix for 8151486, "Class.forName causes memory leak".

Bug:
https://bugs.openjdk.java.net/browse/JDK-8151486
Webrev:
http://cr.openjdk.java.net/~bchristi/8151486/webrev.00/

The test case reproduces the bug as such:

With a SecurityManager enabled, a class ("ClassForName") is loaded from
a user classloader (isa URLClassLoader, "LeakedClassLoader"), and from
that class makes a call to Class.forName() on the system classloader.
(The specific class name isn't so important, I just used
java.util.List).  The result is that the user's classloader is never
collected.

The leak occurs because of the following:

Class.forName0() is passed the "caller" class, ClassForName.

JVM_FindClassFromCaller will "find a class with this name
(java.util.List) in this loader (the system classloader), using the
caller's (ClassForName's) protection domain.  A ProtectionDomain is
created for ClassForName, with ProtectionDomain.classloader referring to
LeakedClassLoader.

This PD is passed to ClassLoader.checkPackageAccess(), and is added to
'domains' of the system classloader (ClassLoader.java line 643).  Thus,
the system classloader holds a reference to the user's classloader via
'domains'.

Nothing is ever removed from 'domains'.  In fact, besides being added
to, I found no other uses of 'domains' - not in library code, not in
hotspot.  (From my research, it's questionable if 'domains' was ever
used).

Removal of 'domains' fixes the leak, and cleans out a little bit of
unused code.

Automated building and testing (JPRT, RBT) looks fine.

Thanks!
-Brent

Re: RFR 8151486 : Class.forName causes memory leak

[David Holmes]

On 6/10/2016 6:19 AM, Mandy Chung wrote:



On Oct 5, 2016, at 12:38 PM, Brent Christian  wrote:

Hi,

Please review my fix for 8151486, "Class.forName causes memory leak".

Bug:
https://bugs.openjdk.java.net/browse/JDK-8151486
Webrev:
http://cr.openjdk.java.net/~bchristi/8151486/webrev.00/



Since domains is not used and not intended to keep PD alive and VM maintains 
its own cache of these initiating PD for performance, removing domains field 
looks good to me.


What then is intended to keep PD alive? Or do we not need to keep them 
alive?


I have always assumed that domains was used to keep the PD alive for as 
long as the classloader is alive.


Thanks,
David
-



Nit in the new test: a few long lines.  Good to have line breaks.

  64 ClassLoader classLoader = new URLClassLoader(new 
URL[]{jarFilePath.toFile().toURI().toURL()}) {

You can call Path.toUri().toURL()

  78 Path testClassesDir = FileSystems.getDefault().getPath(

You can replace this with Paths.get(…)

Otherwise looks good.

Mandy

Re: RFR 8151486 : Class.forName causes memory leak

[Naoto Sato]

Typo in ClassForNameLeak.java? At line 103, "change" -> "chance"?

Naoto

On 10/5/16 12:38 PM, Brent Christian wrote:

Hi,

Please review my fix for 8151486, "Class.forName causes memory leak".

Bug:
https://bugs.openjdk.java.net/browse/JDK-8151486
Webrev:
http://cr.openjdk.java.net/~bchristi/8151486/webrev.00/

The test case reproduces the bug as such:

With a SecurityManager enabled, a class ("ClassForName") is loaded from
a user classloader (isa URLClassLoader, "LeakedClassLoader"), and from
that class makes a call to Class.forName() on the system classloader.
(The specific class name isn't so important, I just used
java.util.List).  The result is that the user's classloader is never
collected.

The leak occurs because of the following:

Class.forName0() is passed the "caller" class, ClassForName.

JVM_FindClassFromCaller will "find a class with this name
(java.util.List) in this loader (the system classloader), using the
caller's (ClassForName's) protection domain.  A ProtectionDomain is
created for ClassForName, with ProtectionDomain.classloader referring to
LeakedClassLoader.

This PD is passed to ClassLoader.checkPackageAccess(), and is added to
'domains' of the system classloader (ClassLoader.java line 643).  Thus,
the system classloader holds a reference to the user's classloader via
'domains'.

Nothing is ever removed from 'domains'.  In fact, besides being added
to, I found no other uses of 'domains' - not in library code, not in
hotspot.  (From my research, it's questionable if 'domains' was ever used).

Removal of 'domains' fixes the leak, and cleans out a little bit of
unused code.

Automated building and testing (JPRT, RBT) looks fine.

Thanks!
-Brent

Re: RFR: JDK-8134373: explore potential uses of convenience factories within the JDK

[Roger Riggs]

Hi,

I would leave the test and code as is for the purposes of 8133273
and leave it to later to resolve 8167222.  There is no compelling need 
to change it.


$.02, Roger

On 10/5/2016 4:45 PM, Stuart Marks wrote:

Stephen,

Thanks for the quick followup clarifications. I'm not entirely sure 
how you'd like to proceed; see discussion below.


Jonathan, also see below.


On 10/5/16 9:07 AM, Jonathan Bluett-Duncan wrote:
Stuart, thank you very much for your continued review of this 
changeset, and for
finding those usages of CookieManager::get in Grepcode for me. I've 
applied the

comment you suggested for ModuleFinder and I've also fixed the
NetscapeCookieStore typo.


Great!

Stephen, thank you for getting back about DateTimeFormatter. It's not 
clear to

me what should be done with
TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. 
Do I
delete it; or do I change it to test that a null TemporalField param 
causes a
NullPointerException to be thrown; or do I do something else? May I 
have your

continued thoughts on this?


OK, this is kind of subtle. This is a TCK (conformance) test, so it 
probably cannot simply be removed; it may need a specification change 
to clarify this case. I've filed JDK-8167222 to cover these issues. 
I've made a note in this bug regarding the potential change in 
DateTimeFormatter.withResolverFields() to use Set.of().


(There's an additional wrinkle with Set.of() aside from rejecting 
nulls; it also rejects duplicates by throwing IAE.)


In any case that code can't be changed to use Set.of() until the 
test/spec issue is resolved, so for the purposes of this changeset, 
I'd suggest simply removing the withResolverFields() comment from the 
webrev. We can revisit this during or after the resolution of 
JDK-8167222.


I think this clears all the issues, so you can probably go ahead and 
work with Patrick to update the webrev. And Patrick, thanks once again 
for hosting the webrev!


s'marks

Re: RFR: JDK-8134373: explore potential uses of convenience factories within the JDK

[Stuart Marks]

Stephen,

Thanks for the quick followup clarifications. I'm not entirely sure how you'd 
like to proceed; see discussion below.


Jonathan, also see below.


On 10/5/16 9:07 AM, Jonathan Bluett-Duncan wrote:

Stuart, thank you very much for your continued review of this changeset, and for
finding those usages of CookieManager::get in Grepcode for me. I've applied the
comment you suggested for ModuleFinder and I've also fixed the
NetscapeCookieStore typo.


Great!


Stephen, thank you for getting back about DateTimeFormatter. It's not clear to
me what should be done with
TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
delete it; or do I change it to test that a null TemporalField param causes a
NullPointerException to be thrown; or do I do something else? May I have your
continued thoughts on this?


OK, this is kind of subtle. This is a TCK (conformance) test, so it probably 
cannot simply be removed; it may need a specification change to clarify this 
case. I've filed JDK-8167222 to cover these issues. I've made a note in this bug 
regarding the potential change in DateTimeFormatter.withResolverFields() to use 
Set.of().


(There's an additional wrinkle with Set.of() aside from rejecting nulls; it also 
rejects duplicates by throwing IAE.)


In any case that code can't be changed to use Set.of() until the test/spec issue 
is resolved, so for the purposes of this changeset, I'd suggest simply removing 
the withResolverFields() comment from the webrev. We can revisit this during or 
after the resolution of JDK-8167222.


I think this clears all the issues, so you can probably go ahead and work with 
Patrick to update the webrev. And Patrick, thanks once again for hosting the webrev!


s'marks

Re: RFR 8151486 : Class.forName causes memory leak

[Mandy Chung]

> On Oct 5, 2016, at 12:38 PM, Brent Christian  
> wrote:
> 
> Hi,
> 
> Please review my fix for 8151486, "Class.forName causes memory leak".
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8151486
> Webrev:
> http://cr.openjdk.java.net/~bchristi/8151486/webrev.00/
> 

Since domains is not used and not intended to keep PD alive and VM maintains 
its own cache of these initiating PD for performance, removing domains field 
looks good to me.

Nit in the new test: a few long lines.  Good to have line breaks.

  64 ClassLoader classLoader = new URLClassLoader(new 
URL[]{jarFilePath.toFile().toURI().toURL()}) {

You can call Path.toUri().toURL()

  78 Path testClassesDir = FileSystems.getDefault().getPath(

You can replace this with Paths.get(…)

Otherwise looks good.

Mandy

Re: [9] RFR of JDK-8085192: java/rmi/activation/Activatable tests fail intermittently due to "Port already in use"

[Roger Riggs]

Hi Chris,

Some comments,

BTW, there is a newer version of Webrev that provides convenient next 
and prev links...



* sun/rmi/server/Activation.java: 1973
 - I'd stick to the normal set of values for a boolean system property: 
use java.lang.Boolean.getProperty("sun.rmi").


* With so many dependencies, how about adding the modules = to an 
appropriate TEST.properties file.


* With the extra security permissions and breaking into package private 
utilities

   this test is going to be more fragile and harder to work with.

* 
test/java/rmi/activation/Activatable/checkAnnotations/CheckAnnotations.java

 - line 184, leftover println...  (or incorrectly indented).
 - line 226 ditto?

* test/java/rmi/activation/Activatable/extLoadedImpl/ExtLoadedImplTest.java
+ 
test/java/rmi/activation/ActivateFailedException/activateFails/ActivateFails.java
+ 
test/java/rmi/activation/ActivationGroup/downloadActivationGroup/DownloadActivationGroup.java
+ 
test/java/rmi/activation/ActivationSystem/activeGroup/IdempotentActiveGroup.java
+ 
test/java/rmi/activation/ActivationSystem/modifyDescriptor/ModifyDescriptor.java
+ 
test/java/rmi/activation/ActivationSystem/stubClassesPermitted/StubClassesPermitted.java
+ 
test/java/rmi/activation/ActivationSystem/unregisterGroup/UnregisterGroup.java

+ test/java/rmi/activation/CommandEnvironment/SetChildEnv.java
+ 
test/java/rmi/activation/rmidViaInheritedChannel/InheritedChannelNotServerSocket.java
+ 
test/java/rmi/activation/rmidViaInheritedChannel/RmidViaInheritedChannel.java

+
 - Do these tests also need @module java.base/sun.nio.ch

* I wonder of the security.policy and rmid.security.policy files can be 
(mostly) shared.

  (but that's not really this issue)

* test/java/rmi/testlibrary/RMID.java
 - 139: typo "do no" -> "do not"

I'm vaguely not very comfortable with scraping the port number off stdout
and the inherited channel pieces seem like a lot of moving parts.

Roger

p.s.  Anyother idea
I assume not all platforms can allow separate processes to open server 
sockets to the same port.
If so, we would just have the client allocate a port (0), mark it 
non-exclusive and keep it open
while passing the port number to RMID. Only after RMID is started close 
the allocating socket.







On 10/3/2016 11:42 AM, Chris Hegarty wrote:

Here is an updated version of this ( ready for review ):

  http://cr.openjdk.java.net/~chegar/8085192_webrev.02/

Changes from previous:

1) Updated Activation/rmid to NOT redirect stderr, if an
   implementation specific system property is used ( we can
   discuss the name )

2) For now, I added createRMIDOnEphemeralPort, rather than
   change the current implementation, as it is being used in
   other places. We can revert this once other usages are
   updated and verified.

-Chris.

On 29/09/16 20:09, Chris Hegarty wrote:
On 29 Sep 2016, at 16:25, Chris Hegarty  
wrote:


I have asked Hamlin to hold off on this for a day or so.  I have an
alternative proposal that eliminates the free port anti-pattern.


It is possible to use the inheritedChannel mechanism to have the rmid
process create the server channel on an ephemeral port and report it
back to the test, i.e. remove the free port pattern.

http://cr.openjdk.java.net/~chegar/8085192_webrev/

1) The port number is reported from rmid to the test over stdout.

2) All tests pass except CheckAnnotations.java, which looks for stderr
( see 3 below ). I think the stderr check can be removed, and the
just check stdout.

3)  rmid, when using inheritChannel, redirects stderr to a tmp file, so
 it is not possible to get the processes stderr over the processes
 stderr pipe. I did’t find that this was an issue when testing ( 
other

 than having to clear out /tmp )

4) This could be expanded to other tests, outside of activation, to
 remove more usages of free port.

This is not yet complete, I just want to share the idea to see if it 
is a

runner, or not.

-Chris.

RFR 8151486 : Class.forName causes memory leak

[Brent Christian]

Hi,

Please review my fix for 8151486, "Class.forName causes memory leak".

Bug:
https://bugs.openjdk.java.net/browse/JDK-8151486
Webrev:
http://cr.openjdk.java.net/~bchristi/8151486/webrev.00/

The test case reproduces the bug as such:

With a SecurityManager enabled, a class ("ClassForName") is loaded from 
a user classloader (isa URLClassLoader, "LeakedClassLoader"), and from 
that class makes a call to Class.forName() on the system classloader. 
(The specific class name isn't so important, I just used 
java.util.List).  The result is that the user's classloader is never 
collected.


The leak occurs because of the following:

Class.forName0() is passed the "caller" class, ClassForName.

JVM_FindClassFromCaller will "find a class with this name 
(java.util.List) in this loader (the system classloader), using the 
caller's (ClassForName's) protection domain.  A ProtectionDomain is 
created for ClassForName, with ProtectionDomain.classloader referring to 
LeakedClassLoader.


This PD is passed to ClassLoader.checkPackageAccess(), and is added to 
'domains' of the system classloader (ClassLoader.java line 643).  Thus, 
the system classloader holds a reference to the user's classloader via 
'domains'.


Nothing is ever removed from 'domains'.  In fact, besides being added 
to, I found no other uses of 'domains' - not in library code, not in 
hotspot.  (From my research, it's questionable if 'domains' was ever used).


Removal of 'domains' fixes the leak, and cleans out a little bit of 
unused code.


Automated building and testing (JPRT, RBT) looks fine.

Thanks!
-Brent

Re: RFR: JDK-8134373: explore potential uses of convenience factories within the JDK

[Stephen Colebourne]

On 5 October 2016 at 17:07, Jonathan Bluett-Duncan
 wrote:
> Stephen, thank you for getting back about DateTimeFormatter. It's not clear
> to me what should be done with
> TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
> delete it; or do I change it to test that a null TemporalField param causes
> a NullPointerException to be thrown; or do I do something else? May I have
> your continued thoughts on this?

I think you should perform no change to DateTimeFormatter (other than
a comment) as part of this changeset. The behaviour of that
DateTimeFormatter method is subtle, and I now suspect that what we
have there might be the best option.

Stephen

RFR: 8166460: jdk/internal/util/jar/TestVersionedStream gets Assertion error

[Steve Drach]

Hi,

Please review the following changeset that creates a replacement for the 
TestVersionedStream test.  The previous test occasionally failed on Linux 
hotspot nightly testing due to jar tool sometimes changing the order of the 
entries.  The new test specifically sets the order of the entries and tests 
both possible orders.

issue: https://bugs.openjdk.java.net/browse/JDK-8166460 

webrev: http://cr.openjdk.java.net/~sdrach/8166460/webrev.00/ 


Thanks,
Steve

Re: Review Request JDK-8166846: jdeps fails to generate module info if there is any class in unnamed package

[Mandy Chung]

Lance,

Thanks for the review. jdeps.properties updated:

err.genmoduleinfo.not.jarfile={0} is a modular JAR file that cannot be 
specified with the --generate-module-info option
err.genmoduleinfo.unnamed.package={0} contains an unnamed package that is not 
allowed in a module

Mandy

> On Oct 5, 2016, at 11:29 AM, Lance Andersen  wrote:
> 
> Hi Mandy
> 
> The code changes look good.
> 
> A couple of minor suggestions
> 
> This looks good.  Only thought I had is 
> 
> err.genmoduleinfo.unnamed.package={0} contains unnamed package; not a valid 
> module to generate module-info.java
> perhaps change it to:
> 
>   {0} contains an unnamed package that is not allowed in a module.
> 
> err.genmoduleinfo.not.jarfile={0} is a modular JAR file; not valid for 
> --generate-module-info option
> I might tweak this to be:
> 
>   {0} is a modular JAR file and may not be specified with the 
> —generate-module-info option
> 
> 
> HTH,
> 
> Best,
> Lance
>> On Oct 4, 2016, at 10:50 PM, Mandy Chung  wrote:
>> 
>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8166846/webrev.00/
>> 
>> This improves jdeps error message to indicate that a JAR file containing 
>> unnamed package is not valid to generate module-info.java.
>> 
>> Mandy
> 
> 
> 
> 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: Review Request JDK-8167014: jdeps: Missing message: warn.skipped.entry

[Alan Bateman]

On 05/10/2016 03:52, Mandy Chung wrote:


http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8167014/webrev.00/index.html

This fixes the missing key in the resource bundle and also include the 
exception message of the invalid entry.  I verified with a JAR file with bad 
entries.


This looks okay to me.

-Alan

Re: Review Request JDK-8166846: jdeps fails to generate module info if there is any class in unnamed package

[Lance Andersen]

Hi Mandy

The code changes look good.

A couple of minor suggestions

This looks good.  Only thought I had is 

err.genmoduleinfo.unnamed.package={0} contains unnamed package; not a valid 
module to generate module-info.java
perhaps change it to:

{0} contains an unnamed package that is not allowed in a module.

err.genmoduleinfo.not.jarfile={0} is a modular JAR file; not valid for 
--generate-module-info option
I might tweak this to be:

{0} is a modular JAR file and may not be specified with the 
—generate-module-info option


HTH,

Best,
Lance
> On Oct 4, 2016, at 10:50 PM, Mandy Chung  wrote:
> 
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8166846/webrev.00/
> 
> This improves jdeps error message to indicate that a JAR file containing 
> unnamed package is not valid to generate module-info.java.
> 
> Mandy

 
  

 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: JDK-8134373: explore potential uses of convenience factories within the JDK

[Patrick Reinhart]

Hi Jonathan,

As soon you got all the changes together, we can go thru them and I will 
recreate the webrev…

-Patrick

> Am 05.10.2016 um 18:07 schrieb Jonathan Bluett-Duncan 
> :
> 
> Stuart, thank you very much for your continued review of this changeset,
> and for finding those usages of CookieManager::get in Grepcode for me. I've
> applied the comment you suggested for ModuleFinder and I've also fixed the
> NetscapeCookieStore typo.
> 
> Stephen, thank you for getting back about DateTimeFormatter. It's not clear
> to me what should be done with
> TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
> delete it; or do I change it to test that a null TemporalField param causes
> a NullPointerException to be thrown; or do I do something else? May I have
> your continued thoughts on this?
> 
> Kind regards,
> Jonathan

Re: RFR JDK-8166261: Scanner.nextInt(int) (and similar methods) throws PatternSyntaxException

[Naoto Sato]

Looks good to me.

The test case could use IntStream.rangeClosed(Character.MIN_RADIX, 
Character.MAX_RADIX) for the good radixes, instead of hard coding ints.


Naoto

On 10/5/16 8:53 AM, Xueming Shen wrote:

Hi

Please help review

issue:  https://bugs.openjdk.java.net/browse/JDK-8166261
webre: http://cr.openjdk.java.net/~sherman/8166261/webrev

The radix sanity check are missing  from
hasNextByte/Short/Int/Long/BigInteger().
The only method we are doing now is useRadix(). The proposed change here
is to
add the check into all above methods that take a radix.

Arguably it's an incompatible api change, but I don't expect it really
breaks anyone's
code.  Need go through ccc if approved.

Thanks,
Sherman

Re: [concurrency-interest] Deprecate all java.util.concurrent.*FieldUpdater

[Andrew Haley]

On 05/10/16 17:55, Vladimir Ivanov wrote:
>>> My interpretation of Martin's comment is that using deprecation for
>>> things that aren't actually broken just means that people will live
>>> with lots of deprecation warnings in their code for years to
>>> come. This could be a perfect case for introducing a weaker
>>> alternative to deprecation, saying "here's something better that you
>>> should really be using for new code" -- e.g. ArrayList vs. Vector. I
>>> remember the Guava team talking about that a lot. Don't know if they
>>> ever implemented it.
>>
>> OK.  But we really do need a way to do that.
> Doesn't enhanced deprecation [1] in 9 cover that?

I can't tell.  As far as I can see there is no annotation which
covers exactly our case:

the API is flawed and is impractical to fix,

usage of the API is likely to lead to errors,

the API has been superseded by another API,

the API is obsolete,

the API is experimental and is subject to incompatible changes,

or any combination of the above.

we'd perhaps need to add

this method over here is better in most cases

...which perhaps implies obsolescent rather than obsolete.

Andrew.

Re: [concurrency-interest] Deprecate all java.util.concurrent.*FieldUpdater

[Vladimir Ivanov]

My interpretation of Martin's comment is that using deprecation for
things that aren't actually broken just means that people will live
with lots of deprecation warnings in their code for years to
come. This could be a perfect case for introducing a weaker
alternative to deprecation, saying "here's something better that you
should really be using for new code" -- e.g. ArrayList vs. Vector. I
remember the Guava team talking about that a lot. Don't know if they
ever implemented it.


OK.  But we really do need a way to do that.

Doesn't enhanced deprecation [1] in 9 cover that?

Best regards,
Vladimir Ivanov

[1] http://openjdk.java.net/jeps/277

Re: RFR: JDK-8134373: explore potential uses of convenience factories within the JDK

[Jonathan Bluett-Duncan]

Stuart, thank you very much for your continued review of this changeset,
and for finding those usages of CookieManager::get in Grepcode for me. I've
applied the comment you suggested for ModuleFinder and I've also fixed the
NetscapeCookieStore typo.

Stephen, thank you for getting back about DateTimeFormatter. It's not clear
to me what should be done with
TCKDateTimeFormatter::test_resolverFields_listOfOneNull in this case. Do I
delete it; or do I change it to test that a null TemporalField param causes
a NullPointerException to be thrown; or do I do something else? May I have
your continued thoughts on this?

Kind regards,
Jonathan

RE: [concurrency-interest] Deprecate all java.util.concurrent.*FieldUpdater

[Justin Sampson]

Deprecation is stronger than that -- it says "we're going to remove this, or we 
wish we could remove it because it's broken, so you'd better change your code 
a.s.a.p." -- e.g. Thread.stop(). My interpretation of Martin's comment is that 
using deprecation for things that aren't actually broken just means that people 
will live with lots of deprecation warnings in their code for years to come. 
This could be a perfect case for introducing a weaker alternative to 
deprecation, saying "here's something better that you should really be using 
for new code" -- e.g. ArrayList vs. Vector. I remember the Guava team talking 
about that a lot. Don't know if they ever implemented it.

Cheers,
Justin


-Original Message-
From: Concurrency-interest [mailto:concurrency-interest-boun...@cs.oswego.edu] 
On Behalf Of Andrew Haley
Sent: Wednesday, October 05, 2016 1:19 AM
To: Martin Buchholz; Remi Forax; Paul Sandoz
Cc: core-libs-dev; concurrency-interest
Subject: Re: [concurrency-interest] Deprecate all 
java.util.concurrent.*FieldUpdater

On 04/10/16 22:19, Martin Buchholz wrote:
> VarHandle is a reasonable replacement for FieldUpdaters, but it's not yet
> complete (where is accumulateAndGet?), and when do you deprecate something
> when the replacement won't be ubiquitous for 10 years?

Surely you have to start somewhere: deprecation is no more than saying
to programmers "Don't use this, use that."  And if you were leaning
over someone's shoulder that's what you would say.

Andrew.


___
Concurrency-interest mailing list
concurrency-inter...@cs.oswego.edu
http://cs.oswego.edu/mailman/listinfo/concurrency-interest

RFR JDK-8166261: Scanner.nextInt(int) (and similar methods) throws PatternSyntaxException

[Xueming Shen]

Hi

Please help review

issue:  https://bugs.openjdk.java.net/browse/JDK-8166261
webre: http://cr.openjdk.java.net/~sherman/8166261/webrev

The radix sanity check are missing  from 
hasNextByte/Short/Int/Long/BigInteger().
The only method we are doing now is useRadix(). The proposed change here 
is to

add the check into all above methods that take a radix.

Arguably it's an incompatible api change, but I don't expect it really 
breaks anyone's

code.  Need go through ccc if approved.

Thanks,
Sherman

Re: [concurrency-interest] Deprecate all java.util.concurrent.*FieldUpdater

[Andrew Haley]

On 05/10/16 15:39, Justin Sampson wrote:

> Deprecation is stronger than that -- it says "we're going to remove
> this, or we wish we could remove it because it's broken, so you'd
> better change your code a.s.a.p." -- e.g. Thread.stop().

We're moving Java to support some new ways of working, and these
changes will inevitably mean that some parts of the project will be
obsolescent.  We need to be able to flag the old ways of doing things
in some way.

Deprecation is a way to do that.  I don't think that Thread.stop() is
a typical case.

> My interpretation of Martin's comment is that using deprecation for
> things that aren't actually broken just means that people will live
> with lots of deprecation warnings in their code for years to
> come. This could be a perfect case for introducing a weaker
> alternative to deprecation, saying "here's something better that you
> should really be using for new code" -- e.g. ArrayList vs. Vector. I
> remember the Guava team talking about that a lot. Don't know if they
> ever implemented it.

OK.  But we really do need a way to do that.

Andrew.

Re: RFR(XS): 8166800: [s390] Top-level build changes required for Linux/s390x

[Volker Simonis]

That would be really great!

Could you please do it in the jdk9/hs forest as that's the place where
we will need it first (i.e. when integrating the whole s390 hotspot
platform files).

Thanks,
Volker

On Wed, Oct 5, 2016 at 5:07 PM, Erik Joelsson  wrote:
> Looks ok to me. I can sponsor this tomorrow.
>
> /Erik
>
>
>
> On 2016-10-05 16:43, Volker Simonis wrote:
>>
>> Hi,
>>
>> can I please have a review and sponsor for the following tiny
>> top-level build change required for building the OpenJDK on
>> Linux/s390:
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2016/s390x/8166800/
>> https://bugs.openjdk.java.net/browse/JDK-8166800
>>
>> All this change does is to add some s390-specifc C and C++ flags for
>> the Hotspot and the class-library build which do not affect any of the
>> existing platforms.
>>
>> Thank you and best regards,
>> Volker
>
>

Re: RFR: 8159855: Create an SPI for tools

[Jonathan Gibbons]

On 10/5/16 3:34 AM, Alan Bateman wrote:

On 05/10/2016 10:54, Stephen Colebourne wrote:


Interesting.

How likely is it that there will be more than one tool of a given name
available? The method name findFirst() seems relatively odd for the
lookup operation.

I'd also note that the name string to pass in are "magic". There are
no constants defined for callers to use. Since there is no obvious way
in the API to find the vendor, this could get tricky across JDKs. Plus
how would a caller know what arguments to pass if each vendors tool
differs?

Just being cautious about the use case being solved.
Many command line tools don't define an API and so not unheard of to 
find code that uses sun.tools.jar.Main.main or the main method of 
other tools. Also common to see code using ProcessBuilder or 
Runtime.exec to launch tools like jar. If the commonly used tools 
(jar, jdeps, ...) document their tool names in their module 
description then it will make it easier to run these tools in the same 
VM. It does mean that some tools will have both an API and a 
documented tool name but that shouldn't be an issue. I'm sure Jon has 
a lot to say on this topic, esp. with linking usage documentation and 
man pages.


As regards constants then not clear where something like this would 
live as it amounts to a registry. Also many of the tools are JDK or 
library specific and so wouldn't have a place in the standard/Java SE 
docs.


The firstFirst(String) method limits itself to tools that are visible 
via the system class loader. Sure, someone might decide to deploy lots 
of libraries that all claim to be the "hammer" tool but this is no 
different to many of the service providers mechanisms. One could have 
the ToolProvider define methods with the tool capabilities that would 
aid selection but that would complicate the API.


-Alan


Informally, I think the common use case for this API is to get "same VM" 
access to tools in the bin/ directory of the product image. That implies 
there is an obvious name for each tool, and that there will be 
documentation for the arguments that each tool that is available.


Also, in general, the doc comments for a module should contain details 
of any services for general use, including details of how to access the 
service (e.g. what name to use, in this case) and how to use it (info 
about arguments, etc, in this case.)  We are working on updating the 
javadoc tool to better support the provision of such information.


-- Jon

Re: RFR(XS): 8166800: [s390] Top-level build changes required for Linux/s390x

[Erik Joelsson]

Looks ok to me. I can sponsor this tomorrow.

/Erik


On 2016-10-05 16:43, Volker Simonis wrote:

Hi,

can I please have a review and sponsor for the following tiny
top-level build change required for building the OpenJDK on
Linux/s390:

http://cr.openjdk.java.net/~simonis/webrevs/2016/s390x/8166800/
https://bugs.openjdk.java.net/browse/JDK-8166800

All this change does is to add some s390-specifc C and C++ flags for
the Hotspot and the class-library build which do not affect any of the
existing platforms.

Thank you and best regards,
Volker

Re: RFR(XXS): 8166801: [s390] Add jvm.cfg file for Linux/s390x

[Erik Joelsson]

Looks good to me.

/Erik


On 2016-10-05 16:43, Volker Simonis wrote:

Hi,

can somebody please review the following trivial change which simply
adds a new jvm.cfg file for Linux/s390x:

http://cr.openjdk.java.net/~simonis/webrevs/2016/s390x/8166801/
https://bugs.openjdk.java.net/browse/JDK-8166801

This is so far the only change we need in the jdk-repository for our
Linux/s390x port.

Thank you and best regards,
Volker

Re: RFR(XXS): 8166801: [s390] Add jvm.cfg file for Linux/s390x

[Aleksey Shipilev]

On 10/05/2016 04:43 PM, Volker Simonis wrote:
> can somebody please review the following trivial change which simply
> adds a new jvm.cfg file for Linux/s390x:
> 
> http://cr.openjdk.java.net/~simonis/webrevs/2016/s390x/8166801/
> https://bugs.openjdk.java.net/browse/JDK-8166801

This looks okay (and consistent with other arches) to me.

Thanks,
-Aleksey

Re: Deprecate all java.util.concurrent.*FieldUpdater

[David M. Lloyd]

I'm sure I recall an email from the past few months which proposed that 
*FieldUpdater are still going to be recommended in many cases over 
VarHandle because the latter is probably too low-level for casual uses. 
It was (IIRC) an argument in favor of more advanced fence methods or 
something like that.


Am I imagining it?

On 10/04/2016 04:19 PM, Martin Buchholz wrote:

VarHandle is a reasonable replacement for FieldUpdaters, but it's not yet
complete (where is accumulateAndGet?), and when do you deprecate something
when the replacement won't be ubiquitous for 10 years?

On Tue, Oct 4, 2016 at 1:32 PM, Remi Forax  wrote:


Given that Java 9 introduces a faster way to emit things like
compareAndSet by using the VarHandke API and that AtomiReference (and
likes) are now rewritten to use VarHandles directly,
i think it's time to deprecate all *FieldUpdater with something saying
that they have been superseded by the VarHandle API.

Rémi
substitute dr deprecator



--
- DML

RFR(XXS): 8166801: [s390] Add jvm.cfg file for Linux/s390x

[Volker Simonis]

Hi,

can somebody please review the following trivial change which simply
adds a new jvm.cfg file for Linux/s390x:

http://cr.openjdk.java.net/~simonis/webrevs/2016/s390x/8166801/
https://bugs.openjdk.java.net/browse/JDK-8166801

This is so far the only change we need in the jdk-repository for our
Linux/s390x port.

Thank you and best regards,
Volker

RFR(XS): 8166800: [s390] Top-level build changes required for Linux/s390x

[Volker Simonis]

Hi,

can I please have a review and sponsor for the following tiny
top-level build change required for building the OpenJDK on
Linux/s390:

http://cr.openjdk.java.net/~simonis/webrevs/2016/s390x/8166800/
https://bugs.openjdk.java.net/browse/JDK-8166800

All this change does is to add some s390-specifc C and C++ flags for
the Hotspot and the class-library build which do not affect any of the
existing platforms.

Thank you and best regards,
Volker

Re: Deprecate all java.util.concurrent.*FieldUpdater

[Remi Forax]

Hi Martin,

On October 4, 2016 11:19:33 PM GMT+02:00, Martin Buchholz  
wrote:
>VarHandle is a reasonable replacement for FieldUpdaters, but it's not
>yet
>complete (where is accumulateAndGet?), 

Seems to be a feature for me :)
I've never liked the methods that takes a lambda in FieldUpdater. If you're 
using a FieldUpdater, you are in the basement, you want a reliable performance 
model. The JLS does not guarantee that a side effect free lambda will be always 
inlined.

>and when do you deprecate
>something
>when the replacement won't be ubiquitous for 10 years?

The right answer is the one from Andrew but i can not resisrt, here is my 
answer:
when you hope that it will not take 10 years to be ubiquitous.

regards,
Remi

>
>On Tue, Oct 4, 2016 at 1:32 PM, Remi Forax  wrote:
>
>> Given that Java 9 introduces a faster way to emit things like
>> compareAndSet by using the VarHandke API and that AtomiReference (and
>> likes) are now rewritten to use VarHandles directly,
>> i think it's time to deprecate all *FieldUpdater with something
>saying
>> that they have been superseded by the VarHandle API.
>>
>> Rémi
>> substitute dr deprecator
>>

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: RFR: 8159855: Create an SPI for tools

[Alan Bateman]

On 05/10/2016 05:19, Mandy Chung wrote:


:

ToolProvider::findFirst(String name) can find tool providers on classpath.  I 
think it needs to wrap the for-loop (specifically iterating on providers) with 
doPrivileged due to the stack-based permission check.

Yes, that will be needed, otherwise the caller will need permission to 
get the system class loader and whatever other permissions are needed to 
locate and and load the tools.


The rest looks okay to me. Personally I would space out the javadoc tags 
more, and indent the second/third lines more than one space but that is 
just minor stuff.


-Alan

RFR:8055033: Shell tests for jrunscript don't pass through VM options

[Srinivas Dama]

Hi,

Please review http://cr.openjdk.java.net/~sdama/8055033/webrev.00/ 
for https://bugs.openjdk.java.net/browse/JDK-8055033 

When launching java and javac from shell script, ${TESTVMOPTS}, ${TESTJAVAOPTS} 
and ${TESTTOOLVMOPTS}, ${TESTJAVACOPTS} should be 
passed in respectively since jtreg sets these environment variables.

Regards,
Srinivas

Re: RFR: 8159855

[Alan Bateman]

On 05/10/2016 13:20, fo...@univ-mlv.fr wrote:


It seems to be a little to JDK centric to me.

It supports the common case, making the advanced cases possible. Also 
it's consistent with the javax.tools API. So overall then I think Jon 
has this right.


-Alan

Re: RFR: 8159855

[forax]

It seems to be a little to JDK centric to me.

Rémi

- Mail original -
> De: "Alan Bateman" 
> À: "Remi Forax" , "Jonathan Gibbons" 
> 
> Cc: compiler-...@openjdk.java.net, "OpenJDK Dev list" 
> 
> Envoyé: Mercredi 5 Octobre 2016 13:39:02
> Objet: Re: RFR: 8159855

> On 05/10/2016 12:10, Remi Forax wrote:
>> Very nice,
>> it will greatly help gradle, maven, etc.
>>
>> in com/sun/tools/javac/main/Main.java,
>> the other constructor should delegate its initialization to the constructor 
>> you
>> just add,
>>   public Main(String name, PrintWriter out) {
>>   this(name, out, out);
>>   }
>>
>> I wonder if findFirst() in ToolProvider should not take a ClassLoader as
>> parameter instead of being restricted to the system classloader.
>>
> The findFirst(tool) method just a convenience, the intention is that if
> you want to use other contexts then you just invoke ServiceLoader to
> locate it.
> 
> -Alan.

Re: RFR: 8159855

[Alan Bateman]

On 05/10/2016 12:10, Remi Forax wrote:

Very nice,
it will greatly help gradle, maven, etc.

in com/sun/tools/javac/main/Main.java,
the other constructor should delegate its initialization to the constructor you 
just add,
  public Main(String name, PrintWriter out) {
  this(name, out, out);
  }

I wonder if findFirst() in ToolProvider should not take a ClassLoader as 
parameter instead of being restricted to the system classloader.

The findFirst(tool) method just a convenience, the intention is that if 
you want to use other contexts then you just invoke ServiceLoader to 
locate it.


-Alan.

Re: Review Request JDK-8167014: jdeps: Missing message: warn.skipped.entry

[Lance Andersen]

Looks Ok Mandy
> On Oct 4, 2016, at 10:52 PM, Mandy Chung  wrote:
> 
> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8167014/webrev.00/index.html
> 
> This fixes the missing key in the resource bundle and also include the 
> exception message of the invalid entry.  I verified with a JAR file with bad 
> entries.
> 
> Mandy

 
  

 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: 8159855

[Remi Forax]

Very nice,
it will greatly help gradle, maven, etc.

in com/sun/tools/javac/main/Main.java,
the other constructor should delegate its initialization to the constructor you 
just add,
 public Main(String name, PrintWriter out) {
 this(name, out, out);
 }

I wonder if findFirst() in ToolProvider should not take a ClassLoader as 
parameter instead of being restricted to the system classloader.

regards,
Rémi

- Mail original -
> De: "Jonathan Gibbons" 
> À: "OpenJDK Dev list" 
> Cc: compiler-...@openjdk.java.net
> Envoyé: Mercredi 5 Octobre 2016 01:39:47
> Objet: RFR: 8159855

> Core-libs folk,
> 
> Please review the following change to add a new service provider class
> java.util.spi.ToolProvider
> 
> which can be used provide simple "command-line" access to select JDK
> tools, without starting a new JVM.
> 
> The following tools are updated to provide access through the new SPI:
> javac, javadoc, javap, jdeps
> 
> It is expected that additional tools will also be updated to provide access,
> but that will be done separately.
> 
> Compiler-dev folk may wish to review the changes to the langtools
> repository.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8159855
> Webrev: http://cr.openjdk.java.net/~jjg/8159855/webrev.03/
> API:
> http://cr.openjdk.java.net/~jjg/8159855/api.02/java/util/spi/ToolProvider.html
> 
> -- Jon

Re: RFR: 8159855: Create an SPI for tools

[Alan Bateman]

On 05/10/2016 10:54, Stephen Colebourne wrote:


Interesting.

How likely is it that there will be more than one tool of a given name
available? The method name findFirst() seems relatively odd for the
lookup operation.

I'd also note that the name string to pass in are "magic". There are
no constants defined for callers to use. Since there is no obvious way
in the API to find the vendor, this could get tricky across JDKs. Plus
how would a caller know what arguments to pass if each vendors tool
differs?

Just being cautious about the use case being solved.
Many command line tools don't define an API and so not unheard of to 
find code that uses sun.tools.jar.Main.main or the main method of other 
tools. Also common to see code using ProcessBuilder or Runtime.exec to 
launch tools like jar. If the commonly used tools (jar, jdeps, ...) 
document their tool names in their module description then it will make 
it easier to run these tools in the same VM. It does mean that some 
tools will have both an API and a documented tool name but that 
shouldn't be an issue. I'm sure Jon has a lot to say on this topic, esp. 
with linking usage documentation and man pages.


As regards constants then not clear where something like this would live 
as it amounts to a registry. Also many of the tools are JDK or library 
specific and so wouldn't have a place in the standard/Java SE docs.


The firstFirst(String) method limits itself to tools that are visible 
via the system class loader. Sure, someone might decide to deploy lots 
of libraries that all claim to be the "hammer" tool but this is no 
different to many of the service providers mechanisms. One could have 
the ToolProvider define methods with the tool capabilities that would 
aid selection but that would complicate the API.


-Alan

Re: RFR: 8167005: Comment on the need for an empty constructor in ArrayList$Itr

[Jonathan Bluett-Duncan]

Okay, that makes sense to me! Thank you for your explanations Claes and
Stuart.

Kind regards,
Jonathan

On 5 October 2016 at 01:57, Stuart Marks  wrote:

> Right, the main point of the comment is to tell the reader the constructor
> isn't superfluous, to prevent it from being cleaned up and accidentally
> causing a regression. Full history can reside in the commit comment, the
> bug database, and in these email logs.
>
> I'd put in a link to a bug only when there's some action on this code
> associated with that bug, e.g., "don't remove this code until bug XXX
> has been fixed."
>
> s'marks
>
>
> On 10/4/16 5:00 PM, Claes Redestad wrote:
>
>> Hi Jonathan,
>>
>> the aim isn't to add an in-depth explanation to the code about exactly
>> the circumstance that led to this constructor and comment being added,
>> but to put a clear message that it was simply, in fact, deliberate, so
>> even the proposed comment might be going further than strictly necessary.
>>
>> I'm also not convinced of the value of putting explicit links to the
>> bug actually pushed, since there's an implicit link in the commit
>> itself anyhow.
>>
>> Thanks!
>>
>> /Claes
>>
>> On 2016-10-04 23:20, Jonathan Bluett-Duncan wrote:
>>
>>> The explanation which Stuart gives for this change in
>>> https://bugs.openjdk.java.net/browse/JDK-8167005 seems to describe why
>>> this constructor is needed much better than the comment itself does. So
>>> I wonder if it's worth adding the link to the bug report in the comment.
>>> E.g.
>>>
>>> // prevent generation of synthetic class required for access to private
>>> // constructor. See: https://bugs.openjdk.java.net/browse/JDK-8167005
>>>
>>> Kind regards,
>>> Jonathan
>>>
>>> On 4 October 2016 at 21:28, Stuart Marks >> > wrote:
>>>
>>>
>>>
>>> On 10/4/16 3:55 AM, Claes Redestad wrote:
>>>
>>>
>>> On 2016-10-04 12:52, Aleksey Shipilev wrote:
>>>
>>> On 10/04/2016 12:50 PM, Claes Redestad wrote:
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~redestad/8167005/webrev.01/
>>> >> >
>>>
>>>
>>> OK.
>>>
>>> Thanks for the speedy review! :-)
>>>
>>>
>>> Thanks for looking at this. The shorter text in the bug report is ok
>>> too.
>>>
>>> s'marks
>>>
>>>
>>>

Re: RFR: 8159855

[Stephen Colebourne]

Interesting.

How likely is it that there will be more than one tool of a given name
available? The method name findFirst() seems relatively odd for the
lookup operation.

I'd also note that the name string to pass in are "magic". There are
no constants defined for callers to use. Since there is no obvious way
in the API to find the vendor, this could get tricky across JDKs. Plus
how would a caller know what arguments to pass if each vendors tool
differs?

Just being cautious about the use case being solved.

Stephen

On 5 October 2016 at 00:39, Jonathan Gibbons
 wrote:
> Core-libs folk,
>
> Please review the following change to add a new service provider class
> java.util.spi.ToolProvider
>
> which can be used provide simple "command-line" access to select JDK
> tools, without starting a new JVM.
>
> The following tools are updated to provide access through the new SPI:
> javac, javadoc, javap, jdeps
>
> It is expected that additional tools will also be updated to provide access,
> but that will be done separately.
>
> Compiler-dev folk may wish to review the changes to the langtools
> repository.
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8159855
> Webrev: http://cr.openjdk.java.net/~jjg/8159855/webrev.03/
> API:
> http://cr.openjdk.java.net/~jjg/8159855/api.02/java/util/spi/ToolProvider.html
>
> -- Jon

Re: [concurrency-interest] Deprecate all java.util.concurrent.*FieldUpdater

[Andrew Haley]

On 04/10/16 22:19, Martin Buchholz wrote:
> VarHandle is a reasonable replacement for FieldUpdaters, but it's not yet
> complete (where is accumulateAndGet?), and when do you deprecate something
> when the replacement won't be ubiquitous for 10 years?

Surely you have to start somewhere: deprecation is no more than saying
to programmers "Don't use this, use that."  And if you were leaning
over someone's shoulder that's what you would say.

Andrew.

Re: RFR: 8062389, 8029459, 8061950: Class.getMethod() is inconsistent with Class.getMethods() results + more

[Peter Levart]

Hi David,


On 10/05/2016 04:27 AM, David Holmes wrote:

Hi Peter,

Without making any comment on what you have actually done, does this 
account for private interface methods correctly?


In short, yes. Class.getMethods() and Class.getMethod() only ever return 
public methods. The source of Method objects is in both cases the 
following invocation:


privateGetDeclaredMethods(/* publicOnly */ true)

in lines 2981 and 3087, respectively.

Regards, Peter



Thanks,
David

On 4/10/2016 11:40 PM, Peter Levart wrote:

Hi,

I have a fix for conformance (P2) bug (8062389
) that also fixes a
conformance (P3) bug (8029459
) and a performance
issue (8061950 ):

http://cr.openjdk.java.net/~plevart/jdk9-dev/Class.getMethods.new/webrev.04/ 




As Daniel Smith writes in 8029459
, the following
situation is as expected:

interface I { void m(); }
interface J extends I { void m(); }
interface K extends J {}
K.class.getMethods() = [ J.m ]

But the following has a different result although it should probably
have the same:

interface I { void m(); }
interface J extends I { void m(); }
interface K extends I, J {}
K.class.getMethods() = [ I.m, J.m ]

He then suggests the following algorithm:

An implementation of getMethods consistent with JLS 8 would include the
following (see Lambda Spec, Part H, 9.4.1 and 8.4.8):
1) The class's/interface's declared (public) methods
2) The getMethods() of the superclass (if this is a class), minus any
that have a match in (1)
3) The getMethods() of each superinterface, minus any that have a match
in (1) or a _concrete_ match in (2) or a match from a more-specific
class/interface in (2) or (3)

But even that is not sufficient for the following situation:

interface E { void m(); }
interface F extends E { void m(); }
abstract class G implements E {}
abstract class H extends G implements F {}
H.class.getMethods() = [ E.m, F.m ]

The desired result of H.class.getMethods() = [ F.m ]

So a more precise definition is required which is implemented in the 
fix.


The getMethods() implementation partitions the union of the following
methods:

1) The class's/interface's declared public methods
2) The getMethods() of the superclass (if this is a class)
3) The non-static getMethods() of each direct superinterface

...into equivalence classes (sets) of methods with same signature
(return type, name, parameter types). Within each such set, only the
"most specific" methods are kept and others are discarded. The union of
the kept methods is the result.

The "more specific" is defined as a partial order within a set of
methods of same signature:

Method A is more specific than method B if:
- A is declared by a class and B is declared by an interface; or
- A is declared by the same type as or a subtype of B's declaring type
and both are either declared by classes or both by interfaces (clearly
if A and B are declared by the same type and have the same signature,
they are the same method)

If A and B are distinct elements from the set of methods with same
signature, then either:
- A is more specific than B; or
- B is more specific than A; or
- A and B are incomparable

A sub-set of "most specific" methods are the methods from the set where
for each such method M, there is no method N != M such that N is "more
specific" than M.

There can be more than one "most specific" method for a particular
signature as they can be inherited from multiple unrelated 
interfaces, but:

- javac prevents compilation when among multiply inherited methods with
same signature there is at least one default method, so in practice,
getMethods() will only return multiple methods with same signature if
they are abstract interface methods. With one exception: bridge methods
that are created by javac for co-variant overrides are default methods
in interfaces. For example:

interface I { Object m(); }
interface J1 extends I { Map m(); }
interface J2 extends I { HashMap m(); }
interface K extends J1, J2 {}

K.class.getMethods() = [ default Object j1.m, abstract Map j1.m, default
Object j2.m, abstract HashMap j2.m ]

But this is an extreme case where one can still expect multiple
non-abstract methods with same signature, but different declaring type,
returned from getMethods().

In order to also fix 8062389
, getMethod() and
getMethods() share the same consolidation logic that results in a set of
"most specific" methods. The partitioning in getMethods() is partially
performed by collecting Methods into a HashMap where the keys are (name,
parameter types) tuples and values are linked lists of Method objects
with possibly varying return and declaring types. The consolidation,
i.e. keeping only the set of most specific methods as new methods are
encountered, is performed 

RE: RFR: 8166875: (tz) Support tzdata2016g

[Ramanand Patil]

Hi Masayoshi,
Thank you for pointing that small miss. During testing phase, I had actually 
renamed "Asia/Rangoon" to "Asia/Yangon" (Asia/Rangoon entry was deleted) in 
TimeZoneNames*.java and this test case was failing along with other test cases, 
hence the bug ID tag was added there. But after correcting the 
TimeZoneNames*.java with correct changes I forgot to remove the bug ID from 
this bug.

Here is the updated Webrev: 
http://cr.openjdk.java.net/~rpatil/8166875/webrev.02/

Regards,
Ramanand.

-Original Message-
From: Masayoshi Okutsu 
Sent: Wednesday, October 05, 2016 8:49 AM
To: Ramanand Patil ; Martin Buchholz 

Cc: core-libs-dev ; i18n-...@openjdk.java.net
Subject: Re:  RFR: 8166875: (tz) Support tzdata2016g

Hi Ramanand,

I don't think it's appropriate to add the bug ID to 
test/sun/util/resources/cldr/Bug8134384.java. This test doesn't verify the 
TimeZoneNames*.java changes of this fix. Otherwise, the change looks OK to me.

Thanks,
Masayoshi


On 10/4/2016 7:22 PM, Ramanand Patil wrote:
> Hi Martin,
> Thank you for your review and explanation of "Yangon". I liked the 
> translation "End of Strife".
>
> Looking at the description of the ZoneNames.java:
> * The zid<->metazone mappings are based on CLDR metaZones.xml.
>   * The alias mappings are based on Link entries in tzdb data files.
>
> I had thought to not update this file because the CLDR metaZones.xml file 
> doesn’t have this entry updated.
> But I think you are correct, since Link entry has this alias mentioned, there 
> is no harm in adding these entries to zidMap and aliasMap arrays.
> Here is the updated Webrev: 
> http://cr.openjdk.java.net/~rpatil/8166875/webrev.01/
>
> Changes done:
>   - Updated src/java.base/share/classes/java/time/format/ZoneName.java to 
> include "Yangon" entry.
>   - Removed unused imports from 
> src/java.base/share/classes/java/time/format/ZoneName.java
>   - Updated ZoneName.java in the test package as well to include 
> "Yangon". [test/java/time/test/java/time/format/ZoneName.java]
>   - Updated the bugID for 
> test/java/time/test/java/time/format/TestZoneTextPrinterParser.java since 
> this uses the "ZoneName.java" defined in test package.
>
> Also, looks like ZoneName.java is trying to maintain a comprehensive list of 
> zone names. Though I found very few zone names are missing from this file 
> like: "Europe/Busingen", "America/Fort_Nelson", "Antarctica/Troll" etc...
>
>
> Regards,
> Ramanand.
>
> From: Martin Buchholz [mailto:marti...@google.com]
> Sent: Monday, October 03, 2016 8:55 PM
> To: Ramanand Patil 
> Cc: i18n-...@openjdk.java.net; core-libs-dev 
> 
> Subject: Re:  RFR: 8166875: (tz) Support tzdata2016g
>
> Hi Ramanand,
> Pleased to meet you!
>
> I expected to see Yangon added to ZoneName, because of the existing 
> reference to Rangoon
>
> java/time/test/java/time/format/ZoneName.java:179:"Asia/Rangoon", 
> "Myanmar", "Asia/Rangoon",
>
> Is ZoneName.java trying to maintain a comprehensive list of zone names?
>
> """Yangon (ရန်ကုန်) is a combination of the two words yan (ရန်) and koun 
> (ကုန်), which mean "enemies" and "run out of", respectively. It is also 
> translated as "End of Strife"."""
>
>
> On Mon, Oct 3, 2016 at 5:27 AM, Ramanand Patil 
>  wrote:
> HI all,
> Please review the latest TZDATA integration (tzdata2016g) to JDK9.
> Bug: https://bugs.openjdk.java.net/browse/JDK-8166875
> Webrev: http://cr.openjdk.java.net/~rpatil/8166875/webrev.00/
>
> All the TimeZone related tests are passed after integration.
> [BugID is updated for tests TimeZoneTest.java and Bug8134384.java, since they 
> verify the renamed TZID "Asia/Yangon"].
>
> Regards,
> Ramanand.