date:20141201

Hi all,

Looking for a reviewer for this trivial typo in Connection.isValid

hg diff
diff -r f619341171c0 src/java.sql/share/classes/java/sql/Connection.java
--- a/src/java.sql/share/classes/java/sql/Connection.java   Sat Nov 29 
11:14:20 2014 -0500
+++ b/src/java.sql/share/classes/java/sql/Connection.java   Mon Dec 01 
11:01:14 2014 -0500
@@ -1116,7 +1116,7 @@
  *
  * @return true if the connection is valid, false otherwise
  * @exception SQLException if the value supplied for 
codetimeout/code
- * is less then 0
+ * is less than 0
  * @since 1.6
  *
  * @see java.sql.DatabaseMetaData#getClientInfoProperties
ljanders-mac:jdk ljanders$ 


Best
Lance


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 8066261: Typo in Connection.isValid


Looks good Lance.

I had to stare at the diff for a while before seeing it ;-)

best regards,

-- daniel

On 01/12/14 17:04, Lance Andersen wrote:

Hi all,

Looking for a reviewer for this trivial typo in Connection.isValid

hg diff
diff -r f619341171c0 src/java.sql/share/classes/java/sql/Connection.java
--- a/src/java.sql/share/classes/java/sql/Connection.java   Sat Nov 29 
11:14:20 2014 -0500
+++ b/src/java.sql/share/classes/java/sql/Connection.java   Mon Dec 01 
11:01:14 2014 -0500
@@ -1116,7 +1116,7 @@
   *
   * @return true if the connection is valid, false otherwise
   * @exception SQLException if the value supplied for 
codetimeout/code
- * is less then 0
+ * is less than 0
   * @since 1.6
   *
   * @see java.sql.DatabaseMetaData#getClientInfoProperties
ljanders-mac:jdk ljanders$


Best
Lance


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

RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException


Hi,

Please find below a patch for:

8065552: setAccessible(true) on fields of Class may throw
 a SecurityException

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8065552/webrev.00/

Description of the problem:

The following test case passes on 8u20 but fails on 8u40 and above:

 public class Test {
 public static void main(String[] args) throws Throwable {
 for (Field f : Class.class.getDeclaredFields()) {
 f.setAccessible(true);
 }
 }
 }

The fix for JDK-6642881 introduced a new private field to Class, named 
classloader, whose accessibility can never be modified (from the 
default of non-accessible to accessible).


This issue manifests itself in Jython where, when the 
Options.respectJavaAccessibility is false (by default it is true), a 
SecurityException occurs when it tries to setAccessible(true) all 
declared fields on Class:



https://hg.python.org/jython/file/tip/src/org/python/core/PyJavaType.java#l405

The SecurityException is lost in the noise of other exceptions as the 
error propagates through the runtime. The observable symptom is

a NullPointerException which occurs when one tries to load the
Jython engine. With 8u40 it fails with exception:

java.lang.NullPointerException
at org.python.core.Py.recursiveIsInstance(Py.java:1861)
at org.python.core.Py.isInstance(Py.java:1828)
at org.python.core.__builtin__.isinstance(__builtin__.java:725)
at org.python.core.Py.displayException(Py.java:1009)
at org.python.core.PyException.printStackTrace(PyException.java:79)
at org.python.core.PyException.toString(PyException.java:98)
at org.apache.commons.logging.impl.SimpleLog.log(SimpleLog.java:329)
at org.apache.commons.logging.impl.SimpleLog.error(SimpleLog.java:525)
at org.apache.bsf.BSFManager.loadScriptingEngine(BSFManager.java:717)
...

The fix is to hide the field from reflection instead of simply
preventing it to be set as accessible.

best regards,

-- daniel

Re: RFR 8066261: Typo in Connection.isValid

Thank you Daniel.

Yes, it is easy to miss (which is probably why it was not caught before :-) )

Best,
Lance
On Dec 1, 2014, at 11:25 AM, Daniel Fuchs daniel.fu...@oracle.com wrote:

 Looks good Lance.
 
 I had to stare at the diff for a while before seeing it ;-)
 
 best regards,
 
 -- daniel
 
 On 01/12/14 17:04, Lance Andersen wrote:
 Hi all,
 
 Looking for a reviewer for this trivial typo in Connection.isValid
 
 hg diff
 diff -r f619341171c0 src/java.sql/share/classes/java/sql/Connection.java
 --- a/src/java.sql/share/classes/java/sql/Connection.javaSat Nov 29 
 11:14:20 2014 -0500
 +++ b/src/java.sql/share/classes/java/sql/Connection.javaMon Dec 01 
 11:01:14 2014 -0500
 @@ -1116,7 +1116,7 @@
   *
   * @return true if the connection is valid, false otherwise
   * @exception SQLException if the value supplied for 
 codetimeout/code
 - * is less then 0
 + * is less than 0
   * @since 1.6
   *
   * @see java.sql.DatabaseMetaData#getClientInfoProperties
 ljanders-mac:jdk ljanders$
 
 
 Best
 Lance
 
 
 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

RFR 8060068 : Remove the static initializer block from DriverManager

Hi all,

Looking for a review for this change to DriverManager to reduce the possibility 
on a deadlock on clinit. that I have been discussing with Mandy.


The change removes the static initializer block as well as the synchronized 
keyword from registerDriver.

The webrev can be found at 
http://cr.openjdk.java.net/%7Elancea/8060068/webrev.02/


Best,
Lance


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: [concurrency-interest] RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics

2014-12-01 Thread Stephan Diestelhorst

Am Dienstag, 25. November 2014, 11:15:36 schrieb Hans Boehm:
 I'm no hardware architect, but fundamentally it seems to me that
 
 load x
 acquire_fence
 
 imposes a much more stringent constraint than
 
 load_acquire x
 
 Consider the case in which the load from x is an L1 hit, but a preceding
 load (from say y) is a long-latency miss.  If we enforce ordering by just
 waiting for completion of prior operation, the former has to wait for the
 load from y to complete; while the latter doesn't.  I find it hard to
 believe that this doesn't leave an appreciable amount of performance on the
 table, at least for some interesting microarchitectures.

I agree, Hans, that this is a reasonable assumption.  Load_acquire x
does allow roach motel, whereas the acquire fence does not.

  In addition, for better or worse, fencing requirements on at least
  Power are actually driven as much by store atomicity issues, as by
  the ordering issues discussed in the cookbook.  This was not
  understood in 2005, and unfortunately doesn't seem to be amenable to
  the kind of straightforward explanation as in Doug's cookbook.

Coming from a strongly ordered architecture to a weakly ordered one
myself, I also needed some mental adjustment about store (multi-copy)
atomicity.  I can imagine others will be unaware of this difference,
too, even in 2014.

Stephan

Re: [concurrency-interest] RFR: 8065804: JEP171:Clarifications/corrections for fence intrinsics

2014-12-01 Thread Hans Boehm

To be concrete here, on Power, loads can normally be ordered by an address
dependency or light-weight fence (lwsync).  However, neither is enough to
prevent the questionable outcome for IRIW, since it doesn't ensure that the
stores in T1 and T2 will be made visible to other threads in a consistent
order.  That outcome can be prevented by using heavyweight fences (sync)
instructions between the loads instead.  Peter Sewell's group concluded
that to enforce correct volatile behavior on Power, you essentially need a
a heavyweight fence between every pair of volatile operations on Power.
That cannot be understood based on simple ordering constraints.

As Stephan pointed out, there are similar issues on ARM, but they're less
commonly encountered in a Java implementation.  If you're lucky, you can
get to the right implementation recipe by looking at only reordering, I
think.


On Tue, Nov 25, 2014 at 4:36 PM, David Holmes davidchol...@aapt.net.au
wrote:

 Stephan Diestelhorst writes:
 
  David Holmes wrote:
   Stephan Diestelhorst writes:
