Re: RFR(M): 8152172: PPC64: Support AES intrinsics

[Vladimir Kozlov]

This question is for core libs.

For JIT to generate optimal code we want to change type of AESCrypt::sessionK 
from Object[] to int[][].
Otherwise JIT have to generate checkcast code to make sure that elements of 
sessionK array are int[].
I see that sessionK field is private so we are not changing public API.

Thanks,
Vladimir

On 3/24/16 6:17 PM, Hiroshi H Horii wrote:

Hi Vladimir,

Thank you for your reviewing.

Is it possible to modify the type of sessionK from Object[] to int[][]?

We can guess, the type was designed for flexibility. However, only int[] is used
to store elements of sessionK, so far. In addition, this field is private.
Though adding checkcast is another solution, it produces overheads.

I attached an additional patch (generated with hg diff -g) to jdk directory.
I would like to ask thoughts about this change of java code.



Regards,
Hiroshi
---
Hiroshi Horii,
IBM Research - Tokyo


Vladimir Kozlov  wrote on 03/24/2016 09:09:11:

 > From: Vladimir Kozlov 
 > To: Hiroshi H Horii/Japan/IBM@IBMJP, "hotspot-compiler-
 > d...@openjdk.java.net" 
 > Cc: "Doerr, Martin" , Tim Ellison
 > , "Simonis, Volker" 
 > Date: 03/24/2016 09:19
 > Subject: Re: RFR(M): 8152172: PPC64: Support AES intrinsics
 >
 > I think we need to add gen_checkcast for objAESCryptKey node since
 > java code has no guarantee that sessionK array elements are
 > initialized with int[]. Or we should modify java code to declare sessionK
 > as int[][].
 >
 > Thanks,
 > Vladimir
 >
 > Note, intrinsics are correctly handle case when
 > On 3/22/16 8:47 AM, Hiroshi H Horii wrote:
 > > Dear all:
 > >
 > > Can I please request reviews for the following change?
 > > This change was created for JDK 9 to enable POWER8's AES
 > > instructions for AES calculation.
 > >
 > > This request follows this discussion.
 > > http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2016-
 > March/021926.html
 > >
 > > Description:
 > > This change adds stub routines support for single-block AES
 > > encryption and decryption operations on the POWER8 platform.
 > > They are available only when the application is configured to
 > > use SunJCE crypto provider on little endian.
 > >
 > > These stubs make use of efficient hardware AES instructions
 > > and thus offer significant performance improvements over
 > > JITed code on POWER8 as on x86 and SPARC. AES stub routines
 > > are enabled by default on POWER8 platforms that support AES
 > > instructions (vcipher). They can be explicitly enabled or
 > > disabled on the command-line using UseAES and
 > > UseAESIntrinsics JVM flags. Unlike x86 and SPARC, vcipher
 > > and vnchiper of POWER8 need the same round keys of AES.
 > > Therefore, inline_aescrypt_Block in library_call.cpp calls the
 > > stub with AESCrypt.sessionK[0] as round keys.
 > >
 > > Summary of source code changes:
 > >
 > >   *src/cpu/ppc/vm/assembler_ppc.hpp
 > >   *src/cpu/ppc/vm/assembler_ppc.inline.hpp
 > > - Adds support for vrld instruction to rotate vector register values
 > >with left doubleword.
 > >
 > >   *src/cpu/ppc/vm/stubGenerator_ppc.cpp
 > > - Defines stubs for single-block AES encryption and decryption
 > >routines supporting all key sizes (128, 192 and 256-bit).
 > > - Current POWER AES decryption instructions are not compatible
 > >with SunJCE expanded decryption key format. Thus decryption
 > >stubs read the expanded encryption keys (sessionK[0]) with
 > >descendant order.
 > > - Encryption stubs use SunJCE expanded encryption key as their
 > >is no incompatibility issue between POWER8 AES encryption
 > >instructions and SunJCE expanded encryption keys.
 > >
 > >   *src/cpu/ppc/vm/vm_version_ppc.cpp
 > > - Detects AES capabilities of the underlying CPU by using
 > >has_vcipher().
 > > - Enables UseAES and UseAESIntrinsics flags if the underlying
 > >CPU supports AES instructions and neither of them is explicitly
 > >disabled on the command-line. Generate warning message if
 > >either of these flags are enabled on the command-line
 > >whereas the underlying CPU does not support AES instructions.
 > >
 > >   *src/share/vm/opto/library_call.cpp
 > >  - Passes the first input parameter, reference to sessionK[0] to the
 > > AES stubs only on the POWER platform.
 > >
 > >*src/share/vm/opto/graphKit.cpp
 > >  - Supports T_NARROWOOP type for GraphKit::load_array_element.
 > >
 > > Bug: https://bugs.openjdk.java.net/browse/JDK-8152172
 > > Webrev: http://cr.openjdk.java.net/~mdoerr/8152172_ppc64le_aes/webrev.00/
 > >
 > >
 > > Regards,
 > > Hiroshi
 > > ---
 > > Hiroshi Horii,
 > > IBM Research - Tokyo
 > >
 >

Re: RFR: JDK-8149925 We don't need jdk.internal.ref.Cleaner any more

[Mandy Chung]

> On Mar 19, 2016, at 7:00 AM, Peter Levart  wrote:
> 
> Here's the webrev:
> 
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/removeInternalCleaner/webrev.08.part2/
> 
>> 
>> On 03/07/2016 07:35 PM, Mandy Chung wrote:
>>> 
>>> I studied webrev.06priv and the history of JDK-6857566. 
>>> 
>>> I’m not comfortable for any arbitrary thread to handle the enqueuing of the 
>>> pending references (this change is more about the fix for JDK-6857566).
>>> 
>> 
>> Why? A Thread is a Thread is a Thread... When legacy Cleaner is removed, 
>> ReferenceHandler thread will be left with swapping pointers only - no custom 
>> code will be involved. The only things I can think of against using 
>> arbitrary thread are:
>> :

My uncomfort was the fix for JDK-6857566 - both enqueuing pending ref and 
invoking the cleaning code in an arbitrary thread.  

Looking at it again - enqueuing the pending reference is not so much of a 
concern (simply updating the link) but the common cleaner could be used by 
other code that may only expect to be invoked in system thread that’s still my 
concern (thinking of thread locals).  On the other hand, invoking 
Deallocator::run (deallocating the native memory) in arbitrary threads has no 
problem.  Consider me being paranoid of the fix for JDK-6857566.  The current 
list of clients using CleanerFactory::cleaner may be safe being called from 
arbitrary threads but I can’t say what will be added in the future.

>> 
>>> The allocating thread may do a System.gc() that may discover phantom 
>>> reachable references.  All it’s interested is only the direct byte buffer 
>>> ones so that it can deallocate the native memory.  What is the downside of 
>>> having a dedicated Cleaner for direct byte buffer that could special case 
>>> for it?
>> 
>> A dedicated Cleaner for direct buffers might be a good idea if other uses of 
>> shared Cleaner in JDK become heavy. So that helping process Cleanable(s) 
>> does not involve other unrelated Cleanable(s). But it comes with a price of 
>> another dedicated background thread.
>> 

