RFR 8071588: The spec for javax.script.ScriptEngineFactory.getProgram() should specify NPEs thrown

[Sundararajan Athijegannathan]

Please review.

Bug: https://bugs.openjdk.java.net/browse/JDK-8071588

jdk webrev: http://cr.openjdk.java.net/~sundar/8071588/jdk/webrev.00/

nashorn webrev:
http://cr.openjdk.java.net/~sundar/8071588/nashorn/webrev.00/

Thanks

-Sundar

Re: RFR:JDK-8075205 java/net/URLClassLoader/closetest/CloseTest.java and GetResourceAsStream.java failed with existing dir

[Srinivasan Raghavan]

Thanks for the review.
> On 18-Oct-2016, at 4:16 PM, Chris Hegarty  wrote:
> 
> 
>> On 17 Oct 2016, at 09:51, Srinivasan Raghavan 
>>  wrote:
>> 
>> Hi all
>> 
>> Please review the fix for the bug 
>> 
>> Bug :https://bugs.openjdk.java.net/browse/JDK-8075205
>> 
>> The tests uses classes directory for the output files. This can result in 
>> the files being left over after the test is complete which can result in 
>> instability. The tests copies the files to be compiled form test src to test 
>> classes which can result in copy of permission and result in instability 
>> because the test has delete operations. The test fails randomly mostly in 
>> copy or delete operation. I propose the test to be refactored to make the 
>> use scratch directory as its output directory and eliminate shell by using 
>> testlibrary utils.
>> 
>> fix : http://cr.openjdk.java.net/~sraghavan/8075205/webrev.00/
> 
> This looks good to me. Thanks Srinivasan.
> 
> -Chris.

Re: RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

On Tue, Oct 18, 2016 at 6:33 PM, Vitaly Davidovich 
wrote:

>
> Indeed - the whole inlining based on simple bytecode size is a big problem
> (I've brought this up in various contexts several times on the compiler
> list, but this is a known issue).  But, some of the methods using the jsr
> style look to already be >35 bytes.  I've seen this style in other places
> in the JDK where the method is definitely above 35.
>

- 35 is not a universal physical constant!
- maybe the same style is used in a similar method where it did cross the
35 byte threshold
- elsewhere in jsr166 we deal with concurrency, where copying fields into
locals is a good reflex for correctness

Re: JDK 9 org.omg.CORBA.ORBSingletonClass loading will not use context class loader

[Tom Hood]

It's unclear if there really is an interop issue.   The application will
launch with 7u55 if I don't set the ORBSingletonClass property, although
that isn't how visibroker 5.2.1 was intended to run, so not setting it
worries me.

Our application does call ORB.init(args,props) once at startup and use that
instance throughout the code we have written.

However, visibroker calls ORB.init() all over the idl generated code and
the vb jar itself.

The javap of the vb jar does show some downcasts to its
com.inprise.vbroker.orb.ORBSingleton implementation in a few classes, but I
haven't tracked down if that code could execute in our application or if
the object it tries to downcast originated from ORB.init().  The simple
test of a basic launch and performing some activities within the
application seem to work.  Sadly we don't have any automated tests that
exercise our use of CORBA (java client to/from c++ server).

The idea of mixing ORB implementations worries me, even if only for the
singleton ORB.  I'm new to CORBA and the client side of our application and
still a newbie to java as I've been primarily working on our c++ server.

Do the reasons for the JDK change to ORB.init() apply in our use case?
There's only one application/context and ORB vendor implementation running
in the jvm in production.  Do you think approach #1 might be a reasonably
safe workaround without the risk of hidden interop issues?


On Tue, Oct 18, 2016 at 1:30 PM, Alan Bateman 
wrote:

> On 18/10/2016 19:57, Tom Hood wrote:
>
> Hello,
>>
>> We have a Java Webstart application that uses CORBA and requires an old
>> version of the Visibroker ORB (5.2.1) that will not launch with Java 9 due
>> to its inclusion of a change originally added to 7u55 and later backed
>> out:
>> Bug ID: JDK-8042789 org.omg.CORBA.ORBSingletonClass loading no
>> longer uses context class loader
>> 
>>
>> What approaches should we consider for our application to support Java 9 ?
>>
>> The singleton ORB is the system-wide type code factory so having it be
> located via the TCCL is highly problematic (many reasons including
> security, memory leaks, and of course it's not going to work then there two
> or more applications/contexts in the same VM trying to do the same thing).
>
> I assume your application can use the 2-arg ORB.init to create the ORB, in
> which case the implementation will be located via the TCCL (so you can
> bundle the ORB implementation with the application). From your mail then it
> sounds like the issue is that something in Visibroker is using the zero-arg
> ORB.init and so gets the JDK ORB as the singleton ORB. Does this lead to an
> interop issue or does Visibroker assuming the singleton ORB is its own type?
>
> -Alan
>

Re: RFR: jsr166 jdk9 integration wave 12

[Vitaly Davidovich]

On Tuesday, October 18, 2016, Martin Buchholz  wrote:

>
>
> On Tue, Oct 18, 2016 at 4:26 PM, Vitaly Davidovich  > wrote:
>
>> "jsr166 style" - makes it easy on javac and the JIT, if not for humans.
>>>
>> In some of the cases here, I'd consider it a significant JIT failure if
>> the "jsr166 style" makes a difference (I'm not sure how this makes a
>> difference to javac - did you mean interpreter?).  Is this jsr style a
>> carryover from long ago? Maybe it needs to be reconsidered.  The code
>> clarity isn't terrible, but it would be unfortunate to do this in simple
>> cases where the compiler ought to handle it just fine - does it not?
>>
>
> I consider it a significant failure that
>
>1. JDK-6445664 
>
> Eliminate remaining performance penalty for using assert
>
> still hasn't been fixed.
>
Indeed - the whole inlining based on simple bytecode size is a big problem
(I've brought this up in various contexts several times on the compiler
list, but this is a known issue).  But, some of the methods using the jsr
style look to already be >35 bytes.  I've seen this style in other places
in the JDK where the method is definitely above 35.  My thinking was this
must be some manual attempt to force the compiler to put the value in a
register or stack slot, for fear that it wouldn't do it on its own because
it can't prove it's safe, rather than inlining.

