Re: RFR: 8159855: Create an SPI for tools

[Mandy Chung]

This SPI is useful and provides as a replacement to existing use of internal 
APIs to launch some of our tools.  We will get jar, jmod, jlink and possibly 
other tools to convert to this SPI.

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.

Otherwise, looks good.

Mandy

> On Oct 4, 2016, at 4:46 PM, Jonathan Gibbons  
> wrote:
> 
> Resend with non-mostly-empty subject line!
> 
> -- Jon
> 
> On 10/04/2016 04:39 PM, 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: RFR: 8159855: Create an SPI for tools

[Mandy Chung]

This SPI is useful and provides as a replacement to existing use of internal 
APIs to launch some of our tools.  We will get jar, jmod, jlink and possibly 
other tools to convert to this SPI.

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.

Otherwise, looks good.

Mandy

> On Oct 4, 2016, at 4:46 PM, Jonathan Gibbons  
> wrote:
> 
> Resend with non-mostly-empty subject line!
> 
> -- Jon
> 
> On 10/04/2016 04:39 PM, 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: RFR: 8166875: (tz) Support tzdata2016g

[Masayoshi Okutsu]

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.

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

[Mandy Chung]

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

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

[Mandy Chung]

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

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

[David Holmes]

Hi Peter,

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


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 only among methods in the list that share same
return type and also removes duplicates. getMethod() uses only one such
list, consolidates methods that match given name and parameter types and
returns the 1st method from the resulting list that has the most
specific return type.

That's it for algorithms used. As partitioning partially 

Re: RFR(M): 8166560: [s390] Basic enablement of s390 port.

[David Holmes]

On 5/10/2016 2:52 AM, Lindenmaier, Goetz wrote:

Hi David,

I fixed the change accordingly:
http://cr.openjdk.java.net/~goetz/wr16/8166560-basic_s390/hotspot.wr03/

But do you really want me to incorporate a bugfix for ARM in the port?


Sure - it is only an error message and we've probably never encountered 
it. All the other code has the case for Aarch64 first so this is no 
different - and no need to comment it as it isn't commented upon elsewhere.


Thanks,
David


Best regards,
  Goetz.


-Original Message-
From: David Holmes [mailto:david.hol...@oracle.com]
Sent: Dienstag, 4. Oktober 2016 09:56
To: Lindenmaier, Goetz ; Volker Simonis

Cc: hotspot-...@openjdk.java.net; core-libs-dev 
Subject: Re: RFR(M): 8166560: [s390] Basic enablement of s390 port.

Hi Goetz,

On 28/09/2016 8:26 PM, Lindenmaier, Goetz wrote:

Hi,

here a new webrev for this change:
http://cr.openjdk.java.net/~goetz/wr16/8166560-

basic_s390/hotspot.wr02/


I reverted the major reorderings in macros.hpp and os_linux.cpp.
David asked me to do so, and I guess it makes reviewing more simple.


Thanks. That said ...


Also this fixes the issue spotted by David which actually was wrong.
The other renaming of ARM to ARM32 was correct, though, as
AARCH64 is defined in both ARM 64-bit ports, and is checked before.
So the existing case checking ARM is only reached if !LP64.
I documented this ...


... We actually have a bug with the Elf32_* defines in os_linux.cpp

1769 #elif  (defined ARM) // ARM must come before AARCH64 because the
closed 64-bit port requires so.
1770   static  Elf32_Half running_arch_code=EM_ARM;

Aarch64 should come first otherwise we will set the wrong value. I've
been told it would only affect an error message but still ... can I ask
you to fix this please. Thanks.

---
src/share/vm/code/codeCache.cpp

I'd prefer to just see something like:

S390_ONLY(if (_heaps == NULL) return;) // May be true when NMT detail is
used

---

Otherwise seems okay.

Thanks,
David


Also I removed the change in mutex.hpp.  Maybe we contribute
the full change this was part of, but independent of the s390 port.

I withdraw the part of the change to jdk introducing jvm.cfg.  Volker wants

to

do a separate change for this.

Best regards,
  Goetz.




-Original Message-
From: Volker Simonis [mailto:volker.simo...@gmail.com]
Sent: Dienstag, 27. September 2016 19:58
To: David Holmes 
Cc: Lindenmaier, Goetz ; hotspot-
d...@openjdk.java.net; core-libs-dev 
Subject: Re: RFR(M): 8166560: [s390] Basic enablement of s390 port.

On Fri, Sep 23, 2016 at 8:11 AM, David Holmes



wrote:

Hi Goetz,

I see a change not related directly to S390 ie change from ARM to ARM32

in

src/os/linux/vm/os_linux.cpp



The change looks a little confusing because Goetz reordered the ifdef
cascades alphabetically (which I think is good).

Besides that, the only real change not related to s390 is indeed the
change from ARM to ARM32 which happend two times in the file.

@Goetz: have you done this intentionally?


It will be a while before I can go through this in any detail.

David


On 23/09/2016 3:52 PM, Lindenmaier, Goetz wrote:


Hi,

This change is part of the s390 port. It contains some basic adaptions
needed for a full hotspot port for linux s390x.

It defines the required macros, platform names and includes.

The s390 port calles CodeCache::contains() in current_frame(), which is
used in NMT. As NMT already collects stack traces before the

CodeCache

is

initialized, contains() needs a check for this.

 Wherever a row of platforms are listed, I sorted them alphabetically.

 The jdk requires the file jvm.cfg.

Please review. I please need a sponsor.
http://cr.openjdk.java.net/~goetz/wr16/8166560-

basic_s390/hotspot.wr01/

http://cr.openjdk.java.net/~goetz/wr16/8166560-