Perhaps provide one Cleaner specific for native memory deallocation or anything 
safe to be called in arbitrary thread.  It could provide the entry point for 
the allocating thread to assist the cleaning (i.e. Bits::reserveMemory could 
call it).  That will make it explicit that this cleaner provides explicit 
control for other threads to assist the cleaning action (and JavaLangRefAccess 
would only be used by this special cleaner and not in NIO).

All clients of Unsafe.freeMemory could use that special cleaner for native 
memory deallocation use such as IOVecWrapper, DirectByteBuffer, Marlin’s 
OffHeapArray.

The common cleaner would be kept for other things to use and it should be 
lazily created to avoid another thread.

Does this sound reasonable?

Mandy

Re: RFR: 8073872: Schemagen fails with StackOverflowError if element references containing class

[Lance Andersen]

Yes I looked at everything

Best
Lance
On Mar 25, 2016, at 3:45 PM, Aleksej Efimov  wrote:

> Thank you for review Lance!
> Small question for clarification: Can I count your review for JDK8 changes 
> too?
> 
> Best Regards,
> Aleksej
> 
> On 03/25/2016 08:46 PM, Lance Andersen wrote:
>> Looks OK Alejsej
>> 
>> On Mar 24, 2016, at 7:38 PM, Aleksej Efimov  
>> wrote:
>> 
>>> Hi,
>>> 
>>> Please, help to review the fix for JDK-8073872 [1]. In JDK9 this bug was 
>>> fixed as part of sync with upstream JAXWS project [2]. The changes that 
>>> fixed stack overflow issue are located in XmlSchemaGenerator class [3].
>>> JDK9 change that needs review is the addition of regression test for this 
>>> issue:
>>> http://cr.openjdk.java.net/~aefimov/8073872/9/00/
>>> 
>>> I will be requesting JDK8 approval for partially backporting the fix for 
>>> this bug to 8u-dev. Can I also ask to review the JDK8 changes:
>>> http://cr.openjdk.java.net/~aefimov/8073872/8/00/
>>> 
>>> Both JDK8/9 changes were tested with JCK/JTREG tests and no failures were 
>>> observed. New test passes with the fix and successfully reproduces reported 
>>> stack overflow on JDKs without the fix.
>>> 
>>> Best Regards,
>>> Aleksej
>>> 
>>> [1] https://bugs.openjdk.java.net/browse/JDK-8073872
>>> [2] https://bugs.openjdk.java.net/browse/JDK-8150174
>>> [3] http://hg.openjdk.java.net/jdk9/jdk9/jaxws/rev/ebff1bd3627a#l3.2
>> 
>> 
>> 
>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering 
>> 1 Network Drive 
>> Burlington, MA 01803
>> lance.ander...@oracle.com
>> 
>> 
>> 
> 



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: 8073872: Schemagen fails with StackOverflowError if element references containing class

[Aleksej Efimov]

Thank you for review Lance!
Small question for clarification: Can I count your review for JDK8 
changes too?


Best Regards,
Aleksej

On 03/25/2016 08:46 PM, Lance Andersen wrote:

Looks OK Alejsej

On Mar 24, 2016, at 7:38 PM, Aleksej Efimov > wrote:



Hi,

Please, help to review the fix for JDK-8073872 [1]. In JDK9 this bug 
was fixed as part of sync with upstream JAXWS project [2]. The 
changes that fixed stack overflow issue are located in 
XmlSchemaGenerator class [3].
JDK9 change that needs review is the addition of regression test for 
this issue:
http://cr.openjdk.java.net/~aefimov/8073872/9/00/ 



I will be requesting JDK8 approval for partially backporting the fix 
for this bug to 8u-dev. Can I also ask to review the JDK8 changes:

http://cr.openjdk.java.net/~aefimov/8073872/8/00/

Both JDK8/9 changes were tested with JCK/JTREG tests and no failures 
were observed. New test passes with the fix and successfully 
reproduces reported stack overflow on JDKs without the fix.


Best Regards,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8073872
[2] https://bugs.openjdk.java.net/browse/JDK-8150174
[3] http://hg.openjdk.java.net/jdk9/jdk9/jaxws/rev/ebff1bd3627a#l3.2




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: [8u-dev] Request for approval: 8150704: XALAN: ERROR: 'No more DTM IDs are available' when transforming with lots of temporary result trees

[Aleksej Efimov]

Hi Rob,
Sure!
Adding corelibs-dev and Joe (JDK9 reviewer).

Hi Joe,
Can you, please, review the backported changes: The source fix is 
identical (except file paths), but regression test changes differs.


Best Regards,
Aleksej

On 03/25/2016 06:07 PM, Rob McKenna wrote:

Hi Aleksej,

I'd like to see a review of the 8 change before approval.

 -Rob

On 25/03/16 03:06, Aleksej Efimov wrote:

Hi,

Please, approve the backport of JDK-8150704 to JDK8. JAXP classes source fix
applies cleanly after reshuffling.
The only discrepancy is the test changes:
In JDK9 the issue was tested by extending the functionality of existing JDK9
test: jaxp/javax/xml/jaxp/unittest/transform/TransformerTest.java
Current test and test suite is absent in JDK8 and the added test case was
backported to JDK8 as a standalone test application:
jdk/test/javax/xml/jaxp/transform/8150704 - JDK8 webrev is provided.
Also the cleanup changes of test code wasn't backported because tests are
absent in JDK8.

JTREG/JCK testing shows no failures with backported changes.

Best Regards,
Aleksej

JBS:
https://bugs.openjdk.java.net/browse/JDK-8150704

JDK8 webrev:
http://cr.openjdk.java.net/~aefimov/8150704/8/00/

JDK9 changeset:
http://hg.openjdk.java.net/jdk9/dev/jaxp/rev/0fe7231b64a6

JDK9 Review thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039272.html

Re: RFR [9] 8152190: Move sun.misc.JarIndex and InvalidJarIndexException to an internal package

[Chris Hegarty]

Take 2.

InvalidJarIndexException is thrown when an index is corrupt. It is a useful 
piece of
information that the deployment of the jar files, on the class path, with 
indices, are
"bad". It is really an Error. It indicates a serious problem with the 
deployment that a
reasonable application should not try to handle.

Propose...

1) Update the Jar File Specification [1] ( 8152276 [2] ):

   "Once the class loader finds a INDEX.LIST file in a particular jar file, it 
always trusts
the information listed in it. If a mapping is found for a particular class, 
but the class
loader fails to find it by following the link,  ** an unspecified Error or 
RuntimeException **
is thrown. When this occurs, the application developer should rerun the jar 
tool on the
extension to get the right information into the index file."

  2) Update the implementation to throw an ( unspecified ) Error. ( 8152190 [3] 
) 

 No user code should every catch or try to handle this Error.
 No user code should every have tried to catch or handle 