Am Dienstag, 25. November 2014, 11:15:36 schrieb Hans Boehm:
 I'm no hardware architect, but fundamentally it seems to me that

 load x
 acquire_fence

 imposes a much more stringent constraint than

 load_acquire x

 Consider the case in which the load from x is an L1 hit, but a
 preceding load (from say y) is a long-latency miss.  If we enforce
 ordering by just waiting for completion of prior operation, the
 former has to wait for the load from y to complete; while the
 latter doesn't.  I find it hard to believe that this doesn't leave
 an appreciable amount of performance on the table, at least for
 some interesting microarchitectures.
   
I agree, Hans, that this is a reasonable assumption.  Load_acquire x
does allow roach motel, whereas the acquire fence does not.
   
  In addition, for better or worse, fencing requirements on at least
  Power are actually driven as much by store atomicity issues, as by
  the ordering issues discussed in the cookbook.  This was not
  understood in 2005, and unfortunately doesn't seem to be
  amenable to
  the kind of straightforward explanation as in Doug's cookbook.
   
Coming from a strongly ordered architecture to a weakly ordered one
myself, I also needed some mental adjustment about store (multi-copy)
atomicity.  I can imagine others will be unaware of this difference,
too, even in 2014.
  
   Sorry I'm missing the connection between fences and multi-copy
  atomicity.
 
  One example is the classic IRIW.  With non-multi copy atomic stores, but
  ordered (say through a dependency) loads in the following example:
 
  Memory: foo = bar = 0
  _T1_ _T2_ _T3_  _T4_
  st (foo),1   st (bar),1   ld r1, (bar)  ld r3,(foo)
addr dep / local fence here   addr dep
ld r2, (foo)  ld r4, (bar)
 
  You may observe r1 = 1, r2 = 0, r3 = 1, r4 = 0 on non-multi-copy atomic
  machines.  On TSO boxes, this is not possible.  That means that the
  memory fence that will prevent such a behaviour (DMB on ARM) needs to
  carry some additional oomph in ensuring multi-copy atomicity, or rather
  prevent you from seeing it (which is the same thing).

 I take it as given that any code for which you may have ordering
 constraints, must first have basic atomicity properties for loads and
 stores. I would not expect any kind of fence to add multi-copy-atomicity
 where there was none.

 David

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

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

Re: [concurrency-interest] RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics

2014-12-01 Thread DT

I see time to time comments in the jvm sources referencing membars and fences. 
Would you say that they are used interchangeably ? Having the same meaning but 
for different CPU arch.

Sent from my iPhone

 On Nov 25, 2014, at 6:04 AM, Paul Sandoz paul.san...@oracle.com wrote:
 
 Hi Martin,
 
 Thanks for looking into this.
 
 1141  * Currently hotspot's implementation of a Java language-level 
 volatile
 1142  * store has the same effect as a storeFence followed by a relaxed 
 store,
 1143  * although that may be a little stronger than needed.
 
 IIUC to emulate hotpot's volatile store you will need to say that a fullFence 
 immediately follows the relaxed store.
 
 The bit that always confuses me about release and acquire is ordering is 
 restricted to one direction, as talked about in orderAccess.hpp [1]. So for a 
 release, accesses prior to the release cannot move below it, but accesses 
 succeeding the release can move above it. And that seems to apply to 
 Unsafe.storeFence [2] (acting like a monitor exit). Is that contrary to C++ 
 release fences where ordering is restricted both to prior and succeeding 
 accesses? [3]
 
 So what about the following?
 
  a = r1; // Cannot move below the fence
  Unsafe.storeFence();
  b = r2; // Can move above the fence?
 
 Paul.
 
 [1] In orderAccess.hpp
 // Execution by a processor of release makes the effect of all memory
 // accesses issued by it previous to the release visible to all
 // processors *before* the release completes.  The effect of subsequent
 // memory accesses issued by it *may* be made visible *before* the
 // release.  I.e., subsequent memory accesses may float above the
 // release, but prior ones may not float below it.
 
 [2] In memnode.hpp
 // Release - no earlier ref can move after (but later refs can move
 // up, like a speculative pipelined cache-hitting Load).  Requires
 // multi-cpu visibility.  Inserted independent of any store, as required
 // for intrinsic sun.misc.Unsafe.storeFence().
 class StoreFenceNode: public MemBarNode {
 public:
  StoreFenceNode(Compile* C, int alias_idx, Node* precedent)
: MemBarNode(C, alias_idx, precedent) {}
  virtual int Opcode() const;
 };
 
 [3] 
 http://preshing.com/20131125/acquire-and-release-fences-dont-work-the-way-youd-expect/
 
 On Nov 25, 2014, at 1:47 AM, Martin Buchholz marti...@google.com wrote:
 
 OK, I worked in some wording for comparison with volatiles.
 I believe you when you say that the semantics of the corresponding C++
 fences are slightly different, but it's rather subtle - can we say
 anything more than closely related to?
 
 On Mon, Nov 24, 2014 at 1:29 PM, Aleksey Shipilev
 aleksey.shipi...@oracle.com wrote:
 Hi Martin,
 
 On 11/24/2014 11:56 PM, Martin Buchholz wrote:
 Review carefully - I am trying to learn about fences by explaining them!
 I have borrowed some wording from my reviewers!
 
 https://bugs.openjdk.java.net/browse/JDK-8065804
 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/fence-intrinsics/
 
 I think implies the effect of C++11 is too strong wording. related
 might be more appropriate.
 
 See also comments here for connection with volatiles:
 https://bugs.openjdk.java.net/browse/JDK-8038978
 
 Take note the Hans' correction that fences generally imply more than
 volatile load/store, but since you are listing the related things in the
 docs, I think the native Java example is good to have.
 
 -Aleksey.
 ___
 Concurrency-interest mailing list
 concurrency-inter...@cs.oswego.edu
 http://cs.oswego.edu/mailman/listinfo/concurrency-interest
 
 ___
 Concurrency-interest mailing list
 concurrency-inter...@cs.oswego.edu
 http://cs.oswego.edu/mailman/listinfo/concurrency-interest

Re: [concurrency-interest] RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics

2014-12-01 Thread Alexander Terekhov

 memory_order_release meaningful, but somewhat different from the
non-fence

it would be nice to have release fence with an artificial dependency
to define a set of actually release stores and not constraining other
subsequent stores (and the order of release stores with respect to
each other), e.g.:

// set multiple flags each indicating 'release' without imposing
// ordering on 'release' stores respect to each other and not
// constraining other subsequent stores

.
.
.

if (atomic_thread_fence(memory_order_release)) {

  flag1.store(READY, memory_order_relaxed);
  flag2.store(READY, memory_order_relaxed);

}

regards,
alexander.

Hans Boehm bo...@acm.org@cs.oswego.edu on 29.11.2014 05:56:04

Sent by:concurrency-interest-boun...@cs.oswego.edu


To: Peter Levart peter.lev...@gmail.com
cc: Vladimir Kozlov vladimir.koz...@oracle.com, concurrency-interest
   concurrency-inter...@cs.oswego.edu, Martin Buchholz
   marti...@google.com, core-libs-dev
   core-libs-dev@openjdk.java.net, dhol...@ieee.org
Subject:Re: [concurrency-interest] RFR: 8065804: JEP 171:
   Clarifications/corrections for fence intrinsics


I basically agree with David's observation.  However the C++

atomic_thread_fence(memory_order_acquire)

actually has somewhat different semantics from load(memory_order_acquire).
It basically ensures that prior atomic loads L are not reordered with later
(i.e. following the fence in program order) loads and stores, making it
something like a LoadLoad|LoadStore fence.  Thus the fence orders two sets
of operations where the acquire load orders a single operation with respect
to a set.  This makes the fence versions of memory_order_acquire and
memory_order_release meaningful, but somewhat different from the non-fence
versions.  The terminology is probably not great, but that seems to be the
most common usage now.

On Wed, Nov 26, 2014 at 11:48 PM, Peter Levart peter.lev...@gmail.com
wrote:
  On 11/27/2014 04:00 AM, David Holmes wrote:
Can I make an observation about acquire() and release() - to me
they are
meaningless when considered in isolation. Given their
definitions they allow
anything to move into a region bounded by acquire() and release
(), then you
can effectively move the whole program into the region and thus
the
acquire() and release() do not constrain any reorderings.
  acquire() and