Anyway, the style is odd, perhaps outdated to some degree, but not that big
of a deal in the grand scheme of things (IMO).

I mostly wanted to know of impact on ArrayDeque performance due to the code
restructuring.


-- 
Sent from my phone

Re: RFR: 8163984: Fix license and copyright headers in jdk9 under test/lib

[David Holmes]

Hi Stanislav,

On 19/10/2016 1:06 AM, Stanislav Smirnov wrote:

Hi,

I'm still looking for volunteers to review


This is trivial. Reviewed. I will push for you, to jdk9/dev.

Thanks,
David


Best regards,
Stanislav Smirnov






On 05 Oct 2016, at 19:44, Stanislav Smirnov  
wrote:

Hi,

Please review this fix for JDK-8163984 
.
This one is similar to another one, I have sent earlier.
Legal notices and Oracle copyrights were updated (white and blank space, 
commas) in tests files under test/lib for uniformity to meet Oracle 
requirements.

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

Webrev: http://cr.openjdk.java.net/~stsmirno/8163984/webrev.00/ 

Best regards,
Stanislav Smirnov

Re: RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

On Tue, Oct 18, 2016 at 4:26 PM, Vitaly Davidovich 
wrote:

> "jsr166 style" - makes it easy on javac and the JIT, if not for humans.
>>
> In some of the cases here, I'd consider it a significant JIT failure if
> the "jsr166 style" makes a difference (I'm not sure how this makes a
> difference to javac - did you mean interpreter?).  Is this jsr style a
> carryover from long ago? Maybe it needs to be reconsidered.  The code
> clarity isn't terrible, but it would be unfortunate to do this in simple
> cases where the compiler ought to handle it just fine - does it not?
>

I consider it a significant failure that

   1. JDK-6445664 

Eliminate remaining performance penalty for using assert

still hasn't been fixed.

Re: RFR: jsr166 jdk9 integration wave 12

[Vitaly Davidovich]

On Tuesday, October 18, 2016, Martin Buchholz  wrote:

>
>
> On Tue, Oct 18, 2016 at 4:00 PM, Vitaly Davidovich  > wrote:
>
>>
>>> > * Change in allocation/capacity policy.
>>> >
>>> > The removal of the power-of-two restriction, and applying a 1.5x growth
>>> > factor (same as ArrayList) seems sensible. Does this mean that the
>>> ability
>>> > to compute the proper array index by using x & (length-1) wasn't worth
>>> it?
>>> > Or at least not worth the extra tail wastage?
>>> >
>>>
>>> There's no integer division to optimize, as with hash tables.
>>
>> But it does add some new branches, doesn't it? Potentially branches that
>> aren't taken prior to JIT compilation, but taken later = deopt.
>>
>
> If it's a smidgeon slower, I don't care that much; improvement in
> flexibility and scalability are more important.
>
Well, others may :).  I'm not saying it's slower, I don't know, but you're
dismissing that possibility a bit too quickly, methinks.

>
> Of course, I do care about algorithmic improvements to e.g. removeIf
>
Sure, no debate.

>
>
>> Curious if you ran some benchmarks on ArrayDeque.
>>
>
> Not yet.  Who would like to show how slow the code is?
>
>
>> Also, some odd stylistic things, such as:
>>
>> while (s == (capacity = (elements = this.elements).length))
>>
>> Is this an attempt to help the JIT or something?
>>
>> "jsr166 style" - makes it easy on javac and the JIT, if not for humans.
>
In some of the cases here, I'd consider it a significant JIT failure if the
"jsr166 style" makes a difference (I'm not sure how this makes a difference
to javac - did you mean interpreter?).  Is this jsr style a carryover from
long ago? Maybe it needs to be reconsidered.  The code clarity isn't
terrible, but it would be unfortunate to do this in simple cases where the
compiler ought to handle it just fine - does it not?


-- 
Sent from my phone

Re: RFR: jsr166 jdk9 integration wave 12

[Vitaly Davidovich]

On Tuesday, October 18, 2016, Martin Buchholz  wrote:

> On Tue, Oct 18, 2016 at 10:15 AM, Stuart Marks  >
> wrote:
>
> >
> >
> > On 10/17/16 6:34 PM, Martin Buchholz wrote:
> >
> >> Most of this is for Stuart - very collection-y.
> >>
> >> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-
> >> jdk9-integration/
> >>
> >> This includes a rewrite of ArrayDeque to bring it to parity with
> ArrayList
> >> (except for List features).
> >> The patch includes public methods ensureCapacity, trimToSize, replaceAll
> >> as in
> >> ArrayList, but we can defer making them public to a later change (or a
> >> later
> >> release), since new public methods are always controversial.  But I'd
> >> really
> >> like to get the performance/scalability changes in for jdk 9.
> >>
> >> It also includes a redo of ArrayList#removeIf to make it more efficient
> >> and
> >> consistent with behavior of other implementations, including ArrayDeque.
> >>
> >> The patches are a little tangled because they step on each other's toes.
> >> File
> >> CollectionTest is in "miscellaneous", but it tests both the ArrayDeque
> and
> >> ArrayList changes.
> >>
> >
> > Hi Martin,
> >
> > ArrayList.removeIf() is nice. I like the way it recovers from the
> > predicate having thrown an exception. I note that it uselessly copies the
> > tail of the array to itself, if the predicate throws an exception and
> > nothing has been deleted yet. You could add a r != w check, or possibly
> > deleted > 0 if you prefer, or maybe we don't care because this is a rare
> > (we hope) error recovery case.
> >
> >
> I had the same thoughts... I added checks for deleted > 0
>
>
> > ***
> >
> > I have some comments on ArrayDeque:
> >
> > * Change in allocation/capacity policy.
> >
> > The removal of the power-of-two restriction, and applying a 1.5x growth
> > factor (same as ArrayList) seems sensible. Does this mean that the
> ability
> > to compute the proper array index by using x & (length-1) wasn't worth
> it?
> > Or at least not worth the extra tail wastage?
> >
>
> There's no integer division to optimize, as with hash tables.