basic_s390/jdk.wr01/


Best regards,
  Goetz.

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

[Stuart Marks]

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: Proposal for adding O_DIRECT support into JDK 9

[Lu, Yingqi]

Hi Brain,

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

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.

Thanks,
Lucy


From: Brian Burkhalter [mailto:brian.burkhal...@oracle.com]
Sent: Tuesday, October 04, 2016 5:06 PM
To: Lu, Yingqi 
Cc: Langer, Christoph ; Alan Bateman 
; core-libs-dev@openjdk.java.net; 
nio-...@openjdk.java.net; Kharbas, Kishor ; 
Kaczmarek, Eric 
Subject: Re: Proposal for adding O_DIRECT support into JDK 9

Hi Lucy,

On Oct 4, 2016, at 10:24 AM, Lu, Yingqi 
> wrote:


Anyone else has any feedback/comments?

I looked over the 8164900-2 patch and did not see any conceptual problems per 
se. I did not however scrutinize all the detailed changes, especially the more 
extensive ones in FileDispatcherImpl.c. I did however run the patch through our 
regression tests for IO and NIO and encountered some problems. Firstly the 
patch breaks the build on OS X [1], Solaris [2], and Windows [3]. Note that the 
code under src/java.base/unix is built on all Unix systems including Linux, OS 
X, and Solaris. Secondly but of lesser note there were some warnings during the 
Linux build [4]. Otherwise the IO and NIO tests succeeded on Linux, but that 
only suggests that pre-existing functionality is preserved as those tests of 
course do not exercise the new code for which I suppose tests will be provided 
in the final version of the patch.

Regards,

Brian

[1] OS X

build/macosx-x64/support/gensrc/java.base/sun/nio/fs/UnixConstants.java:41: 
error: self-reference in initializer

static final int O_DIRECT = O_DIRECT;

^
[2] Solaris

build/solaris-x64/support/gensrc/java.base/sun/nio/fs/UnixConstants.java:41: 
error: self-reference in initializer

static final int O_DIRECT = O_DIRECT;

^
[3] Windows

jdk\src\java.base\windows\classes\java\io\FileDescriptor.java:76: error: 
 is not abstract and does not override 
abstract method getDirect(FileDescriptor) in JavaIOFileDescriptorAccess

new JavaIOFileDescriptorAccess() {

 ^
[4] Linux (Ubuntu 16.04 amd64 desktop)

jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c: In function 
'readDirect':
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c:104:5: warning: 
ignoring return value of 'posix_memalign', declared with attribute 
warn_unused_result [-Wunused-result]
 posix_memalign(, pageSize, newLen);
 ^
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c: In function 
'writeDirect':
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c:131:5: warning: 
ignoring return value of 'posix_memalign', declared with attribute 
warn_unused_result [-Wunused-result]
 posix_memalign(, pageSize, len);
 ^
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c: In function 
'Java_sun_nio_ch_FileDispatcherImpl_readvDirect0':
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c:250:5: warning: 
ignoring return value of 'posix_memalign', declared with attribute 
warn_unused_result [-Wunused-result]
 posix_memalign(, pageSize, newLen);
 ^
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c: In function 
'Java_sun_nio_ch_FileDispatcherImpl_writevDirect0':
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c:393:5: warning: 
ignoring return value of 'posix_memalign', declared with attribute 
warn_unused_result [-Wunused-result]
 posix_memalign(, pageSize, totalLen)

Re: Proposal for adding O_DIRECT support into JDK 9

[Brian Burkhalter]

Hi Lucy,

On Oct 4, 2016, at 10:24 AM, Lu, Yingqi  wrote:

> Anyone else has any feedback/comments?


I looked over the 8164900-2 patch and did not see any conceptual problems per 
se. I did not however scrutinize all the detailed changes, especially the more 
extensive ones in FileDispatcherImpl.c. I did however run the patch through our 
regression tests for IO and NIO and encountered some problems. Firstly the 
patch breaks the build on OS X [1], Solaris [2], and Windows [3]. Note that the 
code under src/java.base/unix is built on all Unix systems including Linux, OS 
X, and Solaris. Secondly but of lesser note there were some warnings during the 
Linux build [4]. Otherwise the IO and NIO tests succeeded on Linux, but that 
only suggests that pre-existing functionality is preserved as those tests of 
course do not exercise the new code for which I suppose tests will be provided 
in the final version of the patch.

Regards,

Brian

[1] OS X
build/macosx-x64/support/gensrc/java.base/sun/nio/fs/UnixConstants.java:41: 
error: self-reference in initializer
static final int O_DIRECT = O_DIRECT;
^
[2] Solaris
build/solaris-x64/support/gensrc/java.base/sun/nio/fs/UnixConstants.java:41: 
error: self-reference in initializer
static final int O_DIRECT = O_DIRECT;
^
[3] Windows
jdk\src\java.base\windows\classes\java\io\FileDescriptor.java:76: error: 
 is not abstract and does not override 
abstract method getDirect(FileDescriptor) in JavaIOFileDescriptorAccess
new JavaIOFileDescriptorAccess() {
 ^
[4] Linux (Ubuntu 16.04 amd64 desktop)

jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c: In function 
‘readDirect’:
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c:104:5: warning: 
ignoring return value of ‘posix_memalign’, declared with attribute 
warn_unused_result [-Wunused-result]
 posix_memalign(, pageSize, newLen);
 ^
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c: In function 
‘writeDirect’:
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c:131:5: warning: 
ignoring return value of ‘posix_memalign’, declared with attribute 
warn_unused_result [-Wunused-result]
 posix_memalign(, pageSize, len);
 ^
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c: In function 
‘Java_sun_nio_ch_FileDispatcherImpl_readvDirect0’:
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c:250:5: warning: 
ignoring return value of ‘posix_memalign’, declared with attribute 
warn_unused_result [-Wunused-result]
 posix_memalign(, pageSize, newLen);
 ^
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c: In function 
‘Java_sun_nio_ch_FileDispatcherImpl_writevDirect0’:
jdk/src/java.base/unix/native/libnio/ch/FileDispatcherImpl.c:393:5: warning: 
ignoring return value of ‘posix_memalign’, declared with attribute 
warn_unused_result [-Wunused-result]
 posix_memalign(, pageSize, totalLen)

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

[Claes Redestad]

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: Create an SPI for tools

[Jonathan Gibbons]

Resend with non-mostly-empty subject line!

-- Jon

On 10/04/2016 04:39 PM, 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

RFR: 8159855

[Jonathan Gibbons]

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

[Stephen Colebourne]

On 4 October 2016 at 23:27, Stuart Marks  wrote:
> 4) The 'resolverFields' problem/comment was regarding DateTimeFormatter (see
> this part of latest webrev), where I realised I couldn't use Set.of instead
> of Collections.unmodifiableSet(new HashSet<>(Arrays.asList(*))), because I
> noticed that one of the java.time tests was testing whether
> DateTimeFormatter.withResolverFields(TemporalField...) could accept null
> parameters, which made using Set.of impossible since it's null-hostile (as
> in it throws NullPointerException upon being constructed with null
> element(s)).
>
> Hm, yes, it's odd that there's a test for an array containing a null, in
> addition to an empty array and a null array. See:
>
> jdk/test/java/time/tck/java/time/format/TCKDateTimeFormatter.java
> test_resolverFields_listOfOneNull()
>
> I'm not entirely sure what's intended here. In any case, let's wait until we
> hear from Mr. Colebourne.