release() only make sense when their own movement is
constrained with
respect to something else - such as lock acquisition/release,
or when
combined with specific load/store actions.

  ...or another acquire/release region?

  Regards, Peter



David

Martin Buchholz writes:
  On Tue, Nov 25, 2014 at 6:04 AM, Paul Sandoz
  paul.san...@oracle.com wrote:
Hi Martin,

Thanks for looking into this.

1141      * Currently hotspot's implementation of a
Java
  language-level volatile
1142      * store has the same effect as a
storeFence followed
  by a relaxed store,
1143      * although that may be a little stronger
than needed.

IIUC to emulate hotpot's volatile store you will
need to say
  that a fullFence immediately follows the relaxed store.

  Right - I've been groking that.

The bit that always confuses me about release and
acquire is
  ordering is restricted to one direction, as talked about
  in
  orderAccess.hpp [1]. So for a release, accesses prior to
  the
  release cannot move below it, but accesses succeeding the
  release
  can move above it. And that seems to apply to
  Unsafe.storeFence
  [2] (acting like a monitor exit). Is that contrary to C++
  release
  fences where ordering is restricted both to prior and
  succeeding
  accesses? [3]
So what about the following?

   a = r1; // Cannot move below the fence
   Unsafe.storeFence();
   b = r2; // Can move above the fence?
  I think the hotspot docs need to be more precise about
  when they're
  talking about movement of stores and when about loads.

// release.  I.e., subsequent memory accesses may
float above the

Re: Packing 2 data points into 1 field in ThreadPoolExecutor

2014-12-01 Thread Vitaly Davidovich

