RE: RFR: 8189102: All tools should support -?, -h and --help

[Lindenmaier, Goetz]

Hi Kumar, 

Thanks for looking at my change!
 
> I looked at some of the components I maintain, and they look good.
May I ask which these are? So I can account whether all parts have been 
reviewed?

> 1. The local variables/fields don't comply with the coding conventions,
> we have been
>   following in the JDK.
Removed all '_' from fields.  Anything else?  TOOLS_NOT_TO_TEST
is formatted as the similar list in VersionCheck.java.

> 2.  Hmm, someone parked a .ini file in bin directory, later removed,
>   perhaps on windows simply check for .exe ? Should a check exist
>   to verify if file has executable permissions ?"File.canExecute".
> 
>   171 // Returns true if the file is not a tool.
>   172 static boolean notATool(String file) {
>   173 file = file.toLowerCase();
>   174 if (file.endsWith(".dll") ||
>   175 file.endsWith(".map") ||
>   176 file.endsWith(".pdb") ||
>   177 file.equals("server")) {  // server subdir on windows.
>   178 return true;
>   179 }
>   180 return false;
>   181 } 
I anyways wondered why .dll + friends are in the exe directory and 
not in the lib directory.  But a check for executable file is better
than this list. Changed.

> 3.  Typo in comment
Fixed. 

> 4. There is a method to check for isEnglishLocale in TestHelper, perhaps
> use it to have the test bale out in non english locales as early as possible ?
I added a bailout at the beginning of the test. Thanks for the advice!
But thinking about this a @requires englishLocale would be useful here.
 
> 5.  Has this been tested on all platforms ? do you need help testing it ?
Our testing is on ntamd64, linuxppc64, linuxppc64le, linuxs390x, linuxx86_64,
sun_64 and aixppc64. I run the jdk jtreg tests on these platforms. I once ran 
the change with the jdk-hs repo, where we test test/hotspot/jtreg and most jck 
tests.  All passed.
I don't think this is very platform dependent (most differences are
between unix/windows) so this platform coverage should suffice 
I think.

I'll post a new webrev later on.

Best regards,
  Goetz.


> 
> Kumar
> 
> 
> On 11/17/2017 3:23 AM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > please review this change. I also filed a CSR for this:
> > http://cr.openjdk.java.net/~goetz/wr17/8189102-
> helpMessage/webrev.02/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8189102
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8191477
> >
> > See the webrev for a detailed description of the changes.
> >
> > If required, I'll make break-out changes to be reviewed separately.
> >
> > I had posted a RFR before, but improved the change to
> > give a more complete overview of currently supported flags
> > for the CSR:
> > http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-
> October/028615.html
> >
> > Best regards,
> >Goetz.
> >

RE: RFR: 8189102: All tools should support -?, -h and --help

[Lindenmaier, Goetz]

Hi,

I uploaded a new webrev:
http://cr.openjdk.java.net/~goetz/wr17/8189102-helpMessage/webrev.04/

This includes the changes 
  - to jshell requested by Robert
  - to the test as requested by Kumar.

See also incremental patch and the test output including all the
help messages referenced in the webrev.

Best regards,
  Goetz.

> -Original Message-
> From: Kumar Srinivasan [mailto:kumar.x.sriniva...@oracle.com]
> Sent: Montag, 27. November 2017 17:43
> To: Lindenmaier, Goetz 
> Cc: core-libs-dev@openjdk.java.net; 'compiler-...@openjdk.java.net'
> ; serviceability-dev (serviceability-
> d...@openjdk.java.net) 
> Subject: Re: RFR: 8189102: All tools should support -?, -h and --help
> 
> Hi,
> 
> I looked at some of the components I maintain, and they look good.
> 
> Wrt. the test:
> 
> 1. The local variables/fields don't comply with the coding conventions,
> we have been
>   following in the JDK.
> 
> 2.  Hmm, someone parked a .ini file in bin directory, later removed,
>   perhaps on windows simply check for .exe ? Should a check exist
>   to verify if file has executable permissions ?"File.canExecute".
> 
>   171 // Returns true if the file is not a tool.
>   172 static boolean notATool(String file) {
>   173 file = file.toLowerCase();
>   174 if (file.endsWith(".dll") ||
>   175 file.endsWith(".map") ||
>   176 file.endsWith(".pdb") ||
>   177 file.equals("server")) {  // server subdir on windows.
>   178 return true;
>   179 }
>   180 return false;
>   181 }
> 
> 
> 3.  Typo in comment
> 
> 201 // Check whether 'flag' appears in 'line' as a word of itself. I must 
> not
> 
> s/I must/It must/
> 
> 
> 4. There is a method to check for isEnglishLocale in TestHelper, perhaps
> use it
> to have the test bale out in non english locales as early as possible ?
> 
> 5.  Has this been tested on all platforms ? do you need help testing it ?
> 
> Kumar
> 
> 
> On 11/17/2017 3:23 AM, Lindenmaier, Goetz wrote:
> > Hi,
> >
> > please review this change. I also filed a CSR for this:
> > http://cr.openjdk.java.net/~goetz/wr17/8189102-
> helpMessage/webrev.02/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8189102
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8191477
> >
> > See the webrev for a detailed description of the changes.
> >
> > If required, I'll make break-out changes to be reviewed separately.
> >
> > I had posted a RFR before, but improved the change to
> > give a more complete overview of currently supported flags
> > for the CSR:
> > http://mail.openjdk.java.net/pipermail/hotspot-dev/2017-
> October/028615.html
> >
> > Best regards,
> >Goetz.
> >