I don't think that is a sensible test. AFAICT, null in the set has no
meaning. Its not documented to have meaning, so it is really a bug
with a test.

> 5) For the changes in the Chronology classes, the era() method now returns
> an immutable array where it didn't before. (The List returned by
> Arrays.asList() can have individual elements modified but its size can't be
> changed.) The spec for eras() says "may be immutable" so presumably this is
> OK. But note, since the returned List is now immutable, a new instance
> needn't be created each time. I'm not sure whether it's worth keeping around
> a cached copy in case someone calls eras() again though.
>
> It would be good if we could hear from Stephen on this one as well.

eras() will rarely be used, so no point caching. Changing to List.of() is fine.

Stephen

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

[Stuart Marks]

On 9/30/16 6:57 AM, Jonathan Bluett-Duncan wrote:
1) It actually didn't occur to me that there was a potential TOCTOU problem in 
ModuleFinder.compose, despite the fact that I found a potential problem in 
FileTreeIterator - it completely missed me, so thank you for finding it! I'll 
see if I can put a similar comment there to what I wrote in FileTreeIterator.

OK. A simple comment such as

// make a copy since it's captured by the ModuleFinder instance belowfinal 
List finderList = List.of(finders);

should be sufficient.
2) ... I decided just now to do a search on Grepcode for usages of 
CookieManager.get 
 and 
CookieHandler.get 
, 
and curiously it looks like they're only used in sun classes in the JDK. So 
this change seems safe to me, unless I've missed something?
While we're looking at CookieManager.java, please fix this typo 
"NetscapeCookieSotre".


  84  * by a more sophisticated CookieStore implementation, e.g. a 
NetscapeCookieSotre.

So, grepcode is kind of hard to work with. I've found that when doing "Find 
Usages" for an API, the results are relative to the JDK version you're looking 
at. Apparently at the time grepcode's indexes were run, most of those other 
libraries weren't building against any version of JDK 8. But if you start the 
search at (say) JDK6 b27, and then go to java.net.CookieHandler.get() and do 
"Find usages", you get a bunch of hits:


http://grepcode.com/search/usages?type=method=repository.grepcode.com%24java%24root@jdk%24openjdk@6-b27@java%24net@CookieHandler@get%28java.net.URI%2Cjava.util.Map%29=u

Same with 6-b27 CookieManager.get():

http://grepcode.com/search/usages?type=method=repository.grepcode.com%24java%24root@jdk%24openjdk@6-b27@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29=u

7u40-b43 CookieHandler.get():

http://grepcode.com/search/usages?type=method=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24net@CookieHandler@get%28java.net.URI%2Cjava.util.Map%29=u

7u40-b43 CookieManager.get():

http://grepcode.com/search/usages?type=method=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24net@CookieManager@get%28java.net.URI%2Cjava.util.Map%29=u

I don't know a way to search for the usage of an API across all versions of the 
JDK. It's moderately painful to have to do the same searches for several 
different JDK versions.


Anyway, I spent some time looking through usage hits from the links above. It's 
hard to know how many there are; you just keep hitting "Next" until there aren't 
any more. (Is there a way to get statistics instead of just page after page of 
matches? I don't know.) There are lots of duplicates -- different versions of 
the same library -- so you can skip past those pretty quickly.


After a while some patterns begin to emerge. For this API, the Map that's 
returned is almost always iterated over looking for "Cookie" and "Cookie2" 
entries, or being probed directly for those entries, and is then thrown away. In 
one case it was stored in a private field of a non-serializable class, but the 
only usage of that field was to iterate over it in a manner similar to other cases.


I encourage you to look through these usages as well.

In any case, it's fairly clear to me that the most common use of the returned 
Map is to read stuff out of it immediately. This isn't definitive, obviously, 
but it does look like this is the most common usage pattern. Based on this I 
think the serialization difference won't be a problem, and that it's OK to 
proceed with this change.
3) In my local copy of jdk9, I've removed the TOCTOU comment in 
FileTreeIterator and changed List.of back to Arrays.asList, as your 
explanation regarding FileTreeWalker has convinced me that TOCTOU is not a 
real problem here