It allows to manipulate two (related) bits of info atomically without
needing a lock and when efficient double CAS is not available (which it
isn't on supported archs).

Sent from my phone
On Dec 1, 2014 12:23 PM, Alex Yursha alexyur...@gmail.com wrote:

 Hi all,

 According to javadoc current implementation of ThreadPoolExecutor packs
 two conceptual fields ‘workerCount’ and ‘runState’ into one actual field
 ‘ctl’ of type AtomicInteger.

 Could you please explain are there any performance or other benefits for
 this? It seems to complicate the class design and I can’t find the positive
 side of this.

 Thanks,
 Alex

Re: Packing 2 data points into 1 field in ThreadPoolExecutor

The only way to use atomic compare and set is to pack all your state
into a single primitive unit.  The largest we have is long.  So we
regularly pack what the C folks would call bitfields into longs.

On Sat, Nov 29, 2014 at 8:13 AM, Alex Yursha alexyur...@gmail.com wrote:
 Hi all,

 According to javadoc current implementation of ThreadPoolExecutor packs two 
 conceptual fields ‘workerCount’ and ‘runState’ into one actual field ‘ctl’ of 
 type AtomicInteger.

 Could you please explain are there any performance or other benefits for 
 this? It seems to complicate the class design and I can’t find the positive 
 side of this.

 Thanks,
 Alex

Re: Packing 2 data points into 1 field in ThreadPoolExecutor

2014-12-01 Thread Alex Yursha

Thanks a lot.

Seems like i need to look deeper into the JVM and hardware intrinsics to 
understand some pecularities of core libs class designs.

Sent from my iPhone

 On Dec 1, 2014, at 20:27, Vitaly Davidovich vita...@gmail.com wrote:
 
 It allows to manipulate two (related) bits of info atomically without needing 
 a lock and when efficient double CAS is not available (which it isn't on 
 supported archs).
 
 Sent from my phone
 
 On Dec 1, 2014 12:23 PM, Alex Yursha alexyur...@gmail.com wrote:
 Hi all,
 
 According to javadoc current implementation of ThreadPoolExecutor packs two 
 conceptual fields ‘workerCount’ and ‘runState’ into one actual field ‘ctl’ 
 of type AtomicInteger.
 
 Could you please explain are there any performance or other benefits for 
 this? It seems to complicate the class design and I can’t find the positive 
 side of this.
 
 Thanks,
 Alex

Re: Packing 2 data points into 1 field in ThreadPoolExecutor

2014-12-01 Thread Alex Yursha

1. Do you mean 'the only way', except using a lock? 

2. I also cant imagine how we can use long primitive type for CAS atomicity 
without a lock if only its not an AtomicLong. Any hint here, please?

Thanks


Sent from my iPhone

 On Dec 1, 2014, at 20:27, Martin Buchholz marti...@google.com wrote:
 
 The only way to use atomic compare and set is to pack all your state
 into a single primitive unit.  The largest we have is long.  So we
 regularly pack what the C folks would call bitfields into longs.
 
 On Sat, Nov 29, 2014 at 8:13 AM, Alex Yursha alexyur...@gmail.com wrote:
 Hi all,
 
 According to javadoc current implementation of ThreadPoolExecutor packs two 
 conceptual fields ‘workerCount’ and ‘runState’ into one actual field ‘ctl’ 
 of type AtomicInteger.
 
 Could you please explain are there any performance or other benefits for 
 this? It seems to complicate the class design and I can’t find the positive 
 side of this.
 
 Thanks,
 Alex

Re: RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException

2014-12-01 Thread Seán Coffey

Looks fine to me Daniel. Thanks for handling it. I can work on the 7u 
backport if necessary.


on the test side would it be worth testing all public classes available 
(e.g in rt.jar ?) to ensure that
Field.setAccessible works as expected and that we don't hit this issue 
again ? It might be some

what of a heavy test for jtreg inclusion though.

regards,
Sean.

On 01/12/14 16:29, Daniel Fuchs wrote:

Hi,

Please find below a patch for:

8065552: setAccessible(true) on fields of Class may throw
 a SecurityException

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8065552/webrev.00/

Description of the problem:

The following test case passes on 8u20 but fails on 8u40 and above:

 public class Test {
 public static void main(String[] args) throws Throwable {
 for (Field f : Class.class.getDeclaredFields()) {
 f.setAccessible(true);
 }
 }
 }

The fix for JDK-6642881 introduced a new private field to Class, named 
classloader, whose accessibility can never be modified (from the 
default of non-accessible to accessible).


This issue manifests itself in Jython where, when the 
Options.respectJavaAccessibility is false (by default it is true), a 
SecurityException occurs when it tries to setAccessible(true) all 
declared fields on Class:



https://hg.python.org/jython/file/tip/src/org/python/core/PyJavaType.java#l405 



The SecurityException is lost in the noise of other exceptions as the 
error propagates through the runtime. The observable symptom is

a NullPointerException which occurs when one tries to load the
Jython engine. With 8u40 it fails with exception:

java.lang.NullPointerException
at org.python.core.Py.recursiveIsInstance(Py.java:1861)
at org.python.core.Py.isInstance(Py.java:1828)
at org.python.core.__builtin__.isinstance(__builtin__.java:725)
at org.python.core.Py.displayException(Py.java:1009)
at org.python.core.PyException.printStackTrace(PyException.java:79)
at org.python.core.PyException.toString(PyException.java:98)
at org.apache.commons.logging.impl.SimpleLog.log(SimpleLog.java:329)
at org.apache.commons.logging.impl.SimpleLog.error(SimpleLog.java:525)
at org.apache.bsf.BSFManager.loadScriptingEngine(BSFManager.java:717)
...

The fix is to hide the field from reflection instead of simply
preventing it to be set as accessible.

best regards,

-- daniel

Re: RFR 8065998: Avoid use of _ as a one-character identifier

2014-12-01 Thread joe darcy


Hi Jan,

The changes look good to me; thanks,

-Joe

On 12/1/2014 5:37 AM, Jan Lahoda wrote:

On 1.12.2014 14:20, Alan Bateman wrote:

On 01/12/2014 12:10, Jan Lahoda wrote:

Hi,

In a preparation for JDK-8061549, I'd like to rename all uses of '_'
as a one-character identifier in the jaxp and jdk repositories. All
the uses I was able to find are in tests, and the identifier is used
as a name of a catch parameter. The proposed new name is ignore, but
if a different name would be more appropriate, I'll be happy to use it.

Webrev for the jaxp repository:
http://cr.openjdk.java.net/~jlahoda/8065998/webrev.00/jaxp/

Webrev for the jdk repository:
http://cr.openjdk.java.net/~jlahoda/8065998/webrev.00/jdk/


In JAXP test Bug4969089 then ignore looks a bit odd given that it's in
a method declaration.


Sorry for that - I've forgot about these uses in method declarations. 
I've updated the patch to use unused for them.




In TypeCheckMicroBenchmark then ignore is might be a misleading too
given that the ArrayStoreException causes a CCE to be thrown.


I've updated the patch to use expected where the exception appears 
to be expected and unused where exception appears to be unexpected, 
but the variable is not used.


Updated patches:
jaxp:
http://cr.openjdk.java.net/~jlahoda/8065998/webrev.01/jaxp/
jdk:
http://cr.openjdk.java.net/~jlahoda/8065998/webrev.01/jdk/

Thanks,
Jan



-Alan

[9, 8u40] RFR (M): 8057020: LambdaForm caches should support eviction

2014-12-01 Thread Vladimir Ivanov


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

There are 2 major LambdaForm caches: LambdaFormEditor-based and 
MethodTypeForm. The former is per-LambdaForm and the latter is per 
method type erased to basic types. The problem is that these caches 
don't support eviction, so they can hold LambdaForms forever.


Usually, it's not a problem since an application has very limited number 
of unique erased method types (e.g. on Octane/Nashorn it varies 1,5-3k 
shapes).


The fix is to use SoftReferences to keep LambdaForms alive as long as 
possible, but avoid throwing OOME until the caches are evicted. I 
experimented with WeakReferences, but it doesn't hold LambdaForms for 
long enough: LambdaForm cache hit rate degrades significantly and it 
negatively affects application startup and warmup, since every 
instantiated LambdaForm is precompiled to bytecode before usage.


Testing: jdk/java/lang/invoke/LFCache in stress mode + jck 
(api/java_lang/invoke), jdk/java/lang/invoke, jdk/java/util/streams, octane


Thanks!

Best regards,
Vladimir Ivanov

Re: RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException

There's a whole set of invariant tests that should be applied to the entire jdk.
Find all the classes that can be loaded, load them, and check all the
invariants you can think of!

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/Class.getMethods-benchmark/test/java/lang/Class/getMethods/LoadAllClassesAndMethods.java.html

But do we have any professional test-writers?

On Mon, Dec 1, 2014 at 9:48 AM, Seán Coffey sean.cof...@oracle.com wrote:
 on the test side would it be worth testing all public classes available (e.g
 in rt.jar ?) to ensure that
 Field.setAccessible works as expected and that we don't hit this issue again
 ? It might be some
 what of a heavy test for jtreg inclusion though.

Re: Better NewIO2 file system implementation for AIX platform

2014-12-01 Thread Volker Simonis

On Mon, Dec 1, 2014 at 9:42 AM, Alan Bateman alan.bate...@oracle.com wrote:
 On 01/12/2014 08:06, deven you wrote:

 Hi All,


Hi Deven,

thank you for your contribution. Please find my comments inline:

 Our current NIO2 file system support to AIX is very limited, hence I took
 some time to try to complete it. Openjdk bug[1] tracks this bug and here
 is
 my patch[2] which enhances the AIX file system especially by adding the
 support by Implementing AixDosFileAttributeView.java and
 AixUserDefinedFileAttributeView.java.
.
 Could anyone take a look?

 Does SAMBA or other CIFS servers running on AIX store the DOS attributes as
 extended attributes? I'm just wondering if the DOS file attribute view makes
 sense or not.


I'm by no means an expert in this area and just started to experiment
a little bit. While looking at tests like
'java/nio/file/attribute/DosFileAttributeView/Basic.java' I was
surprised to see that DosFileAttributeView can also be used on Linux.
While this makes sense for DOS file systems mounted into Linux which
map the DOS attributes to extended attributes on Unix it is probably
academic for file systems like ext2/3/4 which support extended
attributes as well. Unfortunately, tests like
DosFileAttributeView/Basic.java mostly test extended attributes on a
Linux files system because they only create and change files in /tmp
which is hardly ever a mounted DOS file system.

All that said, on AIX the JFS2 file system also supports extended
attributes (see
http://en.wikipedia.org/wiki/Extended_file_attributes). Hopefully the
CIFS client correctly maps the DOS attributes to extended user
attributes but I couldn't check that today because I couldn't find a
AIX box with CIFS client today. I'll try to find one tomorrow, but
perhaps Deven can already confirm this?

Following some more comments:

AixDosFileAttributeView.java
 - please replace ext3 by JFS2 in the comment as I'm not aware of
any ext3 support in AIX

AixFileStore.java
 - the detection if extended attributes are supported doesn't seem to
work. It seems like supportsFileAttributeView() has been just copied
from the corresponding Linux implementation but that won't work on
AIX. Please remove the ext3/4 stuff and do a real check (i.e. check if
the file system is JFS2 and if extended attributes have been enabled
on on the corresponding file system. See 'chfs -a ea=v2', 'man 2
getea', setea, ...).
 - the test DosFileAttributeView/Basic.java should succeed without
saying DOS file attribute not supported.

AixNativeDispatcher.java
AixNativeDispatcher.c
 - you define 'getAixMountEntries()', 'queryMountEntrySize()' and the
corresponding native implementations but they don't seemed to be used
anywhere. Please remove this dead code. If you need to get a list of
all mounted file systems you could use 'getmntctl()' which is already
there and does exactly that.

 I suspect you will need to update a number of tests to (like
 FileSystem/Basic.java) and Files/CopyAndMove.java) to ensure that this new
 code is exercised by the tests.


Yes, could you please elaborate how you have tested your
implementation until now?

Thank you and best regards,
Volker

 A minor comment but we usually use 4-space indentation in the library native
 code.

 -Alan

Re: RFR(s): 8035000: TEST_BUG: remove ActivationLibrary.DestroyThread and have callers call rmid.destroy() instead

2014-12-01 Thread Stuart Marks


Hi all, any reviewers for this one?

s'marks

On 11/24/14 6:26 PM, Stuart Marks wrote:

Hi all,

Here's another test cleanup fix. Basically this normalizes the shutdown/destroy
policy for rmid processes that are started by RMI's test library in support of a
few dozen of the RMI activation tests. The previous implementation was, well,
confused, and it had a redundant timing loop that can be subsumed by other code
in RMI's test library (JavaVM.waitFor).

I've listed this as a small changeset even though lots of files have changed.
In all but three or so of the files, the only change is from calling
ActivationLibrary.rmidCleanup(rmid) to calling rmid.cleanup() which is a
refactoring enabled by the cleanup. The real action is in the files
test/java/rmi/testlibrary/ActivationLibrary.java and RMID.java.

Webrev:

 http://cr.openjdk.java.net/~smarks/reviews/8035000/webrev.0/

Bug:

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

Thanks,

s'marks

Re: RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException

2014-12-01 Thread Mandy Chung


On 12/1/14 8:29 AM, Daniel Fuchs wrote:

8065552: setAccessible(true) on fields of Class may throw
 a SecurityException

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8065552/webrev.00/ 


Thanks for taking this on.  Looks okay in general.  The comment on 
Class.classLoader field - may simply say this field is filtered from 
reflection access, i.e. getDeclaredField will throw NoSuchFieldException.


ClassDeclaredFieldsTest.java
   copyright year should be 2014.
   @summary - this test also verifies that Class.classLoader final 
private field is hidden from reflection access.

   line 86: nit:  can just call new ThreadLocal

Mandy

Re: RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException


Hi Seán,

On 01/12/14 18:48, Seán Coffey wrote:

Looks fine to me Daniel. Thanks for handling it. I can work on the 7u
backport if necessary.


Thanks :-)


on the test side would it be worth testing all public classes available
(e.g in rt.jar ?) to ensure that
Field.setAccessible works as expected and that we don't hit this issue
again ? It might be some
what of a heavy test for jtreg inclusion though.


It could be worth a try. But let's wait until JEP 220 is in.

best regards,

-- daniel



regards,
Sean.

On 01/12/14 16:29, Daniel Fuchs wrote:

Hi,

Please find below a patch for:

8065552: setAccessible(true) on fields of Class may throw
 a SecurityException

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8065552/webrev.00/

Description of the problem:

The following test case passes on 8u20 but fails on 8u40 and above:

 public class Test {
 public static void main(String[] args) throws Throwable {
 for (Field f : Class.class.getDeclaredFields()) {
 f.setAccessible(true);
 }
 }
 }

The fix for JDK-6642881 introduced a new private field to Class, named
classloader, whose accessibility can never be modified (from the
default of non-accessible to accessible).

This issue manifests itself in Jython where, when the
Options.respectJavaAccessibility is false (by default it is true), a
SecurityException occurs when it tries to setAccessible(true) all
declared fields on Class:


https://hg.python.org/jython/file/tip/src/org/python/core/PyJavaType.java#l405


The SecurityException is lost in the noise of other exceptions as the
error propagates through the runtime. The observable symptom is
a NullPointerException which occurs when one tries to load the
Jython engine. With 8u40 it fails with exception:

java.lang.NullPointerException
at org.python.core.Py.recursiveIsInstance(Py.java:1861)
at org.python.core.Py.isInstance(Py.java:1828)
at org.python.core.__builtin__.isinstance(__builtin__.java:725)
at org.python.core.Py.displayException(Py.java:1009)
at org.python.core.PyException.printStackTrace(PyException.java:79)
at org.python.core.PyException.toString(PyException.java:98)
at org.apache.commons.logging.impl.SimpleLog.log(SimpleLog.java:329)
at org.apache.commons.logging.impl.SimpleLog.error(SimpleLog.java:525)
at org.apache.bsf.BSFManager.loadScriptingEngine(BSFManager.java:717)
...

The fix is to hide the field from reflection instead of simply
preventing it to be set as accessible.

best regards,

-- daniel

Re: RFR(s): 8035000: TEST_BUG: remove ActivationLibrary.DestroyThread and have callers call rmid.destroy() instead

Sorry Stuart,  I looked at this last week but guess I forgot to reply.

It looks fine and a nice clean up :-)