InvalidJarIndexException
 
Webrev:
  http://cr.openjdk.java.net/~chegar/8152190/00/

-Chris.

[1] http://docs.oracle.com/javase/8/docs/technotes/guides/jar/jar.html#Overview
[2] https://bugs.openjdk.java.net/browse/JDK-8152276
[3] https://bugs.openjdk.java.net/browse/JDK-8152190



On 21 Mar 2016, at 17:42, Chris Hegarty  wrote:

> 
> On 21 Mar 2016, at 08:33, Alan Bateman  wrote:
> 
>> On 21/03/2016 05:53, Chris Hegarty wrote:
>>> sun.misc.JarIndex, and its accompanying InvalidJarIndexException, are not
>>> "Critical APIs", as defined by JEP 260, so they should be moved out of
>>> sun.misc and placed into a more appropriate package where they can
>>> be encapsulated.
>>> 
>>> http://cr.openjdk.java.net/~chegar/8152190/00/
>>> https://bugs.openjdk.java.net/browse/JDK-8152190
>>> 
>>> Since the Exception type is only ever statically referenced where it is
>>> thrown, I’ve moved it to a static nested class of JarIndex, so as to
>>> keep the namespace cleaner.
>>> 
>> It probably should be located with URLClassPath but we should try to avoid 
>> exporting jdk.internal.loader to other modules.
> 
> Agreed.
> 
>> I suspect the module-info.java in your webrev is based on a merge and 
>> exporting this package to java.rmi seems dubious. Can we create a bug to get 
>> this removed? It looks like RegistryImpl is using it out of convenience 
>> rather than necessity.
> 
> Correct. I filed 8152277 [1] to track this, and will have it out for review 
> soon.
> 
>> There's something fishy with InvalidJarIndexException. The JAR spec mentions 
>> it but it's a JDK-specific exception. If it's thrown by the APIs in 
>> java.util.jar then it should be specified in those APIs and the exception 
>> needs to be java.util.jar.
> 
> You may be right. I filed 8152276 [2] to track this issue.
> 
> Once 8152277 is fixed, I’ll rethink the solution to this issue.
> 
> -Chris.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8152277
> [2] https://bugs.openjdk.java.net/browse/JDK-8152276
> 

Re: RFR: 8152436: Add a test to verify that the root logger correctly reports the caller's information

[Lance Andersen]

Agree, would be nice to have this as a TestNG test but good otherwise.

Enjoy your time off Daniel

On Mar 25, 2016, at 2:12 PM, Mandy Chung  wrote:

> 
>> On Mar 22, 2016, at 8:03 AM, Daniel Fuchs  wrote:
>> 
>> Hi,
>> 
>> Please find below a new test that verifies that JDK-8152389 does
>> not occur in JDK 9.
>> 
>> bug:
>> 8152436: Add a test to verify that the root logger correctly
>>reports the caller's information
>> https://bugs.openjdk.java.net/browse/JDK-8152436
>> 
>> Issue being verified by the test:
>> https://bugs.openjdk.java.net/browse/JDK-8152389
>> 
>> Webrev (test only):
>> http://cr.openjdk.java.net/~dfuchs/webrev_8152436/webrev.00/
> 
> +1
> 
> It’d be nice to convert to TestNG test and could be done in a separate issue.
> 
> Mandy
> 



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

Re: RFR: 8152436: Add a test to verify that the root logger correctly reports the caller's information

[Mandy Chung]

> On Mar 22, 2016, at 8:03 AM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> Please find below a new test that verifies that JDK-8152389 does
> not occur in JDK 9.
> 
> bug:
> 8152436: Add a test to verify that the root logger correctly
> reports the caller's information
> https://bugs.openjdk.java.net/browse/JDK-8152436
> 
> Issue being verified by the test:
> https://bugs.openjdk.java.net/browse/JDK-8152389
> 
> Webrev (test only):
> http://cr.openjdk.java.net/~dfuchs/webrev_8152436/webrev.00/

+1

It’d be nice to convert to TestNG test and could be done in a separate issue.

Mandy

Re: RFR: 8073872: Schemagen fails with StackOverflowError if element references containing class

[Lance Andersen]

Looks OK Alejsej

On Mar 24, 2016, at 7:38 PM, Aleksej Efimov  wrote:

> Hi,
> 
> Please, help to review the fix for JDK-8073872 [1]. In JDK9 this bug was 
> fixed as part of sync with upstream JAXWS project [2]. The changes that fixed 
> stack overflow issue are located in XmlSchemaGenerator class [3].
> JDK9 change that needs review is the addition of regression test for this 
> issue:
> http://cr.openjdk.java.net/~aefimov/8073872/9/00/
> 
> I will be requesting JDK8 approval for partially backporting the fix for this 
> bug to 8u-dev. Can I also ask to review the JDK8 changes:
> http://cr.openjdk.java.net/~aefimov/8073872/8/00/
> 
> Both JDK8/9 changes were tested with JCK/JTREG tests and no failures were 
> observed. New test passes with the fix and successfully reproduces reported 
> stack overflow on JDKs without the fix.
> 
> Best Regards,
> Aleksej
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8073872
> [2] https://bugs.openjdk.java.net/browse/JDK-8150174
> [3] http://hg.openjdk.java.net/jdk9/jdk9/jaxws/rev/ebff1bd3627a#l3.2



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-8031767 Support system or alternative implementations of zlib

[Erik Joelsson]

Looks good to me.

/Erik

On 2016-03-24 23:31, Xueming Shen wrote:

Thanks Erik!

Webrev has been updated accordingly.

http://cr.openjdk.java.net/~sherman/8031767/webrev/

-Sherman

On 03/24/2016 01:57 PM, Erik Joelsson wrote:

Hello,

Yes, I believe we still need to, the reason being that configure 
automatically falls back to bundled if the headers are not available. 
This is what happened when you initially tried your patch in JPRT. I 
still think this is a good way for configure to behave in the general 
case. In the scenario of building our official builds we should 
instead fail fast if the intended configuration is not possible. It's 
true this has not been needed with Macosx so far. It seems zlib 
headers are generally available on that platform. I still think it's 
better to be explicit with these things so that the intention is 
obvious. Solving this kind of problem, of specifying stricter 
requirements for Oracle builds is one of the main features of using Jib.


/Erik

On 2016-03-24 18:21, Xueming Shen wrote:

Erik,

I'm not familiar with the jib-profiles.js. So just want to confirm 
before putting into
the webrev. The proposal is to build with system zlib by default for 
non-windows

platforms, without the need of specifying the configuration/build opton
--with-zlib=system. Do we still need to update this with explicit 
option, as we are

not doing that for macos, which is using the system zlib for a while.

Thanks,
Sherman

On 03/24/2016 06:22 AM, Erik Joelsson wrote:
Hello again. Here is my suggested patch for the jib-profiles.js 
file. This will enforce system zlib for Oracle builds on Linux, 
Solaris and Macosx.


/Erik