OK
4) The 'resolverFields' problem/comment was regarding DateTimeFormatter (see 
this part of latest webrev 
), 
where I realised I couldn't use Set.of instead of 
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(*))), because I 
noticed that one of the java.time tests was testing whether 
DateTimeFormatter.withResolverFields(TemporalField...) could accept null 
parameters, which made using Set.of impossible since it's null-hostile (as in 
it throws NullPointerException upon being constructed with null element(s)).
Hm, yes, it's odd that there's a test for an array containing a null, in 
addition to an empty array and a null array. See:


jdk/test/java/time/tck/java/time/format/TCKDateTimeFormatter.java

Re: RFR: 8166875: (tz) Support tzdata2016g

[Martin Buchholz]

Looks good to me!



On Tue, Oct 4, 2016 at 3:22 AM, 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  net>
> 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  ramanand.pa...@oracle.com> 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.
>

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

[Jonathan Bluett-Duncan]

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: Deprecate all java.util.concurrent.*FieldUpdater

[Martin Buchholz]

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
>

Deprecate all java.util.concurrent.*FieldUpdater

[Remi Forax]

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

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

[Stuart Marks]

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: Proposal for adding O_DIRECT support into JDK 9

[Lu, Yingqi]

Hi Christoph,

Thank you very much for trying our patch. 

We are still seeking the feedback from the community. When we get closer to the 
final version of the patch, we will modify the copyright years. Thank you for 
reminding us!

Anyone else has any feedback/comments?

Thanks,
Lucy

-Original Message-
From: Langer, Christoph [mailto:christoph.lan...@sap.com] 
Sent: Tuesday, October 04, 2016 1:33 AM
To: Lu, Yingqi ; Alan Bateman ; 
core-libs-dev@openjdk.java.net
Cc: nio-...@openjdk.java.net; Kaczmarek, Eric ; 
Kharbas, Kishor 
Subject: RE: Proposal for adding O_DIRECT support into JDK 9

Hi Lucy,

FWIW: I ran a build on AIX and this looks ok.

I also assume in your final version you'll update all copyright years where 
it's not 2016 yet? Other than that the changes look ok to me - but I'm neither 
a reviewer nor a deep expert in the area of your changes.

Best regards
Christoph

> -Original Message-
> From: nio-dev [mailto:nio-dev-boun...@openjdk.java.net] On Behalf Of 
> Lu, Yingqi
> Sent: Freitag, 30. September 2016 18:55
> To: Lu, Yingqi ; Alan Bateman 
> ; core-libs-dev@openjdk.java.net
> Cc: nio-...@openjdk.java.net; Kaczmarek, Eric 
> ; Kharbas, Kishor 
> Subject: RE: Proposal for adding O_DIRECT support into JDK 9
> 
> Hi All,
> 
> Please find the most recent version of the patch available at 
> http://cr.openjdk.java.net/~igraves/8164900-2/
> 
> In this version, we have following two changes:
> 
> 1. Move O_DIRECT flag from StandardOpenOption to ExtendedOpenOption 2. 
> Move the checks of O_DIRECT from native code to Java code.
> 
> Please let us know your feedback.
> 
> Thanks,
> Lucy
> 
> -Original Message-
> From: nio-dev [mailto:nio-dev-boun...@openjdk.java.net] On Behalf Of 
> Lu, Yingqi
> Sent: Tuesday, September 27, 2016 9:57 AM
> To: Alan Bateman ; core-libs- 
> d...@openjdk.java.net
> Cc: nio-...@openjdk.java.net
> Subject: RE: Proposal for adding O_DIRECT support into JDK 9
> 
> Alan,
> 
> Thank you for the explanation, we will modify the code accordingly and 
> send it out soon for review.
> 
> Thanks,
> Lucy
> 
> -Original Message-
> From: Alan Bateman [mailto:alan.bate...@oracle.com]
> Sent: Tuesday, September 27, 2016 8:45 AM
> To: Lu, Yingqi ; core-libs-dev@openjdk.java.net
> Cc: nio-...@openjdk.java.net
> Subject: Re: Proposal for adding O_DIRECT support into JDK 9
> 
> On 26/09/2016 19:50, Lu, Yingqi wrote:
> 
> > Alan, you mean readv0/write0 or read0/write0? I just want to make 
> > sure
> > :-)
> Apologies, I meant each of the native methods where the decision to 
> use direct I/O or not would be a lot more maintainable in Java. You'll 
> see that there are already code paths for direct vs. heap buffers.
> 
> 
> >
> > Anyone else has other opinions on where is the best home for O_DIRECT flag?
> The flags under jdk.unsupported will eventually be removed in the 
> future JDK release?
> >
> > If we agree ExtendedOpenOpen is the best home for O_DIRECT, we can
> modify that for sure.
> >
> I think ExtendedOpenOption is the right place. It's still TDB as to 
> whether to put these extensions but that should be transparent to 
> anyone using this when on the class path.
> 
> -Alan

RE: RFR(M): 8166560: [s390] Basic enablement of s390 port.

[Lindenmaier, Goetz]

Hi David,

I fixed the change accordingly:
http://cr.openjdk.java.net/~goetz/wr16/8166560-basic_s390/hotspot.wr03/

But do you really want me to incorporate a bugfix for ARM in the port?

Best regards,
  Goetz.