Re: Review Request: JDK-8191942: Replace jdeps use of jdk.internal.util.jar.VersionedStream with new public API

[Alan Bateman]

On 28/11/2017 01:50, mandy chung wrote:
This is a follow-up on JDK-8189611 that defines a new public API to 
return a stream of versioned entries and to return the real name of a 
JAR entry.  JDK-8189611 leaves jdeps untouched because jdeps is 
compiled with the boot JDK.


This patch includes:
(1) changes the build not to build jdk.jdeps interim module but 
include com.sun.tools.classfile classes in the CreateSymbol buildtool
(2) replace the use of internal API with the public APIs defined by 
JDK-8191942

(3) remove jdk.internal.util.jar.VersonedStream class

Webrev:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8191942/webrev.00/

Looks good.

-Alan

Re: RFR (S): JDK-8191328: Avoid unnecessary overhead in CRC32C

[Dmitry Chuyko]

Initial version of the patch made worse C1 code because of additionally 
introduced locals, this may be important for client (arm32). I fixed 
this by just coupling xors with brackets. Also I made measurements with 
Graal and AOT. Note, in case of tiered with AOT compiled java.base the 
intrinsic is used if present.


Updated webrev: http://cr.openjdk.java.net/~dchuyko/8191328/webrev.01/
Updated benchmark: 
http://cr.openjdk.java.net/~dchuyko/8191328/webrev.01/CRC32CAltBench.java


Results on my x86 laptop and JDK 10:

Tiered
before  375 ± 6  ns/op
after   334 ± 3  ns/op 11%

Tiered with Graal (JVMCI)
before  356 ± 7  ns/op
after   327 ± 6  ns/op 8%

Tiered with AOT compiled benchmark (non-tiered)
before  1308 ± 58  ns/op
after   1010 ±  8  ns/op 1.3x

Tiered with -XX:MaxInlineLevel=0
before  660 ± 4  ns/op
after   338 ± 3  ns/op 1.9x

C1
before  498 ± 4  ns/op
after   495 ± 4  ns/op same

Interpreter
before  40844 ± 333  ns/op
after   24777 ± 624  ns/op 1.7x

-Dmitry


On 11/16/2017 07:42 PM, Dmitry Chuyko wrote:

On 11/15/2017 09:44 PM, Andrew Haley wrote:

On 15/11/17 18:38, Vitaly Davidovich wrote:

On Wed, Nov 15, 2017 at 12:40 PM, Andrew Haley  wrote:

On 15/11/17 15:38, Alan Bateman wrote:
Moving the nativeOrder out of the loop make sense but I'm curious 
about

the context for improving this implementation.

I wonder about lifting ByteOrder.nativeOrder().  Maybe it fails to
inline because the method is too large: if that happens, we really
lose.  I'm not seeing that, though: it seems to be inlined just fine,
and has no effect.
Sure, it is the effect of missing inlining. But you can relatively 
easily break it by your tiered JIT settings. Not sure about AOT. Like 
(in Hotspot):
-XX:-Inline, -XX:MaxInlineLevel=0 (no wonder to meet this one in 
wild), -XX:FreqInlineSize=3, -XX:InlineSmallCode=15..


In any case, this patch doesn't help anything on my test hardware.

Is this with -Xcomp though? That can generate crap code because
there's no profiling information.  Not that -Xcomp should be the way
to test peak performance IMO, but that is the setting that was used I
believe.
Another noticeable case is -Xint where absolute times of CRC 
calculation are quite long.


Here is a benchmark that is easier to experiment with (no need to 
build jdk or to turn off intrinsics):


http://cr.openjdk.java.net/~dchuyko/8191328/CRC32CAltBench.java

Some x86 results:

default tiered
before  380.957 ± 11.621  ns/op
after   350.838 ±  5.149  ns/op