diff -r 6da9e0c79eac common/conf/jib-profiles.js
--- a/common/conf/jib-profiles.js
+++ b/common/conf/jib-profiles.js
@@ -241,7 +241,7 @@
 target_os: "linux",
 target_cpu: "x64",
 dependencies: concat(common.dependencies, "devkit"),
-configure_args: common.configure_args,
+configure_args: concat(common.configure_args, 
"--with-zlib=system"),

 make_args: common.make_args
 },

@@ -250,7 +250,8 @@
 target_cpu: "x86",
 build_cpu: "x64",
 dependencies: concat(common.dependencies, "devkit"),
-configure_args: concat(common.configure_args, 
common.configure_args_32bit),
+configure_args: concat(common.configure_args, 
common.configure_args_32bit,

+"--with-zlib=system"),
 make_args: common.make_args
 },

@@ -258,7 +259,7 @@
 target_os: "macosx",
 target_cpu: "x64",
 dependencies: concat(common.dependencies, "devkit"),
-configure_args: common.configure_args,
+configure_args: concat(common.configure_args, 
"--with-zlib=system"),

 make_args: common.make_args
 },

@@ -266,7 +267,7 @@
 target_os: "solaris",
 target_cpu: "x64",
 dependencies: concat(common.dependencies, "devkit", 
"cups"),

-configure_args: common.configure_args,
+configure_args: concat(common.configure_args, 
"--with-zlib=system"),

 make_args: common.make_args
 },

@@ -274,7 +275,7 @@
 target_os: "solaris",
 target_cpu: "sparcv9",
 dependencies: concat(common.dependencies, "devkit", 
"cups"),

-configure_args: common.configure_args,
+configure_args: concat(common.configure_args, 
"--with-zlib=system"),

 make_args: common.make_args
 },


On 2016-03-24 09:05, Erik Joelsson wrote:

Hello,

As I wrote in the bug, jdk9/dev currently fails when using 
--with-zlib=system with the new devkit on Linux. That will need to 
be fixed first.


If the intention of this change is to enforce --with-zlib=system 
on OracleJDK builds, we should also update the Jib profile 
definitions for Linux and Solaris. Configure currently falls back 
to internal if external isn't available. That's a bit too brittle 
for our official builds. Enforcing it from Jib is the best way for 
this kind of change IMO.


Please don't post diffs of internal files on the open list. In 
this case, nothing secret was in the diff but it could easily have 
been. There is no need for posting review on the closed 
generated-configure as long as that's the only thing that changed 
in the closed repo, as long as the open changes causing it are 
properly reviewed.


/Erik

On 2016-03-23 19:37, Xueming Shen wrote:

Hi,

This one was discussed back to Feb, and have been waiting for the 
devkit clearance
from the build-dev, which has just been resolved [1]. So here is 
webrev again.


http://cr.openjdk.java.net/~sherman/8031767/webrev

thanks!
Sherman

[1] https://bugs.openjdk.java.net/browse/JDK-8149545

btw, here is the similar change in the corresponding "closed"
autoconf/generated-configure.sh for convenience.

@@ -5353,11 +5353,11 @@

# Do not change or remov

Re: RFR: 8152733: Avoid creating Manifest when checking for Multi-Release attribute

[Paul Sandoz]

> On 25 Mar 2016, at 17:37, Claes Redestad  wrote:
> 
> Interesting, my only concern is we'd add more code/complexity, but it seems 
> the shift will always be == len when not matching a character in src (the 
> proof is left out as an exercise), so this can be simplified further to:
> 
> byte c = b[i + j];
> if (c >= ' ' && c <= 'z') {
>if (c >= 'a') c -= 32; // Canonicalize
> 
>if (c != src[j]) {
>// no match
>int goodShift = (j < len - 1) ? len : 1;
>int badShift = lastOcc[c - 32];
>i += Math.max(j + 1 - badShift, goodShift);
>continue next;
>}
> } else {
>// no match, character not valid for name
>i += len;
>continue next;
> }
> 
> http://cr.openjdk.java.net/~redestad/8152733/webrev.03/
> 

Nice, that’s quite clear in it’s intent. As long as this meets the performance 
goals i am ok with this being slower than other variants, as i think the 
reduction in static footprint is a fare trade in this case.

Paul.

> (included Steve's suggestion)
> 
> Since this avoids reading from lastOcc for anything but the chars actually 
> matching, this actually seems to improve things a bit locally, so I'm willing 
> to accept the pile-on. I'll run some numbers in the lab...
> 
> /Claes

Re: RFR: 8152733: Avoid creating Manifest when checking for Multi-Release attribute

[Claes Redestad]

On 2016-03-25 16:03, Paul Sandoz wrote:

On 25 Mar 2016, at 13:56, Claes Redestad  wrote:


You've replaced the precomputed good suffix shift which i think makes the Math.max in math a 
bit harder to read. A comment or just move a local for "j < len - 1 ? len : 1" 
would make it a bit easier to read.

It would be good to move the value of MULTIRELEASE_CHARS to its own line as the 
current line is requires scrolling when looking at the side-by-side diffs.

Sure: http://cr.openjdk.java.net/~redestad/8152733/webrev.02/


+1


Thanks!




We could marginally improve the bad shift too:

   int badShift = (c < 128) ? lastOcc[c] : 0;

rather than masking (which might lop off the top bit of bytes of value encoded 
characters), that’s clearer code-wise to me. In fact i bet we could reduce the 
last occurrence array by half if we canonicalize to upper-case characters and 
bound the range to [‘ ‘, ‘Z’].

byte c = b[i + j];
int goodShift = 0;
int badShift = 0;
if (c >= ‘ ‘ && c <= ‘z') {
   if (c >= ‘a’) c -= 32; // Canonicalize

   if (c != src[j]) {
 // no match
 goodShift = (j < len - 1) ? len : 1;
 badShift = lastOcc[c - 32];
   }
} else {
 // no match, character not valid for name
 goodShift = (j < len - 1) ? len : 1;
}
if (goodShift > 0) {
 i += Math.max(j + 1 - badShift, goodShift);
 continue next;
}

Just idle musings on my part, feel free to ignore ‘em.


Interesting, my only concern is we'd add more code/complexity, but it 
seems the shift will always be == len when not matching a character in 
src (the proof is left out as an exercise), so this can be simplified 
further to:


byte c = b[i + j];
if (c >= ' ' && c <= 'z') {
if (c >= 'a') c -= 32; // Canonicalize

if (c != src[j]) {
// no match
int goodShift = (j < len - 1) ? len : 1;
int badShift = lastOcc[c - 32];
i += Math.max(j + 1 - badShift, goodShift);
continue next;
}
} else {
// no match, character not valid for name
i += len;
continue next;
}

http://cr.openjdk.java.net/~redestad/8152733/webrev.03/

(included Steve's suggestion)

Since this avoids reading from lastOcc for anything but the chars 
actually matching, this actually seems to improve things a bit locally, 
so I'm willing to accept the pile-on. I'll run some numbers in the lab...


/Claes


Paul.



Difference from previous for convenience:

diff -r fc748fa355e0 src/java.base/share/classes/java/util/jar/JarFile.java
--- a/src/java.base/share/classes/java/util/jar/JarFile.javaFri Mar 25 
13:38:37 2016 +0100
+++ b/src/java.base/share/classes/java/util/jar/JarFile.javaFri Mar 25 
13:47:31 2016 +0100
@@ -887,11 +887,15 @@
 }

 // Statics for hand-coded Boyer-Moore search
-private static final byte[] CLASSPATH_CHARS = 
{'c','l','a','s','s','-','p','a','t','h', ':', ' '};
+private static final byte[] CLASSPATH_CHARS =
+{'c','l','a','s','s','-','p','a','t','h', ':', ' '};
+
 // The bad character shift for "class-path:"
 private static final byte[] CLASSPATH_LASTOCC;

-private static final byte[] MULTIRELEASE_CHARS = 
{'m','u','l','t','i','-','r','e','l','e', 'a', 's', 'e', ':', ' '};
+private static final byte[] MULTIRELEASE_CHARS =
+{'m','u','l','t','i','-','r','e','l','e', 'a', 's', 'e', ':', ' '};
+
 // The bad character shift for "multi-release: "
 private static final byte[] MULTIRELEASE_LASTOCC;

@@ -959,6 +963,8 @@
 /**
  * Returns true if the pattern {@code src} is found in {@code b}.
  * The {@code lastOcc} array is the precomputed bad character shifts.
+ * Since there are no repeated substrings in our search strings,
+ * the good character shifts can be replaced with a comparison.
  */
 private boolean match(byte[] src, byte[] b, byte[] lastOcc) {
 int len = src.length;
@@ -972,7 +978,8 @@
 c += 32;
 }
 if (c != src[j]) {
-i += Math.max(j + 1 - lastOcc[c&0x7F], j < len - 1 ? len : 
1);
+int goodShift = (j < len - 1) ? len : 1;
+i += Math.max(j + 1 - lastOcc[c&0x7F], goodShift);
 continue next;
 }
 }