But it does add some new branches, doesn't it? Potentially branches that
aren't taken prior to JIT compilation, but taken later = deopt.

Curious if you ran some benchmarks on ArrayDeque.

Also, some odd stylistic things, such as:

while (s == (capacity = (elements = this.elements).length))

Is this an attempt to help the JIT or something?


> It was the horror of user discovering that ArrayDeque(2^n) allocated
> 2^(n+1) that persuaded me to go down this road.
> It's also very useful to allocate exactly the requested size on
> ArrayDeque(m) or be precise with trimToSize or allow growth up to (almost)
> Integer.MAX_VALUE.
> A circular buffer is a very simple data structure - our implementation
> should be a model!
>
>
> > (Potential future enhancement: allocate the array lazily, like ArrayList)
> >
> > Note, I haven't digested all the changes yet.
> >
> > * API additions
> >
> > I'm somewhat undecided about these.
> >
> > I've always felt that the ensureCapacity() and trimToSize() methods were
> a
> > sop added to ArrayList aimed at people converting from Vector. The
> > ArrayList.ensureCapacity() method seems to be used a lot, but probably
> not
> > to great effect, since addAll() gives exact sizing anyway. The
> trimToSize()
> > method could be useful in some cases, but should we hold out for a
> > generalized one, as requested by JDK-4619094?
> >
>
> ensureCapacity is the least useful - just saving less than a factor of 2
> allocation on growth.  trimToSize seems more important, since it saves
> memory "permanently".
>
>
> > On the other hand, ArrayDeque is modeling an array, so providing some
> size
> > control seems mostly harmless.
> >
> > The replaceAll() method is a bit oddly placed here. It's a default method
> > on List (which ArrayDeque doesn't, and can't implement) so it's a bit
> > strange to have a special method here with the same name and semantics as
> > the one on List, but with a "fork" of the specification.
> >
> >
> yeah, it's adding a List method even though the mega-feature of List view
> is deferred.
>
>
> > Maybe if JDK-8143850 is implemented to give a list view of an ArrayDeque,
> > the replaceAll() method could be implemented on that:
> >
> > arrayDeque.asList().replaceAll(clazz::method);
> >
>
> yeah, the idea might be that we add List methods like get(i) and replaceAll
> directly on ArrayDeque, but only when you call asList do you get something
> that implements List ... which is ... odd.
>
>
> ***
> >
> > I'm not sure what to think of the arrangement of the tests. The "core"
> > collections, that is, collections in java.util, exclusive of
> > java.util.concurrent, are mostly tested by tests in jdk/test/java/util.
> > This patch adds tests for ArrayList and ArrayDeque inside of
> > jdk/test/java/util/concurrent/tck. There's a test group
> > 

Re: RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

Latest webrev defers potential new methods:

   /* TODO: public */ private void trimToSize()

Re: RFR: jsr166 jdk9 integration wave 12

[Paul Sandoz]

> On 18 Oct 2016, at 12:07, Martin Buchholz  wrote:
> 
> 
> 
> On Tue, Oct 18, 2016 at 10:55 AM, Paul Sandoz  wrote:
> 
> Testing-wise there is always gonna be some overlap. It would be nice to 
> consolidate, although arguably in principle TCK has a slightly different 
> focus. Now that the tck is in the OpenJDK repo perhaps we should add that to 
> the jdk_collections_core?
> 

Ah, i see they are already implicitly included under the jdk_concurrent group.

# All collections, core and concurrent
jdk_collections = \
:jdk_collections_core \
:jdk_concurrent

# java.util.concurrent
# Includes concurrent collections plus other stuff
# Maintained by JSR-166 EG (Doug Lea et al)
jdk_concurrent = \
java/util/concurrent

Hmm… not sure i have a strong opinion here regarding the removal of the core 
group. I think the comment should be updated to state tck tests are included.


> I don't think the distinction between  jdk_collections_core and 
> jdk_collections is useful.  Too many tests test both kinds of collections, 
> and there's  inheritance.  Just get rid of jdk_collections_core, and add:
> 
> test/java/util/Spliterator

Yes, that’s useful to add to the collection group.


> test/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorsTest.java

The above tests j.u.s.Collector implementations rather than collections 
themselves (even thought it produces collections), so i think it should not be 
added.


> test/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectionAndMapModifyStreamTest.java

Ok.

Paul.

Re: JDK 9 org.omg.CORBA.ORBSingletonClass loading will not use context class loader

[Alan Bateman]

On 18/10/2016 19:57, Tom Hood wrote:


Hello,

We have a Java Webstart application that uses CORBA and requires an old
version of the Visibroker ORB (5.2.1) that will not launch with Java 9 due
to its inclusion of a change originally added to 7u55 and later backed out:
Bug ID: JDK-8042789 org.omg.CORBA.ORBSingletonClass loading no
longer uses context class loader


What approaches should we consider for our application to support Java 9 ?

The singleton ORB is the system-wide type code factory so having it be 
located via the TCCL is highly problematic (many reasons including 
security, memory leaks, and of course it's not going to work then there 
two or more applications/contexts in the same VM trying to do the same 
thing).