> -Original Message-
> From: David Holmes [mailto:david.hol...@oracle.com]
> Sent: Dienstag, 4. Oktober 2016 09:56
> To: Lindenmaier, Goetz ; Volker Simonis
> 
> Cc: hotspot-...@openjdk.java.net; core-libs-dev  d...@openjdk.java.net>
> Subject: Re: RFR(M): 8166560: [s390] Basic enablement of s390 port.
> 
> Hi Goetz,
> 
> On 28/09/2016 8:26 PM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > here a new webrev for this change:
> > http://cr.openjdk.java.net/~goetz/wr16/8166560-
> basic_s390/hotspot.wr02/
> >
> > I reverted the major reorderings in macros.hpp and os_linux.cpp.
> > David asked me to do so, and I guess it makes reviewing more simple.
> 
> Thanks. That said ...
> 
> > Also this fixes the issue spotted by David which actually was wrong.
> > The other renaming of ARM to ARM32 was correct, though, as
> > AARCH64 is defined in both ARM 64-bit ports, and is checked before.
> > So the existing case checking ARM is only reached if !LP64.
> > I documented this ...
> 
> ... We actually have a bug with the Elf32_* defines in os_linux.cpp
> 
> 1769 #elif  (defined ARM) // ARM must come before AARCH64 because the
> closed 64-bit port requires so.
> 1770   static  Elf32_Half running_arch_code=EM_ARM;
> 
> Aarch64 should come first otherwise we will set the wrong value. I've
> been told it would only affect an error message but still ... can I ask
> you to fix this please. Thanks.
> 
> ---
> src/share/vm/code/codeCache.cpp
> 
> I'd prefer to just see something like:
> 
> S390_ONLY(if (_heaps == NULL) return;) // May be true when NMT detail is
> used
> 
> ---
> 
> Otherwise seems okay.
> 
> Thanks,
> David
> 
> > Also I removed the change in mutex.hpp.  Maybe we contribute
> > the full change this was part of, but independent of the s390 port.
> >
> > I withdraw the part of the change to jdk introducing jvm.cfg.  Volker wants
> to
> > do a separate change for this.
> >
> > Best regards,
> >   Goetz.
> >
> >
> >
> >> -Original Message-
> >> From: Volker Simonis [mailto:volker.simo...@gmail.com]
> >> Sent: Dienstag, 27. September 2016 19:58
> >> To: David Holmes 
> >> Cc: Lindenmaier, Goetz ; hotspot-
> >> d...@openjdk.java.net; core-libs-dev 
> >> Subject: Re: RFR(M): 8166560: [s390] Basic enablement of s390 port.
> >>
> >> On Fri, Sep 23, 2016 at 8:11 AM, David Holmes
> 
> >> wrote:
> >>> Hi Goetz,
> >>>
> >>> I see a change not related directly to S390 ie change from ARM to ARM32
> in
> >>> src/os/linux/vm/os_linux.cpp
> >>>
> >>
> >> The change looks a little confusing because Goetz reordered the ifdef
> >> cascades alphabetically (which I think is good).
> >>
> >> Besides that, the only real change not related to s390 is indeed the
> >> change from ARM to ARM32 which happend two times in the file.
> >>
> >> @Goetz: have you done this intentionally?
> >>
> >>> It will be a while before I can go through this in any detail.
> >>>
> >>> David
> >>>
> >>>
> >>> On 23/09/2016 3:52 PM, Lindenmaier, Goetz wrote:
> 
>  Hi,
> 
>  This change is part of the s390 port. It contains some basic adaptions
>  needed for a full hotspot port for linux s390x.
> 
>  It defines the required macros, platform names and includes.
> 
>  The s390 port calles CodeCache::contains() in current_frame(), which is
>  used in NMT. As NMT already collects stack traces before the
> CodeCache
> >> is
>  initialized, contains() needs a check for this.
> 
>   Wherever a row of platforms are listed, I sorted them alphabetically.
> 
>   The jdk requires the file jvm.cfg.
> 
>  Please review. I please need a sponsor.
>  http://cr.openjdk.java.net/~goetz/wr16/8166560-
> >> basic_s390/hotspot.wr01/
>  http://cr.openjdk.java.net/~goetz/wr16/8166560-
> basic_s390/jdk.wr01/
> 
>  Best regards,
>    Goetz.
> 
> >>>

RE: RFR: JEP draft for Linux/s3990x port

[Lindenmaier, Goetz]

Hi Vladimir, 

This webrev contains all the changes to hotspot needed for the port:
http://cr.openjdk.java.net/~goetz/wr16/8166730-linuxs390-all/hotspot.wr01/

It includes 
http://cr.openjdk.java.net/~goetz/wr16/8166560-basic_s390/hotspot.wr03/
http://cr.openjdk.java.net/~goetz/wr16/8166561-basic_C1C2_s390/webrev.01/
http://cr.openjdk.java.net/~goetz/wr16/8166562-scratch_emit/webrev.01/
which are out for review. Further it includes
the one change to relocate the pc-relative instructions where we didn't open
a bug for yet, and the new s390-files.

Altogether this passed all our tests that were running on the weekend
on linuxs390.

The s390-files though are not yet fully in shape, I'm still editing them to get
rid of legacy stuff and SAP JVM specific code.  E.g. all the code guarded by 
#ifdef SAPJVM  will go away in the end.

I hope to have the final versions by end of this week.

Best regards,
  Goetz.