Thanks!

/Claes

Re: RFR: 8152733: Avoid creating Manifest when checking for Multi-Release attribute

[Steve Drach]

Hi,

A minor nit on the comment

> + * Since there are no repeated substrings in our search strings,
> + * the good character shifts can be replaced with a comparison.

Probably should be “good suffix shifts”.

Steve

Re: RFR: 8152733: Avoid creating Manifest when checking for Multi-Release attribute

[Paul Sandoz]

> On 25 Mar 2016, at 13:56, Claes Redestad  wrote:
> 
>> 
>> You've replaced the precomputed good suffix shift which i think makes the 
>> Math.max in math a bit harder to read. A comment or just move a local for "j 
>> < len - 1 ? len : 1" would make it a bit easier to read.
>> 
>> It would be good to move the value of MULTIRELEASE_CHARS to its own line as 
>> the current line is requires scrolling when looking at the side-by-side 
>> diffs.
> 
> Sure: http://cr.openjdk.java.net/~redestad/8152733/webrev.02/
> 

+1


We could marginally improve the bad shift too:

  int badShift = (c < 128) ? lastOcc[c] : 0;

rather than masking (which might lop off the top bit of bytes of value encoded 
characters), that’s clearer code-wise to me. In fact i bet we could reduce the 
last occurrence array by half if we canonicalize to upper-case characters and 
bound the range to [‘ ‘, ‘Z’].

byte c = b[i + j];
int goodShift = 0;
int badShift = 0;
if (c >= ‘ ‘ && c <= ‘z') {
  if (c >= ‘a’) c -= 32; // Canonicalize

  if (c != src[j]) {
// no match
goodShift = (j < len - 1) ? len : 1;
badShift = lastOcc[c - 32];
  }
} else {
// no match, character not valid for name
goodShift = (j < len - 1) ? len : 1;
}
if (goodShift > 0) {
i += Math.max(j + 1 - badShift, goodShift);
continue next;
}

Just idle musings on my part, feel free to ignore ‘em.

Paul.


> Difference from previous for convenience:
> 
> diff -r fc748fa355e0 src/java.base/share/classes/java/util/jar/JarFile.java
> --- a/src/java.base/share/classes/java/util/jar/JarFile.javaFri Mar 25 
> 13:38:37 2016 +0100
> +++ b/src/java.base/share/classes/java/util/jar/JarFile.javaFri Mar 25 
> 13:47:31 2016 +0100
> @@ -887,11 +887,15 @@
> }
> 
> // Statics for hand-coded Boyer-Moore search
> -private static final byte[] CLASSPATH_CHARS = 
> {'c','l','a','s','s','-','p','a','t','h', ':', ' '};
> +private static final byte[] CLASSPATH_CHARS =
> +{'c','l','a','s','s','-','p','a','t','h', ':', ' '};
> +
> // The bad character shift for "class-path:"
> private static final byte[] CLASSPATH_LASTOCC;
> 
> -private static final byte[] MULTIRELEASE_CHARS = 
> {'m','u','l','t','i','-','r','e','l','e', 'a', 's', 'e', ':', ' '};
> +private static final byte[] MULTIRELEASE_CHARS =
> +{'m','u','l','t','i','-','r','e','l','e', 'a', 's', 'e', ':', ' 
> '};
> +
> // The bad character shift for "multi-release: "
> private static final byte[] MULTIRELEASE_LASTOCC;
> 
> @@ -959,6 +963,8 @@
> /**
>  * Returns true if the pattern {@code src} is found in {@code b}.
>  * The {@code lastOcc} array is the precomputed bad character shifts.
> + * Since there are no repeated substrings in our search strings,
> + * the good character shifts can be replaced with a comparison.
>  */
> private boolean match(byte[] src, byte[] b, byte[] lastOcc) {
> int len = src.length;
> @@ -972,7 +978,8 @@
> c += 32;
> }
> if (c != src[j]) {
> -i += Math.max(j + 1 - lastOcc[c&0x7F], j < len - 1 ? len 
> : 1);
> +int goodShift = (j < len - 1) ? len : 1;
> +i += Math.max(j + 1 - lastOcc[c&0x7F], goodShift);
> continue next;
> }
> }
> 
> Thanks!
> 
> /Claes

Re: RFR 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present.

[Daniel Fuchs]

On 25/03/16 15:57, Mandy Chung wrote:

Here is the new webrev:
>
>http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.03/index.html
>
>Only changes are in SimpleConsoleLogger.Formatting::getSimpleFormat.

Looks okay.  Minor nit: why not using String::equals?  No need to have a new 
webrev.


Oh - just a matter of taste I guess. Objects.equals will always work :-)
It's not like this code is performance sensitive: it will be called only
once.

Thanks Mandy!

-- daniel

Re: RFR: 8152641: Plugin to generate BMH$Species classes ahead-of-time

[Mandy Chung]