-XX:MaxInlineLevel=0
before  656.791 ± 8.216  ns/op
after  340.999 ± 2.686  ns/op

-Xint
before  36113.441 ± 197.716  ns/op
after   26928.593 ± 133.309  ns/op

-Dmitry


Shrug; maybe.  We shouldn't mess the code up for -Xcomp.

Re: Which test groups to run instead of :jdk (8176838)

[Alexandre (Shura) Iline]

> On Nov 27, 2017, at 11:56 PM, Lindenmaier, Goetz  
> wrote:
> 
> Hi Shura, 
> 
> thanks for your advice!  So we'll run all of them. The number of additional
> tests is small enough to handle in case they fail.
> 
> Do you mind if I put this advice into a comment in the bug?

I would appreciate it!

Thanks you.

Shura


> 
> Best regards,
>  Goetz.
> 
>> -Original Message-
>> From: Alexandre (Shura) Iline [mailto:alexandre.il...@oracle.com]
>> Sent: Montag, 27. November 2017 22:56
>> To: Lindenmaier, Goetz 
>> Cc: core-libs-dev@openjdk.java.net
>> Subject: Re: Which test groups to run instead of :jdk (8176838)
>> 
>> Hi, Goetz.
>> 
>> :jdk, when created, was supposed to include tests which could be run by full
>> JDK, which is, well, all the tests. Over the time there could have been new
>> tests added which were not included in the :jdk group.
>> 
>> In fact, I have just now rolled the repository back to before the 8176838
>> change and checked test count in :jdk test group vs. full test suite.
>> 
>> $jtreg … -l test/jdk/:jdk
>> …
>> Tests found: 8,563
>> $jtreg … -l test/jdk
>> …
>> Tests found: 8,583
>> 
>> The difference consists in these tests:
>> 
>>> native_sanity/simplenativelauncher/ProgramTest.java
>>> native_sanity/simplenativelib2/NativeLib.java
>>> native_sanity/simplenativelib/NativeLib.java
>>> org/omg/CORBA/OrbPropertiesTest.java
>>> sanity/client/SwingSet/src/ButtonDemoScreenshotTest.java
>>> sanity/client/SwingSet/src/ButtonDemoTest.java
>>> sanity/client/SwingSet/src/ComboBoxDemoTest.java
>>> sanity/client/SwingSet/src/DialogDemoTest.java
>>> sanity/client/SwingSet/src/ListDemoTest.java
>>> sanity/client/SwingSet/src/OptionPaneDemoTest.java
>>> sanity/client/SwingSet/src/ProgressBarDemoTest.java
>>> sanity/client/SwingSet/src/ScrollPaneDemoTest.java
>>> sanity/client/SwingSet/src/SliderDemoTest.java
>>> sanity/client/SwingSet/src/SpinnerDemoTest.java
>>> sanity/client/SwingSet/src/SplitPaneDemoTest.java
>>> sanity/client/SwingSet/src/TabbedPaneDemoTest.java
>>> sanity/client/SwingSet/src/TextFieldDemoTest.java
>>> sanity/client/SwingSet/src/ToggleButtonDemoTest.java
>>> sanity/client/SwingSet/src/TreeDemoTest.java
>>> sanity/client/SwingSet/src/WindowDemoTest.java
>> 
>> Shura
>> 
>> 
>> 
>>> On Nov 24, 2017, at 12:46 AM, Lindenmaier, Goetz
>>  wrote:
>>> 
>>> Hi,
>>> 
>>> we used to run test target :jdk.
>>> This has been removed in 8176838.
>>> 
>>> Which target should we run now to get the same or similar set of tests?
>>> 
>>> Best regards,
>>> Goetz.
> 

Re: Review Request: JDK-8191942: Replace jdeps use of jdk.internal.util.jar.VersionedStream with new public API

[Erik Joelsson]

Build change looks good.

/Erik

On 2017-11-27 17:50, mandy chung wrote:
This is a follow-up on JDK-8189611 that defines a new public API to 
return a stream of versioned entries and to return the real name of a 
JAR entry.  JDK-8189611 leaves jdeps untouched because jdeps is 
compiled with the boot JDK.


This patch includes:
(1) changes the build not to build jdk.jdeps interim module but 
include com.sun.tools.classfile classes in the CreateSymbol buildtool
(2) replace the use of internal API with the public APIs defined by 
JDK-8191942

(3) remove jdk.internal.util.jar.VersonedStream class

Webrev:
http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8191942/webrev.00/

Thanks
Mandy

Re: RFR: jsr166 jdk10 integration wave 6

[Paul Sandoz]