I assume your application can use the 2-arg ORB.init to create the ORB, 
in which case the implementation will be located via the TCCL (so you 
can bundle the ORB implementation with the application). From your mail 
then it sounds like the issue is that something in Visibroker is using 
the zero-arg ORB.init and so gets the JDK ORB as the singleton ORB. Does 
this lead to an interop issue or does Visibroker assuming the singleton 
ORB is its own type?


-Alan

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

[joe darcy]

Hello,


On 10/12/2016 8:50 AM, Roger Riggs wrote:

Hi Chris,


On 10/10/2016 9:43 AM, Chris Hegarty wrote:

Roger,


[snip]


Right, I was a little uneasy with this too, to being with, but
it has grown on me ( since it appears stable and reliable in
all my builds and tests ). Also the surface area of the code
change is very small, and the inherit channel mechanism is well
specified and stable.


ok, lets give it a try.




I agree it is time to give another approach to these tests a try. This 
set of tests muddies our overall test results with intermittent 
failures. I suggest giving Chris's approach a try, monitoring any 
changes in the failure rate of the tests for a month or two, and then 
reassessing if the tests need further revisiting.


Thanks,

-Joe

Re: RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

On Tue, Oct 18, 2016 at 10:55 AM, Paul Sandoz 
wrote:

>
> Testing-wise there is always gonna be some overlap. It would be nice to
> consolidate, although arguably in principle TCK has a slightly different
> focus. Now that the tck is in the OpenJDK repo perhaps we should add that
> to the jdk_collections_core?
>

I don't think the distinction between  jdk_collections_core and
jdk_collections is useful.  Too many tests test both kinds of collections,
and there's  inheritance.  Just get rid of jdk_collections_core, and add:

test/java/util/Spliterator
test/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectorsTest.java
test/java/util/stream/test/org/openjdk/tests/java/util/stream/CollectionAndMapModifyStreamTest.java

JDK 9 org.omg.CORBA.ORBSingletonClass loading will not use context class loader

[Tom Hood]

Hello,

We have a Java Webstart application that uses CORBA and requires an old
version of the Visibroker ORB (5.2.1) that will not launch with Java 9 due
to its inclusion of a change originally added to 7u55 and later backed out:
   Bug ID: JDK-8042789 org.omg.CORBA.ORBSingletonClass loading no
longer uses context class loader


What approaches should we consider for our application to support Java 9 ?

Constraints we must satisfy:

   - cannot upgrade visibroker
   - continue to support clients with java versions 1.6+

Constraints we are trying to satisfy:

   - avoid using different vendor ORB
   - avoid replacing CORBA
   - avoid modifying user's machine configuration (e.g. CLASSPATH) or jre
   installation with application-specifics (i.e. java.security, java.policy,
   etc.)

The two properties we set in the jnlp to specify the visibroker orb classes
are:



The two approaches I'm considering so far are:

   1. add vb jar to system class loader at start of application using
   URLClassLoader.addURL
   2. edit vb jar to avoid calling org.omg.CORBA.ORB.init()


*Approach #1: add vb jar to system class loader at start of application*

This works with 7u55.  The application will launch as long as I disable the
security manager (System.setSecurityManager(null)) or use a policy file
entry that explicitly grants AllPermission to the vb jar.  This is
necessary even though our jnlp file has always specified
.  To add the vb jar to the system
class loader, it appears necessary to call the protected
URLClassLoader.addURL method via reflection.

Questions I have about Approach #1:

   1. Are there additional security concerns with disabling the security
   manager at the start of main instead of granting AllPermission via policy
   files?  Our goal is for our application to continue to have the same access
   the user has to the user's machine (mostly windows).  The users do not
   have/run our application with administrator/root privileges.
   2. Is there a better way to make the vb jar visible to the system class
   loader when the application starts without calling a protected addURL
   method?


*Approach #2: edit vb jar to avoid calling org.omg.CORBA.ORB.init()*