> On Mar 25, 2016, at 7:58 AM, Claes Redestad  wrote:
> 
> Hi,
> 
> On 2016-03-25 11:11, Peter Levart wrote:
>> Hi Claes, Mandy,
>> 
>> On 03/25/2016 02:51 AM, Mandy Chung wrote:
>>> Hi Claes,
>>> 
>>> This is a good interesting work to generate BoundMethodHandle$Species_* 
>>> classes at link time.  This will save the class generation time.  Instead 
>>> of Class.forName, you may want to have a class (similar to SystemModules 
>>> [1]) that will be updated at link time so that BoundMethodHandle can 
>>> reference it statically to determine what species are generated at link 
>>> time and even return statically Class of the generated class.
>> 
>> You may still want to load this classes lazily when needed. Otherwise the 
>> BMH.CLASS_CACHE could simply be pre-populated with name -> Class entries 
>> at appropriate early time.
> 
> I think laziness is a good property here, since it won't penalize modular 
> applications that choose to instruct jlink to pregenerate a lot of BMH 
> classes.
> 
> Perhaps we can allow for static resolve + lazy classload by emitting a switch 
> equivalent to:
> 
> switch (types) {
>  case "LL":
>return Species_LL.class;
>  case "L3":
>return Species_L3.class;
>  ..
>  default:
>return generateBMHSpeciesClass(types);
> }
> 

This is exactly what I was thinking.

Mandy

Re: RFR: 8152641: Plugin to generate BMH$Species classes ahead-of-time

[Claes Redestad]

Hi,

On 2016-03-25 11:11, Peter Levart wrote:

Hi Claes, Mandy,

On 03/25/2016 02:51 AM, Mandy Chung wrote:

Hi Claes,

This is a good interesting work to generate 
BoundMethodHandle$Species_* classes at link time.  This will save the 
class generation time.  Instead of Class.forName, you may want to 
have a class (similar to SystemModules [1]) that will be updated at 
link time so that BoundMethodHandle can reference it statically to 
determine what species are generated at link time and even return 
statically Class of the generated class.


You may still want to load this classes lazily when needed. Otherwise 
the BMH.CLASS_CACHE could simply be pre-populated with name -> 
Class entries at appropriate early time.


I think laziness is a good property here, since it won't penalize 
modular applications that choose to instruct jlink to pregenerate a lot 
of BMH classes.


Perhaps we can allow for static resolve + lazy classload by emitting a 
switch equivalent to:


switch (types) {
  case "LL":
return Species_LL.class;
  case "L3":
return Species_L3.class;
  ..
  default:
return generateBMHSpeciesClass(types);
}





About CNFE, the new Class.forName(Module, String) method does not 
throw CNFE that you could use instead (if not static reference as 
suggested above).  If not found, it will return null.


Thanks, this one had passed under my radar. :-)



This plugin accesses the non-public methods in BoundMethodHandle  and 
has to invoke it via reflection.  With module boundary enforced now, 
maybe we could consider moving to some internal package to share with 
this jlink plugin.


I think there are tests needed to be updated when a new plugin is added.

Mandy
[1] 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/3abd25870915/src/java.base/share/classes/jdk/internal/module/SystemModules.java


One thing to consider is the following: the names of the classes obey 
a particular convention which is now hardcoded in two places: the 
BoundMethodHandle and the GenerateBMHClassesPlugin. You could have a 
method with the following signature in BoundMethodHandle$Factory:


Map.Entry generateConcreteBMHClassBytes(String types)

...which would also return the name of the class derived from the 
types which you would then use to derive the path of the .class file 
in the module of the image. Also, this method - although 
package-private becomes part of the API and that should be written in 
a comment.


What do you think?


Definitely should add a comment about generateConcreteBMHClassBytes 
being an internal API used by the plugin.


I'm open to the idea of moving static utility code to some internal 
package, but want to hear what maintainers of java.lang.invoke think 
about factoring out utility code like this.


Thanks!

/Claes



Regards, Peter



On Mar 24, 2016, at 6:42 AM, Claes Redestad 
 wrote:


Hi,

please review this patch which add an enabled-by-default plugin to 
generate a

configurable list of BoundMethodHandle$Species_*-classes using jlink.

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

This plugin adds the --generate-bmh flag to jlink, which takes a
comma-separated list of shortened species type strings.

As an example: --generate-bmh LL,L3,L4 will generate
BoundMethodHandle$Species_LL, ...$Species_L3 and ...$Species_L4 and add
them to the java.base module.

The Species class lookup in BoundMethodHandle will first check if 
there is a
generated class, otherwise generate it as previously. Adding an 
exceptional
path might seem problematic, but as the code generation is 
magnitudes more
expensive than the exception it doesn't seem fruitful to go to 
lengths to avoid

the CNFE.

More notes along with some results can be found here:

http://cr.openjdk.java.net/~redestad/scratch/bmh_species_gen.txt

Thanks!

/Claes

Re: RFR 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present.

[Mandy Chung]

> On Mar 25, 2016, at 5:12 AM, Daniel Fuchs  wrote:
> 
> On 24/03/16 18:56, Daniel Fuchs wrote:
>>> getSimpleFormat should probably check the given key argument is either
>>> DEFAULT_FORMAT_PROP_KEY or JUL_FORMAT_PROP_KEY; otherwise throws IAE.
>>> Otherwise, this method would pass if key is unexpected key if security
>>> manager is absent but fails if  security manager is present.
>> 
>> Oh - right - good idea. Replace the limited doPrivileged with
>> explicit argument checking :-) Let's do that.
>> 
> 
> Here is the new webrev:
> 
> http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.03/index.html
> 
> Only changes are in SimpleConsoleLogger.Formatting::getSimpleFormat.

Looks okay.  Minor nit: why not using String::equals?  No need to have a new 
webrev.

Mandy

Re: RFR: 8152733: Avoid creating Manifest when checking for Multi-Release attribute

[Claes Redestad]

On 2016-03-25 11:40, Alan Bateman wrote:

On 25/03/2016 01:06, Claes Redestad wrote:

Hi,

the integration of the multi-release jar feature[1] caused a number 
of startup regressions, the primary cause of which was determined to 
be due to parsing manifests into java.util.jar.Manifest only to check 
for the Multi-Release attribute.


This can be avoided by reusing the logic already in place to check 
for the Class-Path attribute.


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

Some additional cleanup was done in this joint effort: Paul suggested 
a few improvements/cleanups to the search code, two anonymous classes 
to check settings could be removed, and - even though JarFile doesn't 
provide any thread-safety guarantees - the checkForSpecialAttributes 
method can be made thread-safe with a synchronized block at little to 
no cost.

This looks quite good.


Thanks!



You've replaced the precomputed good suffix shift which i think makes 
the Math.max in math a bit harder to read. A comment or just move a 
local for "j < len - 1 ? len : 1" would make it a bit easier to read.


It would be good to move the value of MULTIRELEASE_CHARS to its own 
line as the current line is requires scrolling when looking at the 
side-by-side diffs.


Sure: http://cr.openjdk.java.net/~redestad/8152733/webrev.02/