> On 27 Nov 2017, at 20:19, Martin Buchholz  wrote:
> 
> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/overview.html
> 
> Thanks to Dávid Karnok and Pavel Rappo for help with SubmissionPublisher.
> 
> 8191937: Lost interrupt in AbstractQueuedSynchronizer when tryAcquire methods 
> throw

AbstractQueuedSynchronizerTest.java
—

1296 public void testCancelCancelRace() throws InterruptedException {

Suggest “DISABLED_” prefix rather than “?


> 8187947: A race condition in SubmissionPublisher

Need some more time to look at this; it is pleasing to see use of 
VH.getAndBitwiseOr :-)


> 8191069: Miscellaneous changes imported from jsr166 CVS 2017-12

+1

Paul.

Re: ThreadPoolExecutor and finalization

[Roger Riggs]

Hi,

So this thread died out a while ago with some alternatives discussed and 
no clear short term solution.
I'd be happy if someone closer to the ThreadPoolExecutor would be able 
to take another look at the issues.

For the time being, it is lower down on my priorities.

Thanks, Roger

[1] 8190324 ThreadPoolExecutor should not specify a dependency on 
finalization 

RFR (JDK10/JAXP) 8191938: Fix lint warnings in JAXP repo: a few Deprecation warrnings and enable -Xlint:all

[Joe Wang]

Hi,

Please review a fix for a few more deprecation warnings. Compiling with 
-Xlint:all showed that these were the last few warnings. We can then 
enable -Xlint:all for the java.xml module.


JBS: https://bugs.openjdk.java.net/browse/JDK-8191938
webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8191938/webrev/

Thanks,
Joe

Re: RFR: jsr166 jdk10 integration wave 6

[Doug Lea]

On 11/28/2017 12:50 PM, Paul Sandoz wrote:

> 
>> 8187947: A race condition in SubmissionPublisher
> 
> Need some more time to look at this; it is pleasing to see use of 
> VH.getAndBitwiseOr :-)
> 
> 

Yes; not only is it the technique that allows covering this
borderline-misuse case without slowing down others, but it also
makes for simpler code.
And even on those machines not natively supporting, the internal
CAS or LL/SC loops to implement are likely faster than what we can do
from Java. I plan to update some other j.u.c classes to use it when
applicable (although not immediately).

-Doug

Re: ThreadPoolExecutor and finalization

[Doug Lea]

On 11/28/2017 01:11 PM, Roger Riggs wrote:
> Hi,
> 
> So this thread died out a while ago with some alternatives discussed and
> no clear short term solution.
> I'd be happy if someone closer to the ThreadPoolExecutor would be able
> to take another look at the issues.
> For the time being, it is lower down on my priorities.

I still think the best move is to delete the method, and
tweak the documentation.

-Doug

Re: RFR (JDK10/JAXP) 8191938: Fix lint warnings in JAXP repo: a few Deprecation warrnings and enable -Xlint:all

[joe darcy]

Hi Joe,

The code changes look fine, but the copyright blocks should *not* be 
updated to include a "@LastModified: Nov 2017" comment.


Cheers,

-Joe


On 11/28/2017 10:11 AM, Joe Wang wrote:

Hi,

Please review a fix for a few more deprecation warnings. Compiling 
with -Xlint:all showed that these were the last few warnings. We can 
then enable -Xlint:all for the java.xml module.


JBS: https://bugs.openjdk.java.net/browse/JDK-8191938
webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8191938/webrev/

Thanks,
Joe

Re: [10] RFR 8176841: Additional Unicode Language-Tag Extensions

[Lance Andersen]

+1
> On Nov 22, 2017, at 2:04 PM, Naoto Sato  wrote:
> 
> I revised the proposed changes, including java.time changes suggested by 
> Stephen (CSR is still in progress):
> 
> https://bugs.openjdk.java.net/browse/JDK-8191349
> 
> The entire webrev is located at:
> 
> http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.05/
> 
> And the diff webrev from the last one is located at:
> 
> http://cr.openjdk.java.net/~naoto/8191349/webrev.04-05/
> 
> I'd appreciate your reviews.
> 
> Naoto
> 
> 
> On 11/9/17 3:34 PM, Naoto Sato wrote:
>> Kindly requesting reviews. I incorporated a fix to the following issue 
>> raised by the test team:
>> https://bugs.openjdk.java.net/browse/JDK-8190918
>> Here is the updated webrev:
>> http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918/webrev.04/
>> And the webrev since the one below (to address 8190918):
>> http://cr.openjdk.java.net/~naoto/8190918/
>> Naoto
>> On 11/2/17 2:42 PM, Naoto Sato wrote:
>>> Hello,
>>> 
>>> Please review the proposed changes for the following issues:
>>> 
>>> 8176841: Additional Unicode Language-Tag Extensions
>>> 8189134: New system properties for the default Locale extensions
>>> 
>>> The proposed changeset is located at:
>>> 
>>> http://cr.openjdk.java.net/~naoto/8176841/webrev.03/
>>> 
>>> This serves as the implementation of JEP 314.
>>> 
>>> Naoto
>>> 
>>> 
>>> 

 
  

 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 (JDK10/JAXP) 8191938: Fix lint warnings in JAXP repo: a few Deprecation warrnings and enable -Xlint:all