Best
Lance
On Dec 1, 2014, at 1:50 PM, Stuart Marks stuart.ma...@oracle.com wrote:

 Hi all, any reviewers for this one?
 
 s'marks
 
 On 11/24/14 6:26 PM, Stuart Marks wrote:
 Hi all,
 
 Here's another test cleanup fix. Basically this normalizes the 
 shutdown/destroy
 policy for rmid processes that are started by RMI's test library in support 
 of a
 few dozen of the RMI activation tests. The previous implementation was, well,
 confused, and it had a redundant timing loop that can be subsumed by other 
 code
 in RMI's test library (JavaVM.waitFor).
 
 I've listed this as a small changeset even though lots of files have 
 changed.
 In all but three or so of the files, the only change is from calling
 ActivationLibrary.rmidCleanup(rmid) to calling rmid.cleanup() which is a
 refactoring enabled by the cleanup. The real action is in the files
 test/java/rmi/testlibrary/ActivationLibrary.java and RMID.java.
 
 Webrev:
 
 http://cr.openjdk.java.net/~smarks/reviews/8035000/webrev.0/
 
 Bug:
 
 https://bugs.openjdk.java.net/browse/JDK-8035000
 
 Thanks,
 
 s'marks



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(S) : 8039953 : [TESTBUG] Timeout java/lang/invoke/MethodHandles/CatchExceptionTest.java

2014-12-01 Thread Igor Ignatyev


http://cr.openjdk.java.net/~iignatyev/8039953/webrev.01/

to have TimeLimitedRunner more general, I've added 'factor' argument to 
its ctor.


Thanks,
Igor

On 12/01/2014 02:39 PM, Igor Ignatyev wrote:

Paul/Vladimir,

thanks for review.

On 12/01/2014 01:29 PM, Vladimir Ivanov wrote:

On 12/1/14, 2:46 PM, Paul Sandoz wrote:

Hi Igor,

This looks ok.

I like how you have factored out things into TimeLimitedRunner. Do you
plan in a future patch to update lambda form test code that uses
similar functionality?

yes, I plan to update existing tests as well as backport
TimeLimitedRunner into hotspot and refactor hotspot tests.



Is the adjustment timeout *= 0.9 necessary? does reduction by 10%
make a difference?

It improves test stability (on highly loaded hosts and in heavy testing
modes), but I doubt it's the right way to go. I'd measure actual startup
time and adjust timeout value, but it seems jtreg doesn't provide such
info.

So, I'd leave it as is for now, file an RFE on jtreg and rewrite this
logic once support in jtreg is implemented.

Yes, I also don't think that it's a right way, but it's the best that we
can do now. Even if jtreg provides startup time, it won't help us w/ vm
shutdown time estimation.


Otherwise, looks good.

Best regards,
Vladimir Ivanov



Paul.

On Nov 29, 2014, at 5:36 PM, Igor Ignatyev igor.ignat...@oracle.com
wrote:


http://cr.openjdk.java.net/~iignatyev/8039953/webrev.00
98 lines changed: 93 ins; 3 del; 2 mod;

Hi all,

Please review the patch:

Problem:
on some configurations,
java/lang/invoke/MethodHandles/CatchExceptionTest.java can timeout
before all test cases are run

Fix:
interrupt test execution if it's not enough time to continue

bug : https://bugs.openjdk.java.net/browse/JDK-8039953
changes in testlibrary :
https://bugs.openjdk.java.net/browse/JDK-8066191
testing: locally

--
Igor



___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR(S) : 8039953 : [TESTBUG] Timeout java/lang/invoke/MethodHandles/CatchExceptionTest.java

2014-12-01 Thread Vladimir Ivanov


Looks good.

Best regards,
Vladimir Ivanov

On 12/1/14, 10:58 PM, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev/8039953/webrev.01/

to have TimeLimitedRunner more general, I've added 'factor' argument to
its ctor.

Thanks,
Igor

On 12/01/2014 02:39 PM, Igor Ignatyev wrote:

Paul/Vladimir,

thanks for review.

On 12/01/2014 01:29 PM, Vladimir Ivanov wrote:

On 12/1/14, 2:46 PM, Paul Sandoz wrote:

Hi Igor,

This looks ok.

I like how you have factored out things into TimeLimitedRunner. Do you
plan in a future patch to update lambda form test code that uses
similar functionality?

yes, I plan to update existing tests as well as backport
TimeLimitedRunner into hotspot and refactor hotspot tests.



Is the adjustment timeout *= 0.9 necessary? does reduction by 10%
make a difference?

It improves test stability (on highly loaded hosts and in heavy testing
modes), but I doubt it's the right way to go. I'd measure actual startup
time and adjust timeout value, but it seems jtreg doesn't provide such
info.

So, I'd leave it as is for now, file an RFE on jtreg and rewrite this
logic once support in jtreg is implemented.

Yes, I also don't think that it's a right way, but it's the best that we
can do now. Even if jtreg provides startup time, it won't help us w/ vm
shutdown time estimation.


Otherwise, looks good.

Best regards,
Vladimir Ivanov



Paul.

On Nov 29, 2014, at 5:36 PM, Igor Ignatyev igor.ignat...@oracle.com
wrote:


http://cr.openjdk.java.net/~iignatyev/8039953/webrev.00
98 lines changed: 93 ins; 3 del; 2 mod;

Hi all,

Please review the patch:

Problem:
on some configurations,
java/lang/invoke/MethodHandles/CatchExceptionTest.java can timeout
before all test cases are run

Fix:
interrupt test execution if it's not enough time to continue

bug : https://bugs.openjdk.java.net/browse/JDK-8039953
changes in testlibrary :
https://bugs.openjdk.java.net/browse/JDK-8066191
testing: locally

--
Igor



___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev

Re: RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException

2014-12-01 Thread Mandy Chung


On 12/1/14 10:54 AM, Daniel Fuchs wrote:



on the test side would it be worth testing all public classes available
(e.g in rt.jar ?) to ensure that
Field.setAccessible works as expected and that we don't hit this issue
again ? It might be some
what of a heavy test for jtreg inclusion though.


It could be worth a try. But let's wait until JEP 220 is in.