> -Original Message-
> From: s390x-port-dev [mailto:s390x-port-dev-boun...@openjdk.java.net]
> On Behalf Of Vladimir Kozlov
> Sent: Montag, 3. Oktober 2016 23:50
> To: Volker Simonis 
> Cc: s390x-port-...@openjdk.java.net; porters-...@openjdk.java.net;
> build-dev ; HotSpot Open Source Developers
> ; Java Core Libs  d...@openjdk.java.net>
> Subject: Re: RFR: JEP draft for Linux/s3990x port
> 
> Hi Volker,
> 
> Can you prepare combined patch (or set of patches) based on latest
> reviews together with s390 code as it will be in final push?
> 
> We want to run it through our pre-integration testing to verify that it
> does not have problems.
> 
> Thanks,
> Vladimir
> 
> On 9/29/16 11:25 AM, Vladimir Kozlov wrote:
> > You need to wait when Mark (OpenJDK Lead) move it to Candidate (or
> > other) state:
> >
> > http://cr.openjdk.java.net/~mr/jep/jep-2.0-02.html
> >
> > Vladimir
> >
> > On 9/29/16 9:55 AM, Volker Simonis wrote:
> >> Hi Vladimir,
> >>
> >> thanks a lot for reviewing and endorsing the JEP.
> >>
> >> I've linked all the relevant issues to the JEP  (they all have a link
> >> to a webrev) and change the state to "Submitted".
> >>
> >> There's just one more small shared change we need for the port for
> >> which we haven't opened a bug now because we are still working on
> >> simplifying it. The current version looks as follows:
> >>
> >> http://cr.openjdk.java.net/~simonis/webrevs/2016/s390x/916-
> constant_table_offset.patch
> >>
> >>
> >> What are the next steps? Should I add a "jdk9-fc-request" label to t
> >> he JEP and add a corresponding "FC Extension Request" comment to it?
> >> Or will this be done automatically once I move it to "Candidate"?
> >>
> >> Is there anything left to do before I can move it to "Candidate" state?
> >>
> >> Thanks a lot and best regards,
> >> Volker
> >>
> >>
> >>
> >>
> >> On Tue, Sep 27, 2016 at 8:15 PM, Vladimir Kozlov
> >>  wrote:
> >>> On 9/27/16 10:49 AM, Volker Simonis wrote:
> 
>  Hi,
> 
>  can you please review and endorse the following draft JEP for the
>  integration of the Linux/s390x port into the jkd9 master repository:
> 
>  https://bugs.openjdk.java.net/browse/JDK-8166730
> >>>
> >>>
> >>> Good.
> >>> Add links to webrevs in a comment. It will help to get umbrella FC
> >>> extension
> >>> approval.
> >>>
> 
>  As detailed in the JEP, the Linux/s390x requires very few shared
>  changes and we therefore don't foresee any impact on the existing
>  platforms at all. Following you can find a short description of the
>  planned changes:
> 
>  hotspot:
>  ===
> 
>  Out for review:
>  8166560: [s390] Basic enablement of s390 port.
>  http://cr.openjdk.java.net/~goetz/wr16/8166560-
> basic_s390/hotspot.wr01/
> 
>  Reviewed:
>  8166562: C2: Suppress relocations in scratch emit.
>  http://cr.openjdk.java.net/~goetz/wr16/8166562-
> scratch_emit/webrev.01/
> 
>  Will send RFR soon (depends on 8166560):
>  8166561: [s390] Adaptions needed for s390 port in C1 and C2.
>  http://cr.openjdk.java.net/~goetz/wr16/8166562-
> scratch_emit/webrev.01
> >>>
> >>>
> >>> Wrong link.
> >>>
> >>> Thanks,
> >>> Vladimir
> >>>
> >>>
> 
>  We are still investigating the need of these shared changes:
> 
> 
> http://cr.openjdk.java.net/~goetz/wr16/s390x_patch_queue/hotspot/9000
> 011-pass_PC_to_retAddrOffset.patch
> 
> 
> 
> http://cr.openjdk.java.net/~goetz/wr16/s390x_patch_queue/hotspot/9000
> 016-constant_table_offset.patch
> 
> 
>  And finally the patch with the s390x-only platform files. We are still
>  editing these to get them into OpenJdk style and shape.
>  Hotspot passes most jck, jtreg and spec tests with these.
> 
> 
> http://cr.openjdk.java.net/~goetz/wr16/s390x_patch_queue/hotspot/9000
> 101-zFiles.patch
> 
> 
>  top-level repository:
>  ===
> 
>  

Re: [9] RFR: 8166645: Include locales plugin throws InternalError with "*" specified.

[Mandy Chung]

> On Oct 4, 2016, at 5:42 AM, Naoto Sato  wrote:
> 
> Thanks, Mandy. I updated it as you suggested.
> 
> http://cr.openjdk.java.net/~naoto/8166645/webrev.02/

+1

Mandy

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

[Chris Hegarty]

> On 4 Oct 2016, at 11:55, 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/

+1

-Chris.

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

[Peter Levart]

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 only among methods in the list that share same 
return type and also removes duplicates. getMethod() uses only one such 
list, consolidates methods that match given name and parameter types and 
returns the 1st method from the resulting list that has the most 
specific return type.


That's it for algorithms used. As partitioning partially happens using 
HashMap with (name, parameter types) keys, lists of methods that form 
values are typically kept short with most of them 

Re: [9] RFR: 8166645: Include locales plugin throws InternalError with "*" specified.

[Naoto Sato]

Thanks, Mandy. I updated it as you suggested.

http://cr.openjdk.java.net/~naoto/8166645/webrev.02/

Naoto

On 10/3/16 7:53 PM, Mandy Chung wrote:



On Oct 3, 2016, at 3:17 PM, Naoto Sato  wrote:

According to an internal review, I added some explanation to the modification. 
No logic modifications.

http://cr.openjdk.java.net/~naoto/8166645/webrev.01/


This looks okay.

I suggest to keep Set originals after splitting the input byte array 
such that
  325 .filter(t -> original.indexOf(t) != -1)