I haven't gone down this path too far other than attempt to identify edits
that would be needed.  The source is unavailable (javap and
http://asm.ow2.org have been helpful).  Here's the list of possible edits I
have so far:

   1. Replace calls to org.omg.CORBA.ORB.init() with a call to our own
   class such as AppORB.init() that always returns the vb ORBSingleton instance
   2. Replace any object that is being downcast to the vb ORBSingleton
   (and/or its subclasses, if any) with a call to AppORB.init()
   3. Check if vb jar calls methods in the jre which then call ORB.init().
   If such calls exist, then I think I may need to replace these calls with
   something equivalent, because otherwise the default jre provided singleton
   will be used (com.sun.corba.se.impl.orb.ORBSingleton).  Maybe it's okay to
   mix the two Singleton ORB implementations in the same jvm, but not knowing
   enough about CORBA at this point it seems like a potential problem.  The
   javadoc does say it is a restricted implementation allowing use as a
   TypeCode factory only, but I'm not sure yet if it is possible to mix and
   match TypeCodes from the two ORB implementations.  Feels a little dicey to
   me.


Please let me know if you can think of any alternate approaches to consider
or potential issues/solutions to the ones outlined above.

Thank you,
-- Tom

P.S. I also posted this question at
https://community.oracle.com/message/14072662#14072662

RFR 8163553 java.lang.LinkageError from test java/lang/ThreadGroup/Stop.java

[Paul Sandoz]

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8163553-vh-mh-link-errors-not-wrapped/webrev/

This is the issue that motivated a change in the behaviour of indy wrapping 
Errors in BootstrapMethodError, JDK-8166974. I plan to push this issue with 
JDK-8166974 to hs, since they are related in behaviour even though there is no 
direct dependency between the patches.


When invoking signature-polymorphic methods a similar but hardcoded dance 
occurs, with an appeal to Java code, to link the call site.

- MethodHandle.invoke/invokeExact (and the VH methods) would wrap all Errors in 
LinkageError. Now they are passed through, thus an Error like ThreadDeath is 
not wrapped.

- MethodHandle.invoke/invokeExact/invokeBasic throw Throwable, and in certain 
cases the Throwable is wrapped in an InternalError. In many other cases Error 
and RuntimeException are propagated, which i think in general is the right 
pattern, so i consistently applied that.

- I updated StringConcatFactory to also pass through Errors and avoid unduly 
wrapping StringConcatException in another instance of StringConcatException. 
(LambdaMetafactory and associated classes required no changes.)

Thanks,
Paul.

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

[Peter Levart]

Hi Mandy,


On 10/18/2016 07:36 PM, Mandy Chung wrote:

On Oct 18, 2016, at 3:55 AM, Peter Levart  wrote:

They do. What do you say about this variant (compressionLevel=9):

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

+1.  Ship it!  Thanks for compressing it!

Mandy



Thanks for reviewing. Pushed.

Hope it's not too bad that I managed to enter non-ascii characters into 
commit message. I was copy-pasting from Jira issue title and didn't 
notice the difference between ' and ’ . Probably an artifact of 
migrating issues from SUN's old bug-tracking system. This issue really 
had a long beard...


Regards, Peter

Re: RFR: jsr166 jdk9 integration wave 12

[Martin Buchholz]

On Tue, Oct 18, 2016 at 10:15 AM, Stuart Marks 
wrote:

>
>
> On 10/17/16 6:34 PM, Martin Buchholz wrote:
>
>> Most of this is for Stuart - very collection-y.
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-
>> jdk9-integration/
>>
>> This includes a rewrite of ArrayDeque to bring it to parity with ArrayList
>> (except for List features).
>> The patch includes public methods ensureCapacity, trimToSize, replaceAll
>> as in
>> ArrayList, but we can defer making them public to a later change (or a
>> later
>> release), since new public methods are always controversial.  But I'd
>> really
>> like to get the performance/scalability changes in for jdk 9.
>>
>> It also includes a redo of ArrayList#removeIf to make it more efficient
>> and
>> consistent with behavior of other implementations, including ArrayDeque.
>>
>> The patches are a little tangled because they step on each other's toes.
>> File
>> CollectionTest is in "miscellaneous", but it tests both the ArrayDeque and
>> ArrayList changes.
>>
>
> Hi Martin,
>
> ArrayList.removeIf() is nice. I like the way it recovers from the
> predicate having thrown an exception. I note that it uselessly copies the
> tail of the array to itself, if the predicate throws an exception and
> nothing has been deleted yet. You could add a r != w check, or possibly
> deleted > 0 if you prefer, or maybe we don't care because this is a rare
> (we hope) error recovery case.
>
>
I had the same thoughts... I added checks for deleted > 0


> ***
>
> I have some comments on ArrayDeque:
>
> * Change in allocation/capacity policy.
>
> The removal of the power-of-two restriction, and applying a 1.5x growth
> factor (same as ArrayList) seems sensible. Does this mean that the ability
> to compute the proper array index by using x & (length-1) wasn't worth it?
> Or at least not worth the extra tail wastage?
>

There's no integer division to optimize, as with hash tables.

It was the horror of user discovering that ArrayDeque(2^n) allocated
2^(n+1) that persuaded me to go down this road.
It's also very useful to allocate exactly the requested size on
ArrayDeque(m) or be precise with trimToSize or allow growth up to (almost)
Integer.MAX_VALUE.
A circular buffer is a very simple data structure - our implementation
should be a model!


> (Potential future enhancement: allocate the array lazily, like ArrayList)
>
> Note, I haven't digested all the changes yet.
>
> * API additions
>
> I'm somewhat undecided about these.
>
> I've always felt that the ensureCapacity() and trimToSize() methods were a
> sop added to ArrayList aimed at people converting from Vector. The
> ArrayList.ensureCapacity() method seems to be used a lot, but probably not
> to great effect, since addAll() gives exact sizing anyway. The trimToSize()
> method could be useful in some cases, but should we hold out for a
> generalized one, as requested by JDK-4619094?
>

ensureCapacity is the least useful - just saving less than a factor of 2
allocation on growth.  trimToSize seems more important, since it saves
memory "permanently".


> On the other hand, ArrayDeque is modeling an array, so providing some size
> control seems mostly harmless.
>
> The replaceAll() method is a bit oddly placed here. It's a default method
> on List (which ArrayDeque doesn't, and can't implement) so it's a bit
> strange to have a special method here with the same name and semantics as
> the one on List, but with a "fork" of the specification.
>
>
yeah, it's adding a List method even though the mega-feature of List view
is deferred.


> Maybe if JDK-8143850 is implemented to give a list view of an ArrayDeque,
> the replaceAll() method could be implemented on that:
>
> arrayDeque.asList().replaceAll(clazz::method);
>

yeah, the idea might be that we add List methods like get(i) and replaceAll
directly on ArrayDeque, but only when you call asList do you get something
that implements List ... which is ... odd.


***
>
> I'm not sure what to think of the arrangement of the tests. The "core"
> collections, that is, collections in java.util, exclusive of
> java.util.concurrent, are mostly tested by tests in jdk/test/java/util.
> This patch adds tests for ArrayList and ArrayDeque inside of
> jdk/test/java/util/concurrent/tck. There's a test group
> jdk_collections_core that's intended to test the core collections, so it'll
> miss the new tests being added here.
>
> I'm not sure what to suggest. The new tests use the JSR166TestCase
> framework, which is probably useful, and probably can't be used if the
> tests are moved elsewhere. I don't know what it would take to convert this
> to jtreg or testng. Or, the core test group could be modified to run
> selected JSR166 test cases, which doesn't seem quite right either.
> Suggestions?
>
>
There's already a certain amount of mixing, e.g. stream tests sometimes
test j.u.c. and Queue tests sometimes test all the Queue implementations.
Unlike non-test code, some redundancy 

Re: RFR: jsr166 jdk9 integration wave 12

[Paul Sandoz]

Hi,

The j.u.c. changes look ok to me as do the misc changes.

The ArrayDeque changes may also change the performance characteristics, a 
space/time trade-off e.g. in current version array bounds checks are strength 
reduced to a zero-based test of the array length. Unsure in practice if this 
really matters.

At this stage in Java 9 i would be inclined not to make ArrayDeque more 
ArrayList-like with new public methods ensureCapacity/trimToSize/replaceAll. Do 
you mind if we hold off an that for now and think more about that?

Testing-wise there is always gonna be some overlap. It would be nice to 
consolidate, although arguably in principle TCK has a slightly different focus. 
Now that the tck is in the OpenJDK repo perhaps we should add that to the 
jdk_collections_core?

Paul.

> On 18 Oct 2016, at 10:15, Stuart Marks  wrote:
> 
> 
> 
> On 10/17/16 6:34 PM, Martin Buchholz wrote:
>> Most of this is for Stuart - very collection-y.
>> 
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/
>> 
>> This includes a rewrite of ArrayDeque to bring it to parity with ArrayList
>> (except for List features).
>> The patch includes public methods ensureCapacity, trimToSize, replaceAll as 
>> in
>> ArrayList, but we can defer making them public to a later change (or a later
>> release), since new public methods are always controversial.  But I'd really
>> like to get the performance/scalability changes in for jdk 9.
>> 
>> It also includes a redo of ArrayList#removeIf to make it more efficient and
>> consistent with behavior of other implementations, including ArrayDeque.
>> 
>> The patches are a little tangled because they step on each other's toes.  
>> File
>> CollectionTest is in "miscellaneous", but it tests both the ArrayDeque and
>> ArrayList changes.
> 
> Hi Martin,
> 
> ArrayList.removeIf() is nice. I like the way it recovers from the predicate 
> having thrown an exception. I note that it uselessly copies the tail of the 
> array to itself, if the predicate throws an exception and nothing has been 
> deleted yet. You could add a r != w check, or possibly deleted > 0 if you 
> prefer, or maybe we don't care because this is a rare (we hope) error 
> recovery case.
> 
> ***
> 
> I have some comments on ArrayDeque:
> 
> * Change in allocation/capacity policy.
> 
> The removal of the power-of-two restriction, and applying a 1.5x growth 
> factor (same as ArrayList) seems sensible. Does this mean that the ability to 
> compute the proper array index by using x & (length-1) wasn't worth it? Or at 
> least not worth the extra tail wastage?
> 
> (Potential future enhancement: allocate the array lazily, like ArrayList)
> 
> Note, I haven't digested all the changes yet.
> 
> * API additions
> 
> I'm somewhat undecided about these.
> 
> I've always felt that the ensureCapacity() and trimToSize() methods were a 
> sop added to ArrayList aimed at people converting from Vector. The 
> ArrayList.ensureCapacity() method seems to be used a lot, but probably not to 
> great effect, since addAll() gives exact sizing anyway. The trimToSize() 
> method could be useful in some cases, but should we hold out for a 
> generalized one, as requested by JDK-4619094?
> 
> On the other hand, ArrayDeque is modeling an array, so providing some size 
> control seems mostly harmless.
> 
> The replaceAll() method is a bit oddly placed here. It's a default method on 
> List (which ArrayDeque doesn't, and can't implement) so it's a bit strange to 
> have a special method here with the same name and semantics as the one on 
> List, but with a "fork" of the specification.
> 
> Maybe if JDK-8143850 is implemented to give a list view of an ArrayDeque, the 
> replaceAll() method could be implemented on that:
> 
>arrayDeque.asList().replaceAll(clazz::method);
> 
> ***
> 
> I'm not sure what to think of the arrangement of the tests. The "core" 
> collections, that is, collections in java.util, exclusive of 
> java.util.concurrent, are mostly tested by tests in jdk/test/java/util. This 
> patch adds tests for ArrayList and ArrayDeque inside of 
> jdk/test/java/util/concurrent/tck. There's a test group jdk_collections_core 
> that's intended to test the core collections, so it'll miss the new tests 
> being added here.
> 
> I'm not sure what to suggest. The new tests use the JSR166TestCase framework, 
> which is probably useful, and probably can't be used if the tests are moved 
> elsewhere. I don't know what it would take to convert this to jtreg or 
> testng. Or, the core test group could be modified to run selected JSR166 test 
> cases, which doesn't seem quite right either. Suggestions?
> 
> ***
> 
> I haven't looked at the other j.u.c changes. I haven't done so historically, 
> but I can if you need a reviewer.
> 
> s'marks
> 

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

[Mandy Chung]

> On Oct 18, 2016, at 3:55 AM, Peter Levart  wrote:
> 
> They do. What do you say about this variant (compressionLevel=9):
> 
> 
> http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/webrev.04/

+1.  Ship it!  Thanks for compressing it!  

Mandy

Re: RFR: jsr166 jdk9 integration wave 12

[Stuart Marks]

On 10/17/16 6:34 PM, Martin Buchholz wrote:

Most of this is for Stuart - very collection-y.

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

This includes a rewrite of ArrayDeque to bring it to parity with ArrayList
(except for List features).
The patch includes public methods ensureCapacity, trimToSize, replaceAll as in
ArrayList, but we can defer making them public to a later change (or a later
release), since new public methods are always controversial.  But I'd really
like to get the performance/scalability changes in for jdk 9.

It also includes a redo of ArrayList#removeIf to make it more efficient and
consistent with behavior of other implementations, including ArrayDeque.

The patches are a little tangled because they step on each other's toes.  File
CollectionTest is in "miscellaneous", but it tests both the ArrayDeque and
ArrayList changes.


Hi Martin,

ArrayList.removeIf() is nice. I like the way it recovers from the predicate 
having thrown an exception. I note that it uselessly copies the tail of the 
array to itself, if the predicate throws an exception and nothing has been 
deleted yet. You could add a r != w check, or possibly deleted > 0 if you 
prefer, or maybe we don't care because this is a rare (we hope) error recovery case.


***

I have some comments on ArrayDeque:

* Change in allocation/capacity policy.

The removal of the power-of-two restriction, and applying a 1.5x growth factor 
(same as ArrayList) seems sensible. Does this mean that the ability to compute 
the proper array index by using x & (length-1) wasn't worth it? Or at least not 
worth the extra tail wastage?


(Potential future enhancement: allocate the array lazily, like ArrayList)

Note, I haven't digested all the changes yet.

* API additions

I'm somewhat undecided about these.

I've always felt that the ensureCapacity() and trimToSize() methods were a sop 
added to ArrayList aimed at people converting from Vector. The 
ArrayList.ensureCapacity() method seems to be used a lot, but probably not to 
great effect, since addAll() gives exact sizing anyway. The trimToSize() method 
could be useful in some cases, but should we hold out for a generalized one, as 
requested by JDK-4619094?


On the other hand, ArrayDeque is modeling an array, so providing some size 
control seems mostly harmless.


The replaceAll() method is a bit oddly placed here. It's a default method on 
List (which ArrayDeque doesn't, and can't implement) so it's a bit strange to 
have a special method here with the same name and semantics as the one on List, 
but with a "fork" of the specification.


Maybe if JDK-8143850 is implemented to give a list view of an ArrayDeque, the 
replaceAll() method could be implemented on that:


arrayDeque.asList().replaceAll(clazz::method);

***

I'm not sure what to think of the arrangement of the tests. The "core" 
collections, that is, collections in java.util, exclusive of 
java.util.concurrent, are mostly tested by tests in jdk/test/java/util. This 
patch adds tests for ArrayList and ArrayDeque inside of 
jdk/test/java/util/concurrent/tck. There's a test group jdk_collections_core 
that's intended to test the core collections, so it'll miss the new tests being 
added here.


I'm not sure what to suggest. The new tests use the JSR166TestCase framework, 
which is probably useful, and probably can't be used if the tests are moved 
elsewhere. I don't know what it would take to convert this to jtreg or testng. 
Or, the core test group could be modified to run selected JSR166 test cases, 
which doesn't seem quite right either. Suggestions?


***

I haven't looked at the other j.u.c changes. I haven't done so historically, but 
I can if you need a reviewer.


s'marks

Re: RFR: 8163984: Fix license and copyright headers in jdk9 under test/lib

[Stanislav Smirnov]

Hi,

I'm still looking for volunteers to review

Best regards,
Stanislav Smirnov





> On 05 Oct 2016, at 19:44, Stanislav Smirnov  
> wrote:
> 
> Hi,
> 
> Please review this fix for JDK-8163984 
> .
> This one is similar to another one, I have sent earlier.
> Legal notices and Oracle copyrights were updated (white and blank space, 
> commas) in tests files under test/lib for uniformity to meet Oracle 
> requirements.
> 
> JBS bug: https://bugs.openjdk.java.net/browse/JDK-8163984 
> 
> Webrev: http://cr.openjdk.java.net/~stsmirno/8163984/webrev.00/ 
> 
> Best regards,
> Stanislav Smirnov
> 
> 
> 
> 
> 

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

[Peter Levart]

Hi Alan,


On 10/14/2016 02:16 PM, Alan Bateman wrote:

On 13/10/2016 18:30, Peter Levart wrote:


Hi Paul, Alan,

I incorporated Paul's suggestions into new webrev:

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



This iteration also contains a nearly-exhaustive test of 
Class.getMethods(): PublicMethodsTest. It is actually a test 
generator. Given a Case template, it generates all variants of 
methods for each of the types in the case. Case1 contains 4 interface 
method variants ^ 3 interfaces * 4 class method variants ^ 3 classes 
= 4^6 = 4096 different sub-cases of which only 1379 compile. The 
results of those 1379 sub-cases are persisted in the Case1.results 
file. Running the test compares the persisted results with actual 
result of executing each sub-case. When running this test on plain 
JDK 9 (without patch), the test finds 218 sub-cases where results 
differ:


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



Looking at those differences gives the impression of the effects of 
the patch.
Overall I think this looks very good. I mostly focused on the 
PublicMethods implementation to satisfy myself that it does selects 
the most specific methods. In passing I wonder if "combine" or "merge" 
might be better than "consolidate" for method name and terminology, a 
minor point of course.


What about "coalesce" ?



Given the behavior change then I think we'll need to capture it in a 
release notes. I can't think of any libraries or frameworks that might 
have see but something might come out of the woodwork and would be 
nice to be able to point to a summary.


What do you think of specifying new behavior in javadoc(s) of 
getMethod() and getMethods() themselves? The description in those docs 
was never very precise. It is composed of a few seemingly unconnected 
statements which don't give a complete picture of what those methods 
return. It is now plainly wrong in getMethod() and does not give any 
clew about what it means to "inherit" a method from supertype in 
getMethods(). I have tried to capture the precise behavior in the 
changed javadocs that I present here:


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

Those javadocs refer to definitions of two other methods: 
Class.getDeclaredMethods() and Class.isAssignableFrom(). Their 
definitions are leveraged so javadocs of getMethod() and getMethods() 
themselves can be simpler - I removed the redundant statements that are 
a direct consequence of the algorithm description.


We could then perhaps point from release notes to the new javadocs or 
even include them.




I assume that copyright headers will be added before this is pushed.


Added in above webrev.

Also there is a @SuppressWarnings arguments that I don't recognize 
(IDE specific)? It would good to trim back some of the really long 
times in the test too (there are some >150 char lines that will be a 
pain to review side-by-side when there are future changes).


The trimming was done. I have missed the @SuppressWarnings. Will do it 
in next iteration.


I have updated the test which now includes Class.getMethod() testing 
too. It also presents results (differences from expected) in a more 
pleasing way - just those results that differ are presented. Here's a 
result of running the test on unpatched JDK 9:


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

There are some more differences now that Class.getMethod() is also part 
of the test. Some interesting consequences of the patch (expected=new 
behavior, actual=old behavior):


interface I { void m(); }
interface J {  }
interface K extends I, J { default void m() {} }
abstract class C {  }
abstract class D extends C implements I {  }
abstract class E extends D implements J, K {  }

expected: E.gM: K.m
  actual: E.gM: I.m

...here E.class.getMethods() has the same result between unpatched and 
patched JDK 9, just getMethod() returns different method. Which means 
that in present JDK, getMethod() returns a method that is not included 
in the getMethods() result.



interface I { void m(); }
interface J { void m(); }
interface K extends I, J { void m(); }
abstract class C { public abstract void m(); }
abstract class D extends C implements I {  }
abstract class E extends D implements J, K {  }

expected: E.gMs: [C.m]
  actual: E.gMs: [C.m, I.m, J.m, K.m]

expected: D.gMs: [C.m]
  actual: D.gMs: [C.m, I.m]

...huge difference, huh.


interface I { default void m() {} }
interface J { void m(); }
interface K extends I, J { void m(); }
abstract class C {  }
abstract class D extends C implements I {  }
abstract class E extends D implements J, K {  }

expected: E.gMs: [K.m]
  actual: E.gMs: [I.m, J.m, K.m]

expected: E.gM: K.m
  actual: E.gM: I.m

...difference in both getMethod() and getMethods().


Regards, Peter

Re: RFR 8071678: javax.script.ScriptContext setAttribute method should clarify behavior when GLOBAL_SCOPE is used and global scope object is null

[Hannes Wallnöfer]

+1

Hannes

> Am 18.10.2016 um 13:36 schrieb Sundararajan Athijegannathan 
> :
> 
> Please review http://cr.openjdk.java.net/~sundar/8071678/webrev.00/ for
> https://bugs.openjdk.java.net/browse/JDK-8071678
> 
> Thanks,
> 
> -Sundar
> 

Re: RFR 8071678: javax.script.ScriptContext setAttribute method should clarify behavior when GLOBAL_SCOPE is used and global scope object is null

[Jim Laskey (Oracle)]

+1

> On Oct 18, 2016, at 8:36 AM, Sundararajan Athijegannathan 
>  wrote:
> 
> Please review http://cr.openjdk.java.net/~sundar/8071678/webrev.00/ for
> https://bugs.openjdk.java.net/browse/JDK-8071678
> 
> Thanks,
> 
> -Sundar
> 

RFR 8071678: javax.script.ScriptContext setAttribute method should clarify behavior when GLOBAL_SCOPE is used and global scope object is null

[Sundararajan Athijegannathan]

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

Thanks,

-Sundar

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

[Peter Levart]

Hi Mandy,


On 10/18/2016 01:51 AM, Mandy Chung wrote:

>Besides, constant names would not be any prettier than class name string 
literals. At least now it is obvious to anyone what package a particular class 
belongs to:
>
> "a.Package" vs. A_PACKAGE ?
>

PACKAGE_CLASS_IN_PKG_A
PUBLIC_SUPERCLASS_IN_PKG_A
PUBLIC_SUBCLASS_IN_PKG_A

Do the suggested variable names help?


They do. What do you say about this variant (compressionLevel=9):

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

Regards, Peter

Re: RFR:JDK-8075205 java/net/URLClassLoader/closetest/CloseTest.java and GetResourceAsStream.java failed with existing dir

[Chris Hegarty]

> On 17 Oct 2016, at 09:51, Srinivasan Raghavan 
>  wrote:
> 
> Hi all
> 
> Please review the fix for the bug 
> 
> Bug :https://bugs.openjdk.java.net/browse/JDK-8075205
> 
> The tests uses classes directory for the output files. This can result in the 
> files being left over after the test is complete which can result in 
> instability. The tests copies the files to be compiled form test src to test 
> classes which can result in copy of permission and result in instability 
> because the test has delete operations. The test fails randomly mostly in 
> copy or delete operation. I propose the test to be refactored to make the use 
> scratch directory as its output directory and eliminate shell by using 
> testlibrary utils.
> 
> fix : http://cr.openjdk.java.net/~sraghavan/8075205/webrev.00/

This looks good to me. Thanks Srinivasan.

-Chris.

Re: RFR: 8168073: Speed up URI creation during module bootstrap

[Chris Hegarty]

On 17/10/2016 22:18, Claes Redestad wrote:
> 
>> On 2016-10-17 21:35, Alan Bateman wrote:
>>> 
>>> JavaNetHttpCookieAccess, JavaNetInetAddressAccess and
>>> JavaNetSocketAccess are the other 3 that are used to get at non-public
>>> types in java.net so I don't think JavaNetUriAccess would be out of place.
>> 
>> TIL!
>> 
>> Ok, renamed and added an assert by Paul's request:
>> 
>> http://cr.openjdk.java.net/~redestad/8168073/webrev.02/

Thanks Claes, this looks good.

-Chris.

Re: RFR: 8168073: Speed up URI creation during module bootstrap

[Alan Bateman]

On 17/10/2016 22:18, Claes Redestad wrote:




On 2016-10-17 21:35, Alan Bateman wrote:


JavaNetHttpCookieAccess, JavaNetInetAddressAccess and
JavaNetSocketAccess are the other 3 that are used to get at non-public
types in java.net so I don't think JavaNetUriAccess would be out of 
place.


TIL!

Ok, renamed and added an assert by Paul's request:

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

Looks good.

-Alan