There will be a test to load all classes when we move to the modular image:
http://hg.openjdk.java.net/jigsaw/m2/jdk/file/26a4132a2fa9/test/jdk/internal/jimage/VerifyJimage.java

Mandy

Re: RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException


Hi Mandy,

Thanks for the review!

The new webrev is here:
http://cr.openjdk.java.net/~dfuchs/webrev_8065552/webrev.01/

The compiler doesn't want the diamond operator at line 86.
It says that it cannot infer the type argument.
So I left it unchanged. The rest of your comments are in.

best regards,

-- daniel


On 01/12/14 19:50, Mandy Chung wrote:

On 12/1/14 8:29 AM, Daniel Fuchs wrote:

8065552: setAccessible(true) on fields of Class may throw
 a SecurityException

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8065552/webrev.00/


Thanks for taking this on.  Looks okay in general.  The comment on
Class.classLoader field - may simply say this field is filtered from
reflection access, i.e. getDeclaredField will throw NoSuchFieldException.

ClassDeclaredFieldsTest.java
copyright year should be 2014.
@summary - this test also verifies that Class.classLoader final
private field is hidden from reflection access.
line 86: nit:  can just call new ThreadLocal

Mandy

Re: RFR: 8065870 Update JAX-WS RI integration to latest version (2.2.11-b141124.1933)

2014-12-01 Thread Stuart Marks

Hi Miran,

I'm pretty distant from the JAX-WS code, but I looked through all of the files
and most of the changes seem sensible. There are a few things that are
questionable though.

**
src/java.xml.ws/share/classes/com/sun/xml/internal/ws/api/streaming/XMLStreamReaderFactory.java

The catch-and-ignore of Throwable at line 565 seems questionable. Wouldn't it be
better to catch a few specific exception types that might be thrown from
setProperty()?

**
src/java.xml.ws/share/classes/com/sun/xml/internal/ws/util/resources/Messages_en.properties

The copyright year is changed from 2013 to 2012.

The unknown character replacement (line 277) is replaced with a '?', though I'm
not sure what's really happening here since webrev might be mishandling
non-ascii characters. If this is intended to be an ascii file, shouldn't the
replacement be a plain single quote (') ?

**
src/jdk.xml.bind/share/classes/com/sun/tools/internal/jxc/MessageBundle.properties

The version numbers in this file seem to be moved forward, but the copyright is
updated from 2014 to 2012. The same appears to be true of the localized versions
of this file.

**
src/jdk.xml.bind/share/classes/com/sun/tools/internal/xjc/MessageBundle.properties

**
src/jdk.xml.bind/share/classes/com/sun/tools/internal/xjc/generator/bean/MessageBundle.properties

** src/jdk.xml.ws/share/classes/com/sun/tools/internal/ws/version.properties

Nothing earth-shattering here. If you want to push this changeset and fix up
these issues later (if indeed they need to be fixed up), I'd be fine with that.

s'marks

On 11/27/14 3:27 AM, Miroslav Kos wrote:

Hi,
there is a bulk update of JAX-B/WS from upstream projects -
webrev: http://cr.openjdk.java.net/~mkos/8065870/jaxws.00/
more details in issue desc: https://bugs.openjdk.java.net/browse/JDK-8065870

Could I ask for a review?

It seems quite big (1126 lines changed) but there are just minor changes/fixes.

Thanks
Miran

Re: RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException

2014-12-01 Thread Mandy Chung


On 12/1/14 11:34 AM, Daniel Fuchs wrote:

Hi Mandy,

Thanks for the review!

The new webrev is here:
http://cr.openjdk.java.net/~dfuchs/webrev_8065552/webrev.01/


Looks good.

Mandy

Re: RFR: 8061950: Class.getMethods() exhibits quadratic time complexity

Looking at Peter's work here is still on my long TODO list, but I was
hoping first to get in my concurrency correctness fixes for core
reflection, which conflicts slightly...

On Sun, Nov 30, 2014 at 1:48 PM, Peter Levart peter.lev...@gmail.com wrote:
 Hi Joel,

 I managed to find some time to create some tests for this patch:

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

 Both MethodTable and HashArray unit tests are provided. I had to create a
 special TestProxy to access package-private classes from the tests.

 There are no changes to j.l.Class or j.l.r.Method from webrev.06 (I just
 re-based them to current tip).

 I also included the patch to StarInheritance test that I forgot to include
 in webrev.06.

 Comments inline...

 On 11/13/2014 10:39 AM, Joel Borggrén-Franck wrote:

 Hi Peter,

 As always, thanks for taking a look at this,

 This is quite big so in order to make this more approachable perhaps you
 can split the patch up into a series? If you start with creating the
 MethodTable interface, adding tests for how the interface should behave and
 refactored the current MethodArray into implementing that interface while
 also changing the lookup logic that would be easier to review.


 Well, there's not much to refactor in MethodArray when implementing
 MethodTable. They are two entirely different APIs with entirely different
 implementations.

 Then you could add different implementations of MethodTable (with
 additional unit tests) as follow up patches.


 You can view the MethodTable.SimpleArrayImpl as the basic implementation of
 the MethodTable API  and a replacement for MethodArray.
 MethodTable.HashArrayImpl is the alternative implementation for bigger
 sizes. The same unit tests are executed against both implementations.

 I am a bit concerned about the size and scope of the implementations. In
 general I would prefer if you targeted these to the precise need of core
 reflection today. If you want to expand these to general purpose data
 structures (even internal ones) I think that is a larger effort.


 I stripped HashArray and only left those methods that are needed to
 implement MethodTable API and execute the tests.

 In general I think the changes to Class are sound, but there is a slight
 change in the default method pruning. The call to removeLessSpecifics was
 deliberately placed at the end, so that all default methods would be present
 (the algorithm is sensitive to the order of pair vise comparisons). Since we
 add methods in a deterministic order, I think consolidate() as you go should
 result in the same set of methods, but I haven’t 100% convinced myself of
 this just yet.


 I think it results in the same methods. I haven't yet found an example where
 it would result in different set of methods. All JDK classes return same
 methods with current implementation as with patched one.

 Have you double checked that all methods returning root Method/Ctors are
 private?


 I checked all usages of private methods that I have changed and are now
 returning root objects and made sure those are copied before being exposed
 to the outside or being modified.

 Regards, Peter


 On 5 nov 2014, at 17:58, Peter Levart peter.lev...@gmail.com wrote:

 Here's new webrev:

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


 The optimizations made from webrev.05 are:

 - getMethod() skips construction of MethodTable if there are no
 (super)interfaces.
 - getMethods() returns just declared public methods if there are no
 superclass and no (super)interfaces.
 - comparing method parameter types is optimized by adding two methods to
 Method/LangReflectAccess/ReflectionFactory.

 New MethodTable implementation based on a linear-probe hash table is a
 space/garbage improvement. I took IdentityHashMap, removed unneeded stuff
 and modified it's API. The result is a HashArray. It's API is similar in
 function and form to java.util.Map, but doesn't use separate keys and
 values. An element of HashArray is a key and a value at the same time.
 Elements are always non-null, so the method return values are unambiguous.
 As HashArray is a linear-probe hash table and there are no Map.Entry objects
 involved, the underlying data structure is very simple and memory efficient.
 It is just a sparse array of elements with length that is always a power of
 two and larger than 3 * size / 2. It also features overriddable element
 equals/hashCode methods. I made it a separate generic class because I think
 it can find it's usage elsewhere (for example as a cannonicalizing cache).

 Since HashArray based MethodTable is more space-efficient I moved the
 line between simple array based and HashArray based MethodTable down to 20
 elements to minimize the worst-case scenario effect. Calling getMethods() on
 all rt.jar classes now constructs about 3/4 simple array based and 1/4
 HashArray based MethodTables.

 HashArray.java:

 I was hoping for a decent set of unit

Re: RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics

On Mon, Nov 24, 2014 at 1:29 PM, Aleksey Shipilev
aleksey.shipi...@oracle.com wrote:
 I think implies the effect of C++11 is too strong wording. related
 might be more appropriate.

I've been struggling to come up with better wording.  The latest webrev says

+ * Corresponds to C11 atomic_thread_fence(memory_order_acquire).

but I'm not totally happy with that either. Essentially equivalent to ?

Re: RFR 8060068 : Remove the static initializer block from DriverManager

2014-12-01 Thread Ulf Zibis


Hi Lance,

to me it's irritating, why there are 2 methods:
- initDriversIfNeeded()
- loadInitialDrivers()
I would combine both to one method.
In lines 90 + 92 there are double spaces.

-Ulf


Am 01.12.2014 um 17:52 schrieb Lance Andersen:

Hi all,

Looking for a review for this change to DriverManager to reduce the possibility on a 
deadlock on clinit. that I have been discussing with Mandy.


The change removes the static initializer block as well as the synchronized 
keyword from registerDriver.

The webrev can be found at 
http://cr.openjdk.java.net/%7Elancea/8060068/webrev.02/


Best,
Lance


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 8060068 : Remove the static initializer block from DriverManager

Hi Ulf,

thank you for the input and  suggestion


On Dec 1, 2014, at 3:27 PM, Ulf Zibis ulf.zi...@cosoco.de wrote:

 Hi Lance,
 
 to me it's irritating, why there are 2 methods:
 - initDriversIfNeeded()
 - loadInitialDrivers()
 I would combine both to one method.

Mandy had asked me previously about this and here was my reply

-
The reason I had the two methods was to further reduce contention checking to 
see if the drivers need to be loaded.  getConnection  gets called frequently so 
I thought that not having the initial check synchronized would be more efficient
-


I think the code gets harder to read if I have one large synchronized block  
assuming I move everything from loadInitialDrivers into the  existing 
synchronized block in initDriversIfNeeded.

 In lines 90 + 92 there are double spaces.

Thank you.
 
 -Ulf
 
 

Best,
Lance
 Am 01.12.2014 um 17:52 schrieb Lance Andersen:
 Hi all,
 
 Looking for a review for this change to DriverManager to reduce the 
 possibility on a deadlock on clinit. that I have been discussing with 
 Mandy.
 
 
 The change removes the static initializer block as well as the synchronized 
 keyword from registerDriver.
 
 The webrev can be found at 
 http://cr.openjdk.java.net/%7Elancea/8060068/webrev.02/
 
 
 Best,
 Lance
 
 
 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: [concurrency-interest] RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics

Hans,

(Thanks for your excellent work on C/C++ 11 and your eternal patience)

On Tue, Nov 25, 2014 at 11:15 AM, Hans Boehm bo...@acm.org wrote:
 It seems to me that a (dubiuously named) loadFence is intended to have
 essentially the same semantics as the (perhaps slightly less dubiously
 named) C++ atomic_thread_fence(memory_order_acquire), and a storeFence
 matches atomic_thread_fence(memory_order_release).  The C++ standard and,
 even more so, Mark Batty's work have a precise definition of what those mean
 in terms of implied synchronizes with relationships.

 It looks to me like this whole implementation model for volatiles in terms
 of fences is fundamentally doomed, and it probably makes more sense to get
 rid of it rather than spending time on renaming it (though we just did the
 latter in Android to avoid similar confusion about semantics).  It's

I would also like to see alignment to leverage the technical and
cultural work done on C11.  I would like to see Unsafe get
load-acquire and store-release methods and these should be used in
preference to fences where possible.  I'd like to see the C11 wording
reused as much as possible.  The meanings of the words acquire and
release are now owned by the C11 community and we should tag
along.

A better API for Unsafe would be

putOrdered - storeRelease
put - storeRelaxed
(ordinary volatile write) - store (default is sequential consistent)

etc ...

but the high cost of renaming methods in Unsafe probably makes this a
no-go, even though Unsafe is not a public API in theory.

At least the documentation of all the methods should indicate what the
memory effects and the corresponding C++11 memory model interpretation
is.

E.g. Unsafe.compareAndSwap should document the memory effects, i.e.
sequential consistency.

Unsafe doesn't currently have a readAcquire method (mirror of
putOrdered) probably because volatile read is _almost_ the same (but
not on ppc!).

 fundamentally incompatible with the way volatiles/atomics are intended to be
 implemented on ARMv8 (and Itanium).  Which I think fundamentally get this
 much closer to right than traditional fence-based ISAs.

 I'm no hardware architect, but fundamentally it seems to me that

 load x
 acquire_fence

 imposes a much more stringent constraint than

 load_acquire x

 Consider the case in which the load from x is an L1 hit, but a preceding
 load (from say y) is a long-latency miss.  If we enforce ordering by just
 waiting for completion of prior operation, the former has to wait for the
 load from y to complete; while the latter doesn't.  I find it hard to
 believe that this doesn't leave an appreciable amount of performance on the
 table, at least for some interesting microarchitectures.