Difference from previous for convenience:

diff -r fc748fa355e0 src/java.base/share/classes/java/util/jar/JarFile.java
--- a/src/java.base/share/classes/java/util/jar/JarFile.javaFri Mar 
25 13:38:37 2016 +0100
+++ b/src/java.base/share/classes/java/util/jar/JarFile.javaFri Mar 
25 13:47:31 2016 +0100

@@ -887,11 +887,15 @@
 }

 // Statics for hand-coded Boyer-Moore search
-private static final byte[] CLASSPATH_CHARS = 
{'c','l','a','s','s','-','p','a','t','h', ':', ' '};

+private static final byte[] CLASSPATH_CHARS =
+{'c','l','a','s','s','-','p','a','t','h', ':', ' '};
+
 // The bad character shift for "class-path:"
 private static final byte[] CLASSPATH_LASTOCC;

-private static final byte[] MULTIRELEASE_CHARS = 
{'m','u','l','t','i','-','r','e','l','e', 'a', 's', 'e', ':', ' '};

+private static final byte[] MULTIRELEASE_CHARS =
+{'m','u','l','t','i','-','r','e','l','e', 'a', 's', 'e', 
':', ' '};

+
 // The bad character shift for "multi-release: "
 private static final byte[] MULTIRELEASE_LASTOCC;

@@ -959,6 +963,8 @@
 /**
  * Returns true if the pattern {@code src} is found in {@code b}.
  * The {@code lastOcc} array is the precomputed bad character shifts.
+ * Since there are no repeated substrings in our search strings,
+ * the good character shifts can be replaced with a comparison.
  */
 private boolean match(byte[] src, byte[] b, byte[] lastOcc) {
 int len = src.length;
@@ -972,7 +978,8 @@
 c += 32;
 }
 if (c != src[j]) {
-i += Math.max(j + 1 - lastOcc[c&0x7F], j < len - 1 
? len : 1);

+int goodShift = (j < len - 1) ? len : 1;
+i += Math.max(j + 1 - lastOcc[c&0x7F], goodShift);
 continue next;
 }
 }

Thanks!

/Claes

Re: JAXP default implementation and JDK-8152063

[David M. Lloyd]

OK, so disappointment again. :)  Thanks for responding; the search 
continues.


On 03/24/2016 06:35 PM, huizhe wang wrote:

Right, that sounds like what I thought you would want: an additional
step in the factory-lookup process to try locating a provider through
the Layer of the caller if TCCL fails.  I think the assumption in the
previous discussion was that a new method would be introduced to take a
Layer as an argument.

-Joe

On 3/24/2016 3:36 PM, David M. Lloyd wrote:

This is all for the case where the user is calling e.g.
javax.xml.stream.XMLInputFactory#newFactory() with no arguments. We
need some way to choose a specific non-JDK default implementation when
there is no service loader info on the TCCL. Using the Layer of the
caller of the newFactory() method would be an ideal solution from our
perspective.

On 03/24/2016 05:18 PM, huizhe wang wrote:

So specifying Layer is preferred solution. If a new method is needed
however, (similar situation to using the method with ClassLoader), that
would mean your users' applications are required to adapt to the new
API. Would you expect that would happen or would you still have existing
applications that can not be updated?

-Joe

On 3/24/2016 2:02 PM, David M. Lloyd wrote:

On 03/24/2016 03:54 PM, huizhe wang wrote:

In this discussion so far,

a) I see that you seemed to have successfully used the method with a
class loader as Daniel suggested.  I assume that solved the issue
reported in JDK-8152063. Am I right, that you no longer have issue
with
using a proxy? Or


No, not solved yet, just in the process of prototyping but ran into a
road block with XMLReaderFactory.


b) you do need JAXP's supporting using a Layer as the context in order
to solve your issue completely?


I think this should be considered the best solution to the problem.


c) On org.xml.sax.helpers.XMLReaderFactory, as Alan and Daniel
said, yes
I'm working with the SAX project to hopefully get a new release out
that
would conform to the ServiceLoader spec.  I'm also suggesting a new
method that takes a ClassLoader that would be useful if (a) worked
for you.


We have some more testing to do before I can say whether this works or
does not work.  But either way it is a rather non-optimal solution,
and if I can avoid using a proxy layer, I would much prefer to do so.

Thanks for your response.









--
- DML

Re: RFR 8150840: Add an internal system property to control the default level of System.Logger when java.logging is not present.

[Daniel Fuchs]

On 24/03/16 18:56, Daniel Fuchs wrote:

getSimpleFormat should probably check the given key argument is either
DEFAULT_FORMAT_PROP_KEY or JUL_FORMAT_PROP_KEY; otherwise throws IAE.
Otherwise, this method would pass if key is unexpected key if security
manager is absent but fails if  security manager is present.


Oh - right - good idea. Replace the limited doPrivileged with
explicit argument checking :-) Let's do that.



Here is the new webrev:

http://cr.openjdk.java.net/~dfuchs/webrev_8150840/webrev.03/index.html

Only changes are in SimpleConsoleLogger.Formatting::getSimpleFormat.

best regards,

-- daniel

Re: RFR: 8152733: Avoid creating Manifest when checking for Multi-Release attribute

[Paul Sandoz]

> On 25 Mar 2016, at 02:06, Claes Redestad  wrote:
> 
> Hi,
> 
> the integration of the multi-release jar feature[1] caused a number of 
> startup regressions, the primary cause of which was determined to be due to 
> parsing manifests into java.util.jar.Manifest only to check for the 
> Multi-Release attribute.
> 
> This can be avoided by reusing the logic already in place to check for the 
> Class-Path attribute.
> 
> Bug:  https://bugs.openjdk.java.net/browse/JDK-8152733
> Webrev: http://cr.openjdk.java.net/~redestad/8152733/webrev.01/
> 
> Some additional cleanup was done in this joint effort: Paul suggested a few 
> improvements/cleanups to the search code, two anonymous classes to check 
> settings could be removed, and - even though JarFile doesn't provide any 
> thread-safety guarantees - the checkForSpecialAttributes method can be made 
> thread-safe with a synchronized block at little to no cost.
> 

This looks good. Thanks for noticing the existing code for class-path parsing.

I agree with Alan, a comment as to why the good suffix shift can be reduced to 
a comparison would be helpful.

Paul.

> Thanks!
> 
> /Claes
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8132734

Re: JDK 9 RFR of JDK-8152749: Mark AdaptorCloseAndInterrupt.java and WatchService/Basic.java as intermittently failing

[Alan Bateman]

On 25/03/2016 11:12, Amy Lu wrote:


Please review the updated webrev.

MayFlies.java (which tracked by the same bug as Basic.java) now 
problem listed.


bugs:
https://bugs.openjdk.java.net/browse/JDK-8152749
https://bugs.openjdk.java.net/browse/JDK-8152755

webrev:
http://cr.openjdk.java.net/~amlu/8152749-8152755/webrev.01/

Looks fine.