[huizhe wang]

Thanks for reviewing!

For the last modified part, we can discuss updating the header script 
offline. Many of the classes have already used this format, I hope 
you're okay with me checking in this changeset.


Thanks,
Joe

On 11/28/2017 10:19 AM, joe darcy wrote:

Hi Joe,

The code changes look fine, but the copyright blocks should *not* be 
updated to include a "@LastModified: Nov 2017" comment.


Cheers,

-Joe


On 11/28/2017 10:11 AM, Joe Wang wrote:

Hi,

Please review a fix for a few more deprecation warnings. Compiling 
with -Xlint:all showed that these were the last few warnings. We can 
then enable -Xlint:all for the java.xml module.


JBS: https://bugs.openjdk.java.net/browse/JDK-8191938
webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8191938/webrev/

Thanks,
Joe

Re: ThreadPoolExecutor and finalization

[David Holmes]

+1

David

On 29/11/2017 4:14 AM, Doug Lea wrote:

On 11/28/2017 01:11 PM, Roger Riggs wrote:

Hi,

So this thread died out a while ago with some alternatives discussed and
no clear short term solution.
I'd be happy if someone closer to the ThreadPoolExecutor would be able
to take another look at the issues.
For the time being, it is lower down on my priorities.


I still think the best move is to delete the method, and
tweak the documentation.

-Doug

Re: RFR: jsr166 jdk10 integration wave 6

[Martin Buchholz]

On Tue, Nov 28, 2017 at 9:50 AM, Paul Sandoz  wrote:

>
> 1296 public void testCancelCancelRace() throws
> InterruptedException {
>
> Suggest “DISABLED_” prefix rather than “?
>

done.

Re: [10] RFR 8176841: Additional Unicode Language-Tag Extensions

[Naoto Sato]

I've got some internal comments (two editorial fixes and java time test 
location move) and reflected them to the existing fix. Updated webrevs 
are located at:


http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.07/
http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.06-07/ 
(from v.06)


Naoto

On 11/27/17 1:26 PM, Stephen Colebourne wrote:

This fixes my previous points, so fine by me. But I am not an OpenJDK reviewer.
Stephen

On 27 November 2017 at 20:54, Naoto Sato  wrote:

Thanks, Stephen. Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.06/

Naoto


On 11/23/17 8:13 AM, Stephen Colebourne wrote:


In DateTimeFormatter line 1508, this would be preferred:

return  new DateTimeFormatter(printerParser, locale, ds,
resolverStyle, resolverFields, c, z);

In DateTimeFormatterBuilder.getLocalizedDateTimePattern() there is no
spec change wrt using "rg".

Should findRegionOverride() just return a Locale instead of am
Optional? It always seems to have an orElse(locale).

Java-Time tests look good.

thanks
Stephen


On 22 November 2017 at 19:04, Naoto Sato  wrote:


I revised the proposed changes, including java.time changes suggested by
Stephen (CSR is still in progress):

https://bugs.openjdk.java.net/browse/JDK-8191349

The entire webrev is located at:


http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.05/

And the diff webrev from the last one is located at:

http://cr.openjdk.java.net/~naoto/8191349/webrev.04-05/

I'd appreciate your reviews.

Naoto



On 11/9/17 3:34 PM, Naoto Sato wrote:



Kindly requesting reviews. I incorporated a fix to the following issue
raised by the test team:

https://bugs.openjdk.java.net/browse/JDK-8190918

Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918/webrev.04/

And the webrev since the one below (to address 8190918):

http://cr.openjdk.java.net/~naoto/8190918/

Naoto



On 11/2/17 2:42 PM, Naoto Sato wrote:



Hello,

Please review the proposed changes for the following issues:

8176841: Additional Unicode Language-Tag Extensions
8189134: New system properties for the default Locale extensions

The proposed changeset is located at:

http://cr.openjdk.java.net/~naoto/8176841/webrev.03/

This serves as the implementation of JEP 314.

Naoto

[10] (S) RFR 8191788: add jdk.internal.vm.compiler to --limit-modules if -Djvmci.Compiler=graal is in the command line