I agree.  Fences should be used rarely.

Re: [concurrency-interest] RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics

David, Paul (i.e. Reviewers) and Doug,

I'd like to commit corrections so we make progress.

I think the current webrev is simple progress with the exception of my
attempt to translate volatiles into fences, which is marginal (but was
a good learning exercise for me).

Re: RFR - 8065552: setAccessible(true) on fields of Class may throw a SecurityException

2014-12-01 Thread Seán Coffey



On 01/12/2014 18:54, Daniel Fuchs wrote:

on the test side would it be worth testing all public classes available
(e.g in rt.jar ?) to ensure that
Field.setAccessible works as expected and that we don't hit this issue
again ? It might be some
what of a heavy test for jtreg inclusion though.


It could be worth a try. But let's wait until JEP 220 is in.
I'm not sure why JEP 220 would be a factor for now. JDK 8u and 7u are 
the production
JDKs for a while to come. Martin's testcase looks like a good approach. 
I just
ran it through the rt.jar classes and had it test for 
f.setAccessible(true); on all
getDeclaredFields in each class. Run time shouldn't be an issue I think 
: (6-7 seconds)



class load: 19624 classes, 5162.67 ms total time, 0.2631 ms per class
getMethods: 19624 classes, 1076.00 ms total time, 0.0548 ms per class


regards,
Sean.

Re: [concurrency-interest] RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics

2014-12-01 Thread Hans Boehm

Needless to say, I would clearly also like to see a simple correspondence.

But this does raise the interesting question of whether put/get and
store(..., memory_order_relaxed)/load(memory_order_relaxed) are intended to
have similar semantics.  I would guess not, in that the former don't
satisfy coherence; accesses to the same variable can be reordered as for
normal variable accesses, while the C++11/C11 variants do provide those
guarantees.  On most, but not all, architectures that's entirely a compiler
issue; the hardware claims to provide that guarantee.

This affects, for example, whether a variable that is only ever incremented
by one thread can appear to another thread to decrease in value.  Or if a
reference set to a non-null value exactly once can appear to change back to
null after appearing non-null.  In my opinion, it makes sense to always
provide coherence for atomics, since the overhead is small, and so are the
odds of getting code relying on non-coherent racing accesses correct.  But
for ordinary variables whose accesses are not intended to race the
trade-offs are very different.



Hans