can be replaced with .filter(originals::contains)

Mandy

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

[Claes Redestad]

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! :-)

/Claes

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

[Aleksey Shipilev]

On 10/04/2016 12:50 PM, Claes Redestad wrote:
> Webrev: http://cr.openjdk.java.net/~redestad/8167005/webrev.01/

OK.

-Aleksey

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

[Claes Redestad]

Hi,

as suggested by Stuart Marks in the review thread for JDK-8166840, we
should add a comment as to why an empty constructor is important in the
private inner Itr class, to avoid it being accidentally "cleaned up".

Webrev: http://cr.openjdk.java.net/~redestad/8167005/webrev.01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8167005

Thanks!

/Claes

RE: RFR: 8166875: (tz) Support tzdata2016g

[Ramanand Patil]

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.

RE: Proposal for adding O_DIRECT support into JDK 9

[Langer, Christoph]

Hi Lucy,

FWIW: I ran a build on AIX and this looks ok.

I also assume in your final version you'll update all copyright years where 
it's not 2016 yet? Other than that the changes look ok to me - but I'm neither 
a reviewer nor a deep expert in the area of your changes.

Best regards
Christoph

> -Original Message-
> From: nio-dev [mailto:nio-dev-boun...@openjdk.java.net] On Behalf Of Lu,
> Yingqi
> Sent: Freitag, 30. September 2016 18:55
> To: Lu, Yingqi ; Alan Bateman
> ; core-libs-dev@openjdk.java.net
> Cc: nio-...@openjdk.java.net; Kaczmarek, Eric ;
> Kharbas, Kishor 
> Subject: RE: Proposal for adding O_DIRECT support into JDK 9
> 
> Hi All,
> 
> Please find the most recent version of the patch available at
> http://cr.openjdk.java.net/~igraves/8164900-2/
> 
> In this version, we have following two changes:
> 
> 1. Move O_DIRECT flag from StandardOpenOption to ExtendedOpenOption
> 2. Move the checks of O_DIRECT from native code to Java code.
> 
> Please let us know your feedback.
> 
> Thanks,
> Lucy
> 
> -Original Message-
> From: nio-dev [mailto:nio-dev-boun...@openjdk.java.net] On Behalf Of Lu,
> Yingqi
> Sent: Tuesday, September 27, 2016 9:57 AM
> To: Alan Bateman ; core-libs-
> d...@openjdk.java.net
> Cc: nio-...@openjdk.java.net
> Subject: RE: Proposal for adding O_DIRECT support into JDK 9
> 
> Alan,
> 
> Thank you for the explanation, we will modify the code accordingly and send it
> out soon for review.
> 
> Thanks,
> Lucy
> 
> -Original Message-
> From: Alan Bateman [mailto:alan.bate...@oracle.com]
> Sent: Tuesday, September 27, 2016 8:45 AM
> To: Lu, Yingqi ; core-libs-dev@openjdk.java.net
> Cc: nio-...@openjdk.java.net
> Subject: Re: Proposal for adding O_DIRECT support into JDK 9
> 
> On 26/09/2016 19:50, Lu, Yingqi wrote:
> 
> > Alan, you mean readv0/write0 or read0/write0? I just want to make sure
> > :-)
> Apologies, I meant each of the native methods where the decision to use direct
> I/O or not would be a lot more maintainable in Java. You'll see that there are
> already code paths for direct vs. heap buffers.
> 
> 
> >
> > Anyone else has other opinions on where is the best home for O_DIRECT flag?
> The flags under jdk.unsupported will eventually be removed in the future JDK
> release?
> >
> > If we agree ExtendedOpenOpen is the best home for O_DIRECT, we can
> modify that for sure.
> >
> I think ExtendedOpenOption is the right place. It's still TDB as to whether 
> to put
> these extensions but that should be transparent to anyone using this when on
> the class path.
> 
> -Alan

Re: RFR(M): 8166560: [s390] Basic enablement of s390 port.

[David Holmes]

Hi Goetz,

On 28/09/2016 8:26 PM, Lindenmaier, Goetz wrote:

Hi,

here a new webrev for this change:
http://cr.openjdk.java.net/~goetz/wr16/8166560-basic_s390/hotspot.wr02/

I reverted the major reorderings in macros.hpp and os_linux.cpp.
David asked me to do so, and I guess it makes reviewing more simple.


Thanks. That said ...


Also this fixes the issue spotted by David which actually was wrong.
The other renaming of ARM to ARM32 was correct, though, as
AARCH64 is defined in both ARM 64-bit ports, and is checked before.
So the existing case checking ARM is only reached if !LP64.
I documented this ...


... We actually have a bug with the Elf32_* defines in os_linux.cpp

1769 #elif  (defined ARM) // ARM must come before AARCH64 because the 
closed 64-bit port requires so.

1770   static  Elf32_Half running_arch_code=EM_ARM;

Aarch64 should come first otherwise we will set the wrong value. I've 
been told it would only affect an error message but still ... can I ask 
you to fix this please. Thanks.


---
src/share/vm/code/codeCache.cpp

I'd prefer to just see something like:

S390_ONLY(if (_heaps == NULL) return;) // May be true when NMT detail is 
used


---

Otherwise seems okay.

Thanks,
David


Also I removed the change in mutex.hpp.  Maybe we contribute
the full change this was part of, but independent of the s390 port.

I withdraw the part of the change to jdk introducing jvm.cfg.  Volker wants to
do a separate change for this.

Best regards,
  Goetz.




-Original Message-
From: Volker Simonis [mailto:volker.simo...@gmail.com]
Sent: Dienstag, 27. September 2016 19:58
To: David Holmes 
Cc: Lindenmaier, Goetz ; hotspot-
d...@openjdk.java.net; core-libs-dev 
Subject: Re: RFR(M): 8166560: [s390] Basic enablement of s390 port.