[Vladimir Kozlov]

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler to tests which have 
--limit-modules option. Unfortunately tests start failing on SPARC where Graal 
(jdk.internal.vm.compiler) is not available. JDK-8191653 [2] reversed 8190975 changes to fix that 
problem.


This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM only when Graal is used: when 
it is explicitly specified with -Djvmci.Compiler=graal or in default case and when UseJVMCICompiler 
is true.


I tested the fix with failed tests from JDK-8190975 which are mostly AppCDS tests in 
test/runtime/appcds/jigsaw and also test/jdk/java/lang/String/concat/WithSecurityManager.java


I think this fix may break upgradeable status of Graal (for Oracle Labs version 
of Graal).
But it should be fine since it is only used with --limit-modules which is not 
used by Labs.

Thanks,
Vladimir

[1] http://hg.openjdk.java.net/jdk/hs/rev/8deb7919d118
[2] http://hg.openjdk.java.net/jdk/hs/rev/b38d8aadcada

Re: [10] RFR 8176841: Additional Unicode Language-Tag Extensions

[Lance Andersen]

Updates look good :-)
> On Nov 28, 2017, at 6:27 PM, Naoto Sato  wrote:
> 
> I've got some internal comments (two editorial fixes and java time test 
> location move) and reflected them to the existing fix. Updated webrevs are 
> located at:
> 
> http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.07/
> http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.06-07/
>  (from v.06)
> 
> Naoto
> 
> On 11/27/17 1:26 PM, Stephen Colebourne wrote:
>> This fixes my previous points, so fine by me. But I am not an OpenJDK 
>> reviewer.
>> Stephen
>> On 27 November 2017 at 20:54, Naoto Sato  wrote:
>>> Thanks, Stephen. Here is the updated webrev:
>>> 
>>> http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.06/
>>> 
>>> Naoto
>>> 
>>> 
>>> On 11/23/17 8:13 AM, Stephen Colebourne wrote:
 
 In DateTimeFormatter line 1508, this would be preferred:
 
return  new DateTimeFormatter(printerParser, locale, ds,
 resolverStyle, resolverFields, c, z);
 
 In DateTimeFormatterBuilder.getLocalizedDateTimePattern() there is no
 spec change wrt using "rg".
 
 Should findRegionOverride() just return a Locale instead of am
 Optional? It always seems to have an orElse(locale).
 
 Java-Time tests look good.
 
 thanks
 Stephen
 
 
 On 22 November 2017 at 19:04, Naoto Sato  wrote:
> 
> I revised the proposed changes, including java.time changes suggested by
> Stephen (CSR is still in progress):
> 
> https://bugs.openjdk.java.net/browse/JDK-8191349
> 
> The entire webrev is located at:
> 
> 
> http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918.8191349/webrev.05/
> 
> And the diff webrev from the last one is located at:
> 
> http://cr.openjdk.java.net/~naoto/8191349/webrev.04-05/
> 
> I'd appreciate your reviews.
> 
> Naoto
> 
> 
> 
> On 11/9/17 3:34 PM, Naoto Sato wrote:
>> 
>> 
>> Kindly requesting reviews. I incorporated a fix to the following issue
>> raised by the test team:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8190918
>> 
>> Here is the updated webrev:
>> 
>> http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918/webrev.04/
>> 
>> And the webrev since the one below (to address 8190918):
>> 
>> http://cr.openjdk.java.net/~naoto/8190918/
>> 
>> Naoto
>> 
>> 
>> 
>> On 11/2/17 2:42 PM, Naoto Sato wrote:
>>> 
>>> 
>>> Hello,
>>> 
>>> Please review the proposed changes for the following issues:
>>> 
>>> 8176841: Additional Unicode Language-Tag Extensions
>>> 8189134: New system properties for the default Locale extensions
>>> 
>>> The proposed changeset is located at:
>>> 
>>> http://cr.openjdk.java.net/~naoto/8176841/webrev.03/
>>> 
>>> This serves as the implementation of JEP 314.
>>> 
>>> Naoto
>>> 
>>> 
>>> 
> 
>>> 

 
  

 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-8186087: jar tool fails to create a multi-release jar when validating nested classes

[Paul Sandoz]

+1

Validator
--
 365 public static void main(String[] args) throws IOException {
 366 System.out.println("validating: " +
 367 new Validator(new Main(System.out, System.err, "validator"),
 368   new ZipFile(args[0]))
 369 .validate());
 370 }

Left over from debugging?

Paul.