Re: JDK 9 RFR of JDK-8152749: Mark AdaptorCloseAndInterrupt.java and WatchService/Basic.java as intermittently failing

[Amy Lu]

On 3/25/16 6:49 PM, Alan Bateman wrote:



On 25/03/2016 08:52, Amy Lu wrote:

Below nio tests are known to fail intermittently:

java/nio/channels/etc/AdaptorCloseAndInterrupt.java (JDK-8136899, 
JDK-8150811)

java/nio/file/WatchService/Basic.java (JDK-7158947)
This looks okay, alternative put JDK-7158947 on the exclude list until 
we get a Solaris patch for this.


-Alan.


Please review the updated webrev.

MayFlies.java (which tracked by the same bug as Basic.java) now problem 
listed.


bugs:
https://bugs.openjdk.java.net/browse/JDK-8152749
https://bugs.openjdk.java.net/browse/JDK-8152755

webrev:
http://cr.openjdk.java.net/~amlu/8152749-8152755/webrev.01/

Thanks,
Amy

Re: JDK 9 RFR of JDK-8152749: Mark AdaptorCloseAndInterrupt.java and WatchService/Basic.java as intermittently failing

[Alan Bateman]

On 25/03/2016 08:52, Amy Lu wrote:

Below nio tests are known to fail intermittently:

java/nio/channels/etc/AdaptorCloseAndInterrupt.java (JDK-8136899, 
JDK-8150811)

java/nio/file/WatchService/Basic.java (JDK-7158947)
This looks okay, alternative put JDK-7158947 on the exclude list until 
we get a Solaris patch for this.


-Alan.

Re: RFR: 8152733: Avoid creating Manifest when checking for Multi-Release attribute

[Alan Bateman]

On 25/03/2016 01:06, Claes Redestad wrote:

Hi,

the integration of the multi-release jar feature[1] caused a number of 
startup regressions, the primary cause of which was determined to be 
due to parsing manifests into java.util.jar.Manifest only to check for 
the Multi-Release attribute.


This can be avoided by reusing the logic already in place to check for 
the Class-Path attribute.


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

Some additional cleanup was done in this joint effort: Paul suggested 
a few improvements/cleanups to the search code, two anonymous classes 
to check settings could be removed, and - even though JarFile doesn't 
provide any thread-safety guarantees - the checkForSpecialAttributes 
method can be made thread-safe with a synchronized block at little to 
no cost.

This looks quite good.

You've replaced the precomputed good suffix shift which i think makes 
the Math.max in math a bit harder to read. A comment or just move a 
local for "j < len - 1 ? len : 1" would make it a bit easier to read.


It would be good to move the value of MULTIRELEASE_CHARS to its own line 
as the current line is requires scrolling when looking at the 
side-by-side diffs.


-Alan

Re: RFR: 8152641: Plugin to generate BMH$Species classes ahead-of-time

[Peter Levart]

Hi Claes, Mandy,

On 03/25/2016 02:51 AM, Mandy Chung wrote:

Hi Claes,

This is a good interesting work to generate BoundMethodHandle$Species_* classes at 
link time.  This will save the class generation time.  Instead of Class.forName, you 
may want to have a class (similar to SystemModules [1]) that will be updated at link 
time so that BoundMethodHandle can reference it statically to determine what species 
are generated at link time and even return statically Class of the generated 
class.


You may still want to load this classes lazily when needed. Otherwise 
the BMH.CLASS_CACHE could simply be pre-populated with name -> Class 
entries at appropriate early time.




About CNFE, the new Class.forName(Module, String) method does not throw CNFE 
that you could use instead (if not static reference as suggested above).  If 
not found, it will return null.

This plugin accesses the non-public methods in BoundMethodHandle  and has to 
invoke it via reflection.  With module boundary enforced now, maybe we could 
consider moving to some internal package to share with this jlink plugin.

I think there are tests needed to be updated when a new plugin is added.

Mandy
[1] 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/3abd25870915/src/java.base/share/classes/jdk/internal/module/SystemModules.java


One thing to consider is the following: the names of the classes obey a 
particular convention which is now hardcoded in two places: the 
BoundMethodHandle and the GenerateBMHClassesPlugin. You could have a 
method with the following signature in BoundMethodHandle$Factory:


Map.Entry generateConcreteBMHClassBytes(String types)

...which would also return the name of the class derived from the types 
which you would then use to derive the path of the .class file in the 
module of the image. Also, this method - although package-private 
becomes part of the API and that should be written in a comment.


What do you think?

Regards, Peter




On Mar 24, 2016, at 6:42 AM, Claes Redestad  wrote:

Hi,

please review this patch which add an enabled-by-default plugin to generate a
configurable list of BoundMethodHandle$Species_*-classes using jlink.

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

This plugin adds the --generate-bmh flag to jlink, which takes a
comma-separated list of shortened species type strings.

As an example: --generate-bmh LL,L3,L4 will generate
BoundMethodHandle$Species_LL, ...$Species_L3 and ...$Species_L4 and add
them to the java.base module.

The Species class lookup in BoundMethodHandle will first check if there is a
generated class, otherwise generate it as previously. Adding an exceptional
path might seem problematic, but as the code generation is magnitudes more
expensive than the exception it doesn't seem fruitful to go to lengths to avoid
the CNFE.

More notes along with some results can be found here:

http://cr.openjdk.java.net/~redestad/scratch/bmh_species_gen.txt

Thanks!

/Claes

JDK 9 RFR of JDK-8152749: Mark AdaptorCloseAndInterrupt.java and WatchService/Basic.java as intermittently failing

[Amy Lu]

Below nio tests are known to fail intermittently:

java/nio/channels/etc/AdaptorCloseAndInterrupt.java (JDK-8136899, 
JDK-8150811)

java/nio/file/WatchService/Basic.java (JDK-7158947)

This patch is to mark the test accordingly with keyword 'intermittent'.

bug: https://bugs.openjdk.java.net/browse/JDK-8152749
webrev: http://cr.openjdk.java.net/~amlu/8152749/webrev.00/

Thanks,
Amy

--- old/test/java/nio/channels/etc/AdaptorCloseAndInterrupt.java
2016-03-25 16:46:48.0 +0800
+++ new/test/java/nio/channels/etc/AdaptorCloseAndInterrupt.java
2016-03-25 16:46:48.0 +0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 2016, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -24,7 +24,7 @@
 /* @test
  * @bug 7184932
  * @summary Test asynchronous close and interrupt of timed socket adapter 
methods
- * @key randomness
+ * @key randomness intermittent
  */
 
 import java.io.*;

--- old/test/java/nio/file/WatchService/Basic.java  2016-03-25 
16:46:49.0 +0800
+++ new/test/java/nio/file/WatchService/Basic.java  2016-03-25 
16:46:49.0 +0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2008, 2016, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,6 +23,7 @@
 
 /* @test

  * @bug 4313887 6838333 7017446 8011537 8042470
+ * @key intermittent
  * @summary Unit test for java.nio.file.WatchService
  * @library ..
  * @run main Basic