On Fri, Sep 23, 2016 at 8:11 AM, David Holmes 
wrote:

Hi Goetz,

I see a change not related directly to S390 ie change from ARM to ARM32 in
src/os/linux/vm/os_linux.cpp



The change looks a little confusing because Goetz reordered the ifdef
cascades alphabetically (which I think is good).

Besides that, the only real change not related to s390 is indeed the
change from ARM to ARM32 which happend two times in the file.

@Goetz: have you done this intentionally?


It will be a while before I can go through this in any detail.

David


On 23/09/2016 3:52 PM, Lindenmaier, Goetz wrote:


Hi,

This change is part of the s390 port. It contains some basic adaptions
needed for a full hotspot port for linux s390x.

It defines the required macros, platform names and includes.

The s390 port calles CodeCache::contains() in current_frame(), which is
used in NMT. As NMT already collects stack traces before the CodeCache

is

initialized, contains() needs a check for this.

 Wherever a row of platforms are listed, I sorted them alphabetically.

 The jdk requires the file jvm.cfg.

Please review. I please need a sponsor.
http://cr.openjdk.java.net/~goetz/wr16/8166560-

basic_s390/hotspot.wr01/

http://cr.openjdk.java.net/~goetz/wr16/8166560-basic_s390/jdk.wr01/

Best regards,
  Goetz.

Re: RFR: 6378384 (reflect) subclass can’t access superclass’s protected fields and methods by reflection

[Peter Levart]

To a potential reviewer...

The change might look scary at first as it touches reflective access 
controls and by that, security of the platform, but:


- it is just (32 ins; 56 del; 62 mod) lines. The rest is a new jtreg 
test which is beneficial by itself as such test did not exist until now.
- the jtreg test is exhaustive and proves that the patch does not have 
undesired effects.


Thanks,

Peter Levart

On 10/02/2016 11:51 PM, Peter Levart wrote:

Hi,

I added an exhaustive jtreg test that covers all possible situations.

From the set of the following classes:

package a;
public class PublicSuper {...}

package a;
class Package {...}

package b;
public class PublicSub extends a.PublicSuper {...}

package b;
class Package {...}

it creates a set of all possible triplets:

(currentClass, memberClass, targetClass)

where:

currentClass - the class making the reflective access
memberClass - the member's declaring class
targetClass - the target object's class (for accessing instance fields 
and methods - must be equal to or subclass of memberClass)


For each such triplet it checks the reflective access to each of the 
following members:


{private, package, protected, public} x {instance, static} x {field, 
method}


and:

 {private, package, protected, public} x {constructor}

When running on unpatched build 9-ea+137, the result is:

http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/AccessControlTest_unpatched.jtr

When running on patched build of 9, the result is:

http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/AccessControlTest_patched.jtr


The difference is exactly in the following two cases which fail for 
unpatched and are fixed by the patch:


b.PublicSub accessing a.PublicSuper's PROTECTED_STATIC_FIELD - 
expected allowed, actual denied : FAILURE
b.PublicSub accessing a.PublicSuper's PROTECTED_STATIC_METHOD - 
expected allowed, actual denied : FAILURE


QED.


This is new webrev:

http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/webrev.02/

I think the test proves the effect of the patch is as intended, 
therefore it should not be a problem to review it.



Regards, Peter


On 10/01/2016 12:20 AM, Peter Levart wrote:

Hi,

I have a fix for a 10 year old bug (JDK-6378384 
). It was marked as 
a duplicate of a 19 year old bug (JDK-4032740 
) which is marked 
as a duplicate of a 17 year old bug (JDK-4283544 
) which is still 
open. But this bug is not a strict duplicate. This bug only concerns 
reflective access to protected members.


Here's a proposed fix:

http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/webrev.01/


The bug manifests itself as not being able to access protected static 
methods or fields from a subclass located in a different package. 
Instance protected methods and fields can be accessed, and using an 
undocumented trick, also static methods and fields, but the trick is 
very subtle. The specification for Field.get/set and Method.invoke 
says, respectively:


 * If the underlying field is a static field, the {@code obj} 
argument

 * is ignored; it may be null.

and:

 * If the underlying method is static, then the specified 
{@code obj}

 * argument is ignored. It may be null.

Well, it is not exactly so! The obj argument is used as a 'target' 
even for protected static members and it is ensured that its class is 
equal or a subclass of the class that accesses the member. So if you 
pass an instance of a subclass of the protected method's declaring 
class into the get/set/invoke, you can access the static protected 
member. If you pass 'null', you get IllegalAccessException.


The problem is in the design of 
jdk.internal.reflect.Reflection#ensureMemberAccess method which is 
used to check reflective access. It takes an Object 'target' 
argument, which is supposed to be null when accessing static 
methods/fields and it is null also when accessing constructors. 
Because of constructors and the method's API, it has to be overly 
restrictive as it must only allow calling protected constructors from 
within the constructor's declaring class itself or same package, 
while protected static methods could be called from any subclass.


By redesigning the API of this method, replacing Object 'target' 
parameter with Class 'targetClass' parameter and by passing the 
constructor's declaring class into this method instead of null, 
reflective checks suddenly start to look more like JLS dictates (but 
still a long way from it, unfortunately).


As a bonus, sun.reflect.misc.ReflectUtil#ensureMemberAccess method 
(used from AtomicXXXFieldUpdater classes) does not need the following 
comment any more:


 * Reflection.ensureMemberAccess is overly-restrictive
 * due to a bug. We awkwardly work around it for now.