> On 27 Nov 2017, at 18:57, Xueming Shen  wrote:
> 
> Hi
> 
> Please help review the change for #8186087
> 
> Issue: https://bugs.openjdk.java.net/browse/JDK-8186087
> webrev: http://cr.openjdk.java.net/~sherman/8186087/webrev
> 
> The proposed change is to handle the "outer class" of a  nested class 
> correctly, instead of simply
> relying on "top level" class. Other than that, a "minor" change is to also 
> stop trying to "validate"
> the non-class/resource entries, for example, if you have a resource "foo" 
> both in the base and
> versioned directory, and with different "content", currently implementation 
> throws a warning
> message, which does not appear to be desired and appropriate.
> 
> There is a "known/open" issue during discussion regarding how to handle the 
> entry that its class
> name does not match its entry name (for example, class p.foo.class is located 
> at bar/p/foo.class,
> currently jar.validator fails the validation with error msg). It appears to 
> be a separate issue, so
> I'm leaving that one of this one for now.
> 
> Thanks,
> Sherman

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Stuart Marks]

On 11/17/17 9:43 PM, John Rose wrote:

Late to the party, but these lines rub me the wrong way:

@return the new {@code List}
@return the new {@code Set}
@return the new {@code Map}

The word "new" is a loaded term, which usually means
(or can be easily mistaken to mean) that a new object
identity is guaranteed.  Thus, "new" shouldn't be used
to specify the behavior of value-based classes.

Given that that the underlying objects are of VBCs,
and that we are encouraging programmers to rely on
the efficiency of chained copies, it should say something
like this instead:

@return a {@code List} containing the same elements as the given collection
@return a {@code Set} containing the same elements as the given collection
@return a {@code Map} containing the same mappings as the given map

(Or even s/return a/return an unmodifiable/.)


OK, per your other message, we'll stick with 
Collectors.toUnmodifiableList/Set/Map, in the hope that in the future we can 
work on improving various incarnations of toList() to have more desirable 
properties.


Meanwhile, I agree that "@return the new {@code List}" is wrong and I'll adjust 
it (respectively, Set and Map).


Another point from our discussions is that the copyOf() methods should get an 
@implNote that says they'll try not to make unnecessary copies.


Also included are some markup changes and a small Set.copyOf implementation 
change suggested upthread.


Final(?) webrev:

http://cr.openjdk.java.net/~smarks/reviews/8177290/webrev.3/

Specdiff:


http://cr.openjdk.java.net/~smarks/reviews/8177290/specdiff.3/overview-summary.html

s'marks

Re: [10] (S) RFR 8191788: add jdk.internal.vm.compiler to --limit-modules if -Djvmci.Compiler=graal is in the command line

[David Holmes]

Hi Vladimir,

On 29/11/2017 9:51 AM, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler 
to tests which have --limit-modules option. Unfortunately tests start 
failing on SPARC where Graal (jdk.internal.vm.compiler) is not 
available. JDK-8191653 [2] reversed 8190975 changes to fix that problem.


This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM 
only when Graal is used: when it is explicitly specified with 
-Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is 
true.


This seems a reasonable way to handle this - at least for now.

Typo:

3379 // and --limit-modules are spcified.

spcified -> specified

Thanks,
David
-

I tested the fix with failed tests from JDK-8190975 which are mostly 
AppCDS tests in test/runtime/appcds/jigsaw and also 
test/jdk/java/lang/String/concat/WithSecurityManager.java


I think this fix may break upgradeable status of Graal (for Oracle Labs 
version of Graal).
But it should be fine since it is only used with --limit-modules which 
is not used by Labs.


Thanks,
Vladimir

[1] http://hg.openjdk.java.net/jdk/hs/rev/8deb7919d118
[2] http://hg.openjdk.java.net/jdk/hs/rev/b38d8aadcada

Re: RFR(m): 8177290 add copy factory methods for unmodifiable List, Set, Map

[Stuart Marks]

On 11/22/17 8:45 AM, Remi Forax wrote:

I think i prefer toImmutableList() than toUnmodifiableList() because the List
is truly immutable and not an unmodifiable proxy in front of a mutable List
(like Collections.unmodifiableList() does).


Immutability is like wine.