On Mon, Dec 1, 2014 at 12:40 PM, Martin Buchholz marti...@google.com
wrote:

 Hans,

 (Thanks for your excellent work on C/C++ 11 and your eternal patience)

 On Tue, Nov 25, 2014 at 11:15 AM, Hans Boehm bo...@acm.org wrote:
  It seems to me that a (dubiuously named) loadFence is intended to have
  essentially the same semantics as the (perhaps slightly less dubiously
  named) C++ atomic_thread_fence(memory_order_acquire), and a storeFence
  matches atomic_thread_fence(memory_order_release).  The C++ standard and,
  even more so, Mark Batty's work have a precise definition of what those
 mean
  in terms of implied synchronizes with relationships.
 
  It looks to me like this whole implementation model for volatiles in
 terms
  of fences is fundamentally doomed, and it probably makes more sense to
 get
  rid of it rather than spending time on renaming it (though we just did
 the
  latter in Android to avoid similar confusion about semantics).  It's

 I would also like to see alignment to leverage the technical and
 cultural work done on C11.  I would like to see Unsafe get
 load-acquire and store-release methods and these should be used in
 preference to fences where possible.  I'd like to see the C11 wording
 reused as much as possible.  The meanings of the words acquire and
 release are now owned by the C11 community and we should tag
 along.

 A better API for Unsafe would be

 putOrdered - storeRelease
 put - storeRelaxed
 (ordinary volatile write) - store (default is sequential consistent)

 etc ...

 but the high cost of renaming methods in Unsafe probably makes this a
 no-go, even though Unsafe is not a public API in theory.

 At least the documentation of all the methods should indicate what the
 memory effects and the corresponding C++11 memory model interpretation
 is.

 E.g. Unsafe.compareAndSwap should document the memory effects, i.e.
 sequential consistency.

 Unsafe doesn't currently have a readAcquire method (mirror of
 putOrdered) probably because volatile read is _almost_ the same (but
 not on ppc!).

  fundamentally incompatible with the way volatiles/atomics are intended
 to be
  implemented on ARMv8 (and Itanium).  Which I think fundamentally get this
  much closer to right than traditional fence-based ISAs.
 
  I'm no hardware architect, but fundamentally it seems to me that
 
  load x
  acquire_fence
 
  imposes a much more stringent constraint than
 
  load_acquire x
 
  Consider the case in which the load from x is an L1 hit, but a preceding
  load (from say y) is a long-latency miss.  If we enforce ordering by just
  waiting for completion of prior operation, the former has to wait for the
  load from y to complete; while the latter doesn't.  I find it hard to
  believe that this doesn't leave an appreciable amount of performance on
 the
  table, at least for some interesting microarchitectures.

 I agree.  Fences should be used rarely.

Re: RFR: 8061950: Class.getMethods() exhibits quadratic time complexity

2014-12-01 Thread Peter Levart



On 12/01/2014 09:09 PM, Martin Buchholz wrote:

Looking at Peter's work here is still on my long TODO list, but I was
hoping first to get in my concurrency correctness fixes for core
reflection, which conflicts slightly...


No problem. I can rebase the patch after your fixes are in.

Regards, Peter


On Sun, Nov 30, 2014 at 1:48 PM, Peter Levart peter.lev...@gmail.com wrote:

Hi Joel,

I managed to find some time to create some tests for this patch:

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

Both MethodTable and HashArray unit tests are provided. I had to create a
special TestProxy to access package-private classes from the tests.

There are no changes to j.l.Class or j.l.r.Method from webrev.06 (I just
re-based them to current tip).

I also included the patch to StarInheritance test that I forgot to include
in webrev.06.

Comments inline...

On 11/13/2014 10:39 AM, Joel Borggrén-Franck wrote:

Hi Peter,

As always, thanks for taking a look at this,

This is quite big so in order to make this more approachable perhaps you
can split the patch up into a series? If you start with creating the
MethodTable interface, adding tests for how the interface should behave and
refactored the current MethodArray into implementing that interface while
also changing the lookup logic that would be easier to review.

Well, there's not much to refactor in MethodArray when implementing
MethodTable. They are two entirely different APIs with entirely different
implementations.


Then you could add different implementations of MethodTable (with
additional unit tests) as follow up patches.

You can view the MethodTable.SimpleArrayImpl as the basic implementation of
the MethodTable API  and a replacement for MethodArray.
MethodTable.HashArrayImpl is the alternative implementation for bigger
sizes. The same unit tests are executed against both implementations.


I am a bit concerned about the size and scope of the implementations. In
general I would prefer if you targeted these to the precise need of core
reflection today. If you want to expand these to general purpose data
structures (even internal ones) I think that is a larger effort.

I stripped HashArray and only left those methods that are needed to
implement MethodTable API and execute the tests.


In general I think the changes to Class are sound, but there is a slight
change in the default method pruning. The call to removeLessSpecifics was
deliberately placed at the end, so that all default methods would be present
(the algorithm is sensitive to the order of pair vise comparisons). Since we
add methods in a deterministic order, I think consolidate() as you go should
result in the same set of methods, but I haven’t 100% convinced myself of
this just yet.

I think it results in the same methods. I haven't yet found an example where
it would result in different set of methods. All JDK classes return same
methods with current implementation as with patched one.


Have you double checked that all methods returning root Method/Ctors are
private?

I checked all usages of private methods that I have changed and are now
returning root objects and made sure those are copied before being exposed
to the outside or being modified.

Regards, Peter



On 5 nov 2014, at 17:58, Peter Levart peter.lev...@gmail.com wrote:


Here's new webrev:

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


The optimizations made from webrev.05 are:

- getMethod() skips construction of MethodTable if there are no
(super)interfaces.
- getMethods() returns just declared public methods if there are no
superclass and no (super)interfaces.
- comparing method parameter types is optimized by adding two methods to
Method/LangReflectAccess/ReflectionFactory.

New MethodTable implementation based on a linear-probe hash table is a
space/garbage improvement. I took IdentityHashMap, removed unneeded stuff
and modified it's API. The result is a HashArray. It's API is similar in
function and form to java.util.Map, but doesn't use separate keys and
values. An element of HashArray is a key and a value at the same time.
Elements are always non-null, so the method return values are unambiguous.
As HashArray is a linear-probe hash table and there are no Map.Entry objects
involved, the underlying data structure is very simple and memory efficient.
It is just a sparse array of elements with length that is always a power of
two and larger than 3 * size / 2. It also features overriddable element
equals/hashCode methods. I made it a separate generic class because I think
it can find it's usage elsewhere (for example as a cannonicalizing cache).

Since HashArray based MethodTable is more space-efficient I moved the
line between simple array based and HashArray based MethodTable down to 20
elements to minimize the worst-case scenario effect. Calling getMethods() on
all rt.jar classes now constructs about 3/4 simple array based and 1/4
HashArray based MethodTables.

Re: RFR(s): 8035000: TEST_BUG: remove ActivationLibrary.DestroyThread and have callers call rmid.destroy() instead

2014-12-01 Thread Stuart Marks


Thanks Lance, I knew you'd come through. :-)


On 12/1/14 10:53 AM, Lance Andersen wrote:

Sorry Stuart,  I looked at this last week but guess I forgot to reply.

It looks fine and a nice clean up :-)

Best
Lance
On Dec 1, 2014, at 1:50 PM, Stuart Marks stuart.ma...@oracle.com 
mailto:stuart.ma...@oracle.com wrote:



Hi all, any reviewers for this one?

s'marks

On 11/24/14 6:26 PM, Stuart Marks wrote:

Hi all,

Here's another test cleanup fix. Basically this normalizes the shutdown/destroy
policy for rmid processes that are started by RMI's test library in support of a
few dozen of the RMI activation tests. The previous implementation was, well,
confused, and it had a redundant timing loop that can be subsumed by other code
in RMI's test library (JavaVM.waitFor).

I've listed this as a small changeset even though lots of files have changed.
In all but three or so of the files, the only change is from calling
ActivationLibrary.rmidCleanup(rmid) to calling rmid.cleanup() which is a
refactoring enabled by the cleanup. The real action is in the files
test/java/rmi/testlibrary/ActivationLibrary.java and RMID.java.

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8035000/webrev.0/ 
http://cr.openjdk.java.net/%7Esmarks/reviews/8035000/webrev.0/


Bug:

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

Thanks,

s'marks


http://oracle.com/us/design/oracle-email-sig-198324.gif
http://oracle.com/us/design/oracle-email-sig-198324.gifhttp://oracle.com/us/design/oracle-email-sig-198324.gif
http://oracle.com/us/design/oracle-email-sig-198324.gifLance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com mailto:lance.ander...@oracle.com

Re: [concurrency-interest] RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics

2014-12-01 Thread Doug Lea


On 12/01/2014 03:46 PM, Martin Buchholz wrote:

David, Paul (i.e. Reviewers) and Doug,

I'd like to commit corrections so we make progress.


The current one looks OK to me.
(http://cr.openjdk.java.net/~martin/webrevs/openjdk9/fence-intrinsics/)

-Doug




I think the current webrev is simple progress with the exception of my
attempt to translate volatiles into fences, which is marginal (but was
a good learning exercise for me).

Re: [concurrency-interest] RFR: 8065804: JEP 171: Clarifications/corrections for fence intrinsics