If you put a spoonful of wine in a barrel full of sewage, you get sewage. If you 
put a spoonful of sewage in a barrel full of wine, you get sewage. 
(Schopenhauer's Law of Entropy)


Similarly, if you add a little immutability to something mutable, you get 
mutability. And if you add a little mutability to something immutable, you get 
mutability.


To get the desired properties of immutability (e.g., thread-safety, or the 
ability to hand around references safely without making defensive copies), it's 
insufficient for a collection to be "immutable"; you also have to make some 
statements about the contents. It's thus more precise to say that a collection 
is unmodifiable, and to provide a clear definition of an unmodifiable collection.


Although unmodifiable collections have been around since the collections 
framework was introduced, oddly enough the concept has never had a good 
definition. The current proposal includes definitions for unmodifiability, 
unmodifiable collections, and unmodifiable views, and it clearly specifies which 
APIs return unmodifiable views and which return unmodifiable collections.


s'marks

Re: [10] (S) RFR 8191788: add jdk.internal.vm.compiler to --limit-modules if -Djvmci.Compiler=graal is in the command line

[Calvin Cheung]

Hi Vladimir,

Fix looks good to me.

Could you break up lines #3386 (104 chars) and #3391 (146 chars)?
No need to send another webrev for the above change.

thanks,
Calvin

On 11/28/17, 3:51 PM, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added 
jdk.internal.vm.compiler to tests which have --limit-modules option. 
Unfortunately tests start failing on SPARC where Graal 
(jdk.internal.vm.compiler) is not available. JDK-8191653 [2] reversed 
8190975 changes to fix that problem.


This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM 
only when Graal is used: when it is explicitly specified with 
-Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is 
true.


I tested the fix with failed tests from JDK-8190975 which are mostly 
AppCDS tests in test/runtime/appcds/jigsaw and also 
test/jdk/java/lang/String/concat/WithSecurityManager.java


I think this fix may break upgradeable status of Graal (for Oracle 
Labs version of Graal).
But it should be fine since it is only used with --limit-modules which 
is not used by Labs.


Thanks,
Vladimir

[1] http://hg.openjdk.java.net/jdk/hs/rev/8deb7919d118
[2] http://hg.openjdk.java.net/jdk/hs/rev/b38d8aadcada

Re: [10] (S) RFR 8191788: add jdk.internal.vm.compiler to --limit-modules if -Djvmci.Compiler=graal is in the command line

[Vladimir Kozlov]

Thank you, David

On 11/28/17 8:57 PM, David Holmes wrote:

Hi Vladimir,

On 29/11/2017 9:51 AM, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler to tests which have --limit-modules option. 
Unfortunately tests start failing on SPARC where Graal (jdk.internal.vm.compiler) is not available. JDK-8191653 [2] 
reversed 8190975 changes to fix that problem.


This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM only when Graal is used: when it is explicitly 
specified with -Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is true.


This seems a reasonable way to handle this - at least for now.

Typo:

3379 // and --limit-modules are spcified.

spcified -> specified


Will fix.

Vladimir



Thanks,
David
-

I tested the fix with failed tests from JDK-8190975 which are mostly AppCDS tests in test/runtime/appcds/jigsaw and 
also test/jdk/java/lang/String/concat/WithSecurityManager.java


I think this fix may break upgradeable status of Graal (for Oracle Labs version 
of Graal).
But it should be fine since it is only used with --limit-modules which is not 
used by Labs.

Thanks,
Vladimir

[1] http://hg.openjdk.java.net/jdk/hs/rev/8deb7919d118
[2] http://hg.openjdk.java.net/jdk/hs/rev/b38d8aadcada

Re: [10] (S) RFR 8191788: add jdk.internal.vm.compiler to --limit-modules if -Djvmci.Compiler=graal is in the command line

[Vladimir Kozlov]

Thank you, Calvin

On 11/28/17 10:15 PM, Calvin Cheung wrote:

Hi Vladimir,

Fix looks good to me.

Could you break up lines #3386 (104 chars) and #3391 (146 chars)?


I will do that.

Vladimir


No need to send another webrev for the above change.

thanks,
Calvin

On 11/28/17, 3:51 PM, Vladimir Kozlov wrote:

http://cr.openjdk.java.net/~kvn/8191788/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8191788

This is redo of JDK-8190975 [1] fix which added jdk.internal.vm.compiler to tests which have --limit-modules option. 
Unfortunately tests start failing on SPARC where Graal (jdk.internal.vm.compiler) is not available. JDK-8191653 [2] 
reversed 8190975 changes to fix that problem.


This fix adds jdk.internal.vm.compiler to --limit-modules list in JVM only when Graal is used: when it is explicitly 
specified with -Djvmci.Compiler=graal or in default case and when UseJVMCICompiler is true.


I tested the fix with failed tests from JDK-8190975 which are mostly AppCDS tests in test/runtime/appcds/jigsaw and 
also test/jdk/java/lang/String/concat/WithSecurityManager.java


I think this fix may break upgradeable status of Graal (for Oracle Labs version 
of Graal).
But it should be fine since it is only used with --limit-modules which is not 
used by Labs.

Thanks,
Vladimir

[1] http://hg.openjdk.java.net/jdk/hs/rev/8deb7919d118
[2] http://hg.openjdk.java.net/jdk/hs/rev/b38d8aadcada