date:20141202

On Nov 25, 2014, at 3:06 AM, Martin Buchholz marti...@google.com wrote:

 I tried to address all the known problems in sun/reflect (except for
 the performance ones), including the ones in Peter's webrev (although
 it now looks quite different).
 
 I broke down and switched to using AtomicReferenceFieldUpdaters for
 all lazily initialized operations,
 like I had been thinking of doing.
 
 For the weird classes where we need to lazily switch the
 implementation of bounds, I store in an Object[] and cas to update.
 
 Now that we're using atomic updaters, it's hard to have any relaxed
 operations unless we also introduce Unsafe.  My current thinking is
 this code is not performance critical enough to do that sort of
 brittle thing.  Volatile reads are either already very cheap or are
 likely to get cheaper over time.  The code using updaters seems
 robust, efficient, and deadlock free, unlike some other code in the
 JDK.  Using updaters means there will never be an extremely rare bug
 due to different objects being returned from a reflection method.
 
 http://cr.openjdk.java.net/~martin/webrevs/openjdk9/core-reflection-more-safety/
 

I am not convinced about the use of CAS. If we can get away with just volatile 
fields I think the code is simpler. Are you concerned there are cases of 
identity and mutability?

All other aspects look good.

Paul.

RFR 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom

Hi,

Please find below a patch to remove the networking code computing a seed in 
ThreadLocal/SplittableRandom.

We thought it a good idea at the time :-) but subsequently on certain platforms 
this results in very high initalization costs that can propagate to other 
classes such as ConcurrentSkipList*.

The short-term solution is to remove this code and fallback just using current 
system time. This needs to be back-ported to 8u40.

A longer term solution is to provide a simple public API to get access to some 
seed bytes that is optimal for the underlying platform, for example, based on 
Peter's investigations. For linux /dev/urandom is sufficient as a source of 
bytes. The main problem seems to be Windows. It would also be nice to back-port 
to say 8u60 using a private API and update TLR/SR.

Paul.

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

diff -r 1b599b4755bd src/java.base/share/classes/java/util/SplittableRandom.java
--- a/src/java.base/share/classes/java/util/SplittableRandom.java   Mon Dec 
01 17:59:39 2014 -0800
+++ b/src/java.base/share/classes/java/util/SplittableRandom.java   Tue Dec 
02 10:53:47 2014 +0100
@@ -237,34 +237,7 @@
 s = (s  8) | ((long)(seedBytes[i])  0xffL);
 return s;
 }
-long h = 0L;
-try {
-EnumerationNetworkInterface ifcs =
-NetworkInterface.getNetworkInterfaces();
-boolean retry = false; // retry once if getHardwareAddress is null
-while (ifcs.hasMoreElements()) {
-NetworkInterface ifc = ifcs.nextElement();
-if (!ifc.isVirtual()) { // skip fake addresses
-byte[] bs = ifc.getHardwareAddress();
-if (bs != null) {
-int n = bs.length;
-int m = Math.min(n  1, 4);
-for (int i = 0; i  m; ++i)
-h = (h  16) ^ (bs[i]  8) ^ bs[n-1-i];
-if (m  4)
-h = (h  8) ^ bs[n-1-m];
-h = mix64(h);
-break;
-}
-else if (!retry)
-retry = true;
-else
-break;
-}
-}
-} catch (Exception ignore) {
-}
-return (h ^ mix64(System.currentTimeMillis()) ^
+return (mix64(System.currentTimeMillis()) ^
 mix64(System.nanoTime()));
 }
 
diff -r 1b599b4755bd 
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java
--- a/src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java   
Mon Dec 01 17:59:39 2014 -0800
+++ b/src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java   
Tue Dec 02 10:53:47 2014 +0100
@@ -147,34 +147,7 @@
 s = (s  8) | ((long)(seedBytes[i])  0xffL);
 return s;
 }
-long h = 0L;
-try {
-EnumerationNetworkInterface ifcs =
-NetworkInterface.getNetworkInterfaces();
-boolean retry = false; // retry once if getHardwareAddress is null
-while (ifcs.hasMoreElements()) {
-NetworkInterface ifc = ifcs.nextElement();
-if (!ifc.isVirtual()) { // skip fake addresses
-byte[] bs = ifc.getHardwareAddress();
-if (bs != null) {
-int n = bs.length;
-int m = Math.min(n  1, 4);
-for (int i = 0; i  m; ++i)
-h = (h  16) ^ (bs[i]  8) ^ bs[n-1-i];
-if (m  4)
-h = (h  8) ^ bs[n-1-m];
-h = mix64(h);
-break;
-}
-else if (!retry)
-retry = true;
-else
-break;
-}
-}
-} catch (Exception ignore) {
-}
-return (h ^ mix64(System.currentTimeMillis()) ^
+return (mix64(System.currentTimeMillis()) ^
 mix64(System.nanoTime()));
 }

Re: RFR 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom


On 02/12/2014 10:02, Paul Sandoz wrote:

Hi,

Please find below a patch to remove the networking code computing a seed in 
ThreadLocal/SplittableRandom.

We thought it a good idea at the time :-) but subsequently on certain platforms 
this results in very high initalization costs that can propagate to other 
classes such as ConcurrentSkipList*.

The short-term solution is to remove this code and fallback just using current 
system time. This needs to be back-ported to 8u40.

A longer term solution is to provide a simple public API to get access to some 
seed bytes that is optimal for the underlying platform, for example, based on 
Peter's investigations. For linux /dev/urandom is sufficient as a source of 
bytes. The main problem seems to be Windows. It would also be nice to back-port 
to say 8u60 using a private API and update TLR/SR.

Paul.
The approach seems pragmatic and the change looks okay to me (you might 
have some unused imports to removed too).


-Alan.

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


On 01/12/2014 13:37, Jan Lahoda wrote:

:



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/

webrev.01 looks good to me.

-Alan

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


On 01/12/2014 21:09, Seán Coffey wrote:


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.
I assume this is only because the test would need changes to use the 
jrt file system to locate all class files (and infer the type names).


-Alan.

Re: RFR 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom


On Dec 2, 2014, at 11:10 AM, Alan Bateman alan.bate...@oracle.com wrote:

 On 02/12/2014 10:02, Paul Sandoz wrote:
 Hi,
 
 Please find below a patch to remove the networking code computing a seed in 
 ThreadLocal/SplittableRandom.
 
 We thought it a good idea at the time :-) but subsequently on certain 
 platforms this results in very high initalization costs that can propagate 
 to other classes such as ConcurrentSkipList*.
 
 The short-term solution is to remove this code and fallback just using 
 current system time. This needs to be back-ported to 8u40.
 
 A longer term solution is to provide a simple public API to get access to 
 some seed bytes that is optimal for the underlying platform, for example, 
 based on Peter's investigations. For linux /dev/urandom is sufficient as a 
 source of bytes. The main problem seems to be Windows. It would also be nice 
 to back-port to say 8u60 using a private API and update TLR/SR.
 
 Paul.
 The approach seems pragmatic and the change looks okay to me

Thanks.


 (you might have some unused imports to removed too).
 

Doh! yes, removed.

Paul.

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


On Dec 2, 2014, at 1:58 AM, Doug Lea d...@cs.oswego.edu wrote:

 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/)
 

Same here, looks ok.

I anticipate we will be revisiting this area with the enhanced volatiles [1] 
work and related JMM updates, where there will be a public API for low-level 
enhanced field/array access [2].

As you rightly observed Unsafe does not currently have a get/read-acquire 
method. Implementations of [2] currently emulate that with a relaxed read + 
Unsafe.loadFence. It's something we need to add.

Paul.

[1] http://openjdk.java.net/jeps/193
[2] 
http://hg.openjdk.java.net/valhalla/valhalla/jdk/file/2d4531473a89/src/java.base/share/classes/java/lang/invoke/VarHandle.java

Re: RFR 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom

2014-12-02 Thread Doug Lea


On 12/02/2014 05:02 AM, Paul Sandoz wrote:

Hi,

Please find below a patch to remove the networking code computing a seed in 
ThreadLocal/SplittableRandom.



Thanks for pushing this through.
(I also adjusted 166 sources accordingly.)
One more touch-up for SplittableRandom is to remove mention
of network interface in internal comments:

***
*** 140,150 
   * other cases, this split must be performed in a thread-safe
   * manner, so we use an AtomicLong to represent the seed rather
   * than use an explicit SplittableRandom. To bootstrap the
!  * defaultGen, we start off using a seed based on current time and
!  * network interface address unless the java.util.secureRandomSeed
!  * property is set. This serves as a slimmed-down (and insecure)
!  * variant of SecureRandom that also avoids stalls that may occur
!  * when using /dev/random.
   *
   * It is a relatively simple matter to apply the basic design here
   * to use 128 bit seeds. However, emulating 128bit arithmetic and
--- 139,148 
   * other cases, this split must be performed in a thread-safe
   * manner, so we use an AtomicLong to represent the seed rather
   * than use an explicit SplittableRandom. To bootstrap the
!  * defaultGen, we start off using a seed based on current time
!  * unless the java.util.secureRandomSeed property is set. This
!  * serves as a slimmed-down (and insecure) variant of SecureRandom
!  * that also avoids stalls that may occur when using /dev/random.
   *
   * It is a relatively simple matter to apply the basic design here
   * to use 128 bit seeds. However, emulating 128bit arithmetic and
***

Re: RFR 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom


On Dec 2, 2014, at 1:17 PM, Doug Lea d...@cs.oswego.edu wrote:

 On 12/02/2014 05:02 AM, Paul Sandoz wrote:
 Hi,
 
 Please find below a patch to remove the networking code computing a seed in 
 ThreadLocal/SplittableRandom.
 
 
 Thanks for pushing this through.
 (I also adjusted 166 sources accordingly.)
 One more touch-up for SplittableRandom is to remove mention
 of network interface in internal comments:
 
 ***
 *** 140,150 
   * other cases, this split must be performed in a thread-safe
   * manner, so we use an AtomicLong to represent the seed rather
   * than use an explicit SplittableRandom. To bootstrap the
 !  * defaultGen, we start off using a seed based on current time and
 !  * network interface address unless the java.util.secureRandomSeed
 !  * property is set. This serves as a slimmed-down (and insecure)
 !  * variant of SecureRandom that also avoids stalls that may occur
 !  * when using /dev/random.
   *
   * It is a relatively simple matter to apply the basic design here
   * to use 128 bit seeds. However, emulating 128bit arithmetic and
 --- 139,148 
   * other cases, this split must be performed in a thread-safe
   * manner, so we use an AtomicLong to represent the seed rather
   * than use an explicit SplittableRandom. To bootstrap the
 !  * defaultGen, we start off using a seed based on current time
 !  * unless the java.util.secureRandomSeed property is set. This
 !  * serves as a slimmed-down (and insecure) variant of SecureRandom
 !  * that also avoids stalls that may occur when using /dev/random.
   *
   * It is a relatively simple matter to apply the basic design here
   * to use 128 bit seeds. However, emulating 128bit arithmetic and
 ***
 

Thanks, updated my copy,
Paul.

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

2014-12-02 Thread Miroslav Kos

Hi Stuart,
minor stuff like invalid characters and copyright years I can fix before
push, the rest will leave for next integration which should be soon.

Thanks!
Miran

On 01/12/14 20:35, Stuart Marks wrote:

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 8065998: Avoid use of _ as a one-character identifier

2014-12-02 Thread Jan Lahoda


Thanks to all for the comments!

Jan

On 2.12.2014 11:15, Alan Bateman wrote:

On 01/12/2014 13:37, Jan Lahoda wrote:

:



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/

webrev.01 looks good to me.

-Alan

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

2014-12-02 Thread Vladimir Ivanov


Thanks, Paul!
Updated webrev in place.


On Dec 1, 2014, at 5:58 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com 
wrote:


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



That LambdaFormEditor.putInCache method just got more gnarly :-)

Generally looks good.

In src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java

  366 lambdaForm.transformCache = c = ta;

Do you need to set c? It's a local variable and by this point the method 
should return rather than loop.
I did it mostly as a cleanup. Now I think that it doesn't help much. 
Removed (+ similar change in another place).



In test/java/lang/invoke/LFCaching/LambdaFormTestCase.java

   55 private static final ListGarbageCollectorMXBean gcInfo;
   56
   57 private static long gcCount() {
   58 return gcInfo.stream()
   59 .map(GarbageCollectorMXBean::getCollectionCount)
   60 .reduce(0L, Long::sum);
   61 }

You can do:

   gcInfo.stream().mapToLong(GarbageCollectorMXBean::getCollectionCount).sum();

Good point. Updated.

Best regards,
Vladimir Ivanov



Paul.


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
___
mlvm-dev mailing list
mlvm-...@openjdk.java.net
http://mail.openjdk.java.net/mailman/listinfo/mlvm-dev




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

Re: RFR 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom


On Dec 2, 2014, at 5:42 PM, Peter Levart peter.lev...@gmail.com wrote:

 On 12/02/2014 11:02 AM, Paul Sandoz wrote:
 Hi,
 
 Please find below a patch to remove the networking code computing a seed in 
 ThreadLocal/SplittableRandom.
 
 We thought it a good idea at the time :-) but subsequently on certain 
 platforms this results in very high initalization costs that can propagate 
 to other classes such as ConcurrentSkipList*.
 
 The short-term solution is to remove this code and fallback just using 
 current system time. This needs to be back-ported to 8u40.
 
 A longer term solution is to provide a simple public API to get access to 
 some seed bytes that is optimal for the underlying platform, for example, 
 based on Peter's investigations. For linux /dev/urandom is sufficient as a 
 source of bytes. The main problem seems to be Windows. It would also be nice 
 to back-port to say 8u60 using a private API and update TLR/SR.
 
 Hi,
 
 Here's a proof of concept for an API that just delegates to system-provided 
 cryptographically secure (as declared by the system(s)) pseudo random 
 number generator:
 
 http://cr.openjdk.java.net/~plevart/jdk9-dev/SystemRandom/webrev.01/
 

That's rather good. I updated the bug with a link to your email.

Paul.

Re: Packing 2 data points into 1 field in ThreadPoolExecutor

2014-12-02 Thread Alex Yursha

Thanks for all responses. All this is much clearer now.

Am I right that all this state packing is for better performance only and the 
same behaviour can be achieved by using explicit locks?

 On Dec 2, 2014, at 05:32, David Holmes david.hol...@oracle.com wrote:
 
 On 2/12/2014 3:44 AM, Alex Yursha wrote:
 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?
 
 AtomicFieldUpdater can apply atomic operations to plain (volatile) int and 
 long fields. The Unsafe API can also be used to do it more performantly.
 
 David
 
 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: [9, 8u40] RFR (M): 8057020: LambdaForm caches should support eviction


On Dec 2, 2014, at 5:20 PM, Vladimir Ivanov vladimir.x.iva...@oracle.com 
wrote:

 Thanks, Paul!
 Updated webrev in place.

+1.

Paul.

Re: RFR 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom


On 02/12/2014 16:42, Peter Levart wrote:

:

Here's a proof of concept for an API that just delegates to 
system-provided cryptographically secure (as declared by the 
system(s)) pseudo random number generator:


http://cr.openjdk.java.net/~plevart/jdk9-dev/SystemRandom/webrev.01/

The API looks reasonable to me too, I'm just not sure that java.util is 
the right place and whether it needs to be a Java SE API.


-Alan

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

2014-12-02 Thread Stuart Marks

Great, this sounds fine.

s'marks

On 12/2/14 6:03 AM, Miroslav Kos wrote:

Hi Stuart,
minor stuff like invalid characters and copyright years I can fix before push,
the rest will leave for next integration which should be soon.

Thanks!
Miran

On 01/12/14 20:35, Stuart Marks wrote:

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

2014-12-02 Thread Stuart Marks


Hi Lance,

Overall, looks fine.

Typo earleir at line 569.

I agree with having two separate init methods, since initDriversIfNeeded() 
conveniently separates the (safe) double-checked locking idiom from the actual 
initialization legwork in loadInitialDrivers().


I'm not entirely convinced, however :-), that DCL is necessary, but I could see 
that it might be helpful if some of these operations are called frequently. 
Sorry if this had been discussed previously.


s'marks



On 12/1/14 12:39 PM, Lance Andersen wrote:

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

Hi Stuart,
On Dec 2, 2014, at 1:35 PM, Stuart Marks stuart.ma...@oracle.com wrote:

 Hi Lance,
 
 Overall, looks fine.

Thank you for the review
 
 Typo earleir at line 569.

fixed
 
 I agree with having two separate init methods, since initDriversIfNeeded() 
 conveniently separates the (safe) double-checked locking idiom from the 
 actual initialization legwork in loadInitialDrivers().
 
 I'm not entirely convinced, however :-), that DCL is necessary, but I could 
 see that it might be helpful if some of these operations are called 
 frequently. Sorry if this had been discussed previously.

getConnection in particular can get called often in some environments so I 
thought using DCL would be the safe way to go.

Best,
Lance

 
 s'marks
 
 
 
 On 12/1/14 12:39 PM, Lance Andersen wrote:
 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
 
 
 



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: 8065991: LogManager unecessarily calls JavaAWTAccess from within a critical section


Hi Daniel,

On 11/26/14 9:11 AM, Daniel Fuchs wrote:

Hi,

Please find below a patch for:

8065991: LogManager unecessarily calls JavaAWTAccess from within
 a critical section
https://bugs.openjdk.java.net/browse/JDK-8065991

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


line 465 can be moved together with line 461.

For the logger created by EventQueue in non-applet env, do we
expect JavaAWTAccess.getAppletContext to return null (as it
should only find the main AppContext)?

As the deadlock reported from JDK-8065709, it hits the race when
main AppContext object has been created and the numAppContexts counter
has been incremented but mainAppContext is not set in which case
even with this patch, it will still call AppContext.getAppContext()
and hit that deadlockon sun.awt.AppContext$GetAppContextLock.

Should JavaAWTAccess.getAppletContext simply return null (not calling
getAppContext) if it determines the main AppContext is being initialized?

Mandy

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


On 12/1/14 8:52 AM, Lance Andersen wrote:

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/


initDriversIfNeeded() is called to ensure that the drivers are registered.
So it may be better to have a getter method to ensure driver classes are
loaded as well as return the registeredDrivers copy-on-write-arraylist.
i.e. you could rename initDriversIfNeeded to getRegisteredDrivers and
replace
  for(DriverInfo aDriver : registeredDrivers) {

with
  for(DriverInfo aDriver : getRegisteredDrivers())

line 564-567: this comment should belong to loadInitialDrivers right?
Nit line 567 - not align (one more space to the right needed)

Is driversSync object necessary?  Can you make loadInitialDrivers be
a synchronized method and move line 575-581 except 578 to the
loadInitialDrivers method (driversInitialized is volatile which is
good).

Mandy

Re: RFR: 8065991: LogManager unecessarily calls JavaAWTAccess from within a critical section

2014-12-02 Thread Daniel Fuchs


Hi Mandy,

On 02/12/14 20:24, Mandy Chung wrote:

Hi Daniel,

On 11/26/14 9:11 AM, Daniel Fuchs wrote:

Hi,

Please find below a patch for:

8065991: LogManager unecessarily calls JavaAWTAccess from within
 a critical section
https://bugs.openjdk.java.net/browse/JDK-8065991

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


line 465 can be moved together with line 461.


Good point. It should not matter much however.


For the logger created by EventQueue in non-applet env, do we
expect JavaAWTAccess.getAppletContext to return null (as it
should only find the main AppContext)?


The mainAppContext is equivalent to 'null' for us.


As the deadlock reported from JDK-8065709, it hits the race when
main AppContext object has been created and the numAppContexts counter
has been incremented but mainAppContext is not set in which case
even with this patch, it will still call AppContext.getAppContext()
and hit that deadlockon sun.awt.AppContext$GetAppContextLock.


I'm not sure I follow. I don't think it will. Only when the
LogManager has not yet been initialized - because getUserContext()
ends up being called inside the lock held by ensureLogManagerInitialized().

This patch doesn't pretend to solve JDK-8065709. This patch simply
removes one unnecessary lock around getAppContext().


Should JavaAWTAccess.getAppletContext simply return null (not calling
getAppContext) if it determines the main AppContext is being initialized?


I'd prefer not to touch this code unless it's proven wrong.

To solve the actual deadlock - several options have been proposed - and
I think they should be preferred - or at least experimented.

best regards,

-- daniel



Mandy

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

Hi Mandy,

Thank you for the review, please see below

On Dec 2, 2014, at 2:56 PM, Mandy Chung mandy.ch...@oracle.com wrote:

 On 12/1/14 8:52 AM, Lance Andersen wrote:
 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/
 
 initDriversIfNeeded() is called to ensure that the drivers are registered. 
This is a one time operation  only those specified via the system property of 
supporting ServiceLoader.  

A user could invoke Class.forName(my java.sql.Driver impl) and cause a driver 
to be registered at any time
 So it may be better to have a getter method to ensure driver classes are 
 loaded as well as return the registeredDrivers copy-on-write-arraylist.
 i.e. you could rename initDriversIfNeeded to getRegisteredDrivers and
 replace
   for(DriverInfo aDriver : registeredDrivers) {
 
 with 
   for(DriverInfo aDriver : getRegisteredDrivers()) 
I can do that if you think that is a cleaner approach?  I am flexible :-)
 
 line 564-567: this comment should belong to loadInitialDrivers right?
I had left it there as both methods kind of flow together, but I moved it per 
your suggestion.  Thank you
 Nit line 567 - not align (one more space to the right needed)
fixed
 
 Is driversSync object necessary? 
I am not 100% sure but I thought having its own monitor would further reduce 
the  contention possibility and given this was only reported in this one case, 
I was playing it safe :-).

  Can you make loadInitialDrivers be
 a synchronized method and move line 575-581 except 578 to the 
 loadInitialDrivers method (driversInitialized is volatile which is 
 good).
That is doable, I just thought keeping the DCL code together makes it easier to 
read and decided to have its own monitor.  If you feel we should go this way, I 
will, just let me know 

Best,
Lance
 
 Mandy



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

Help us with choosing issue BG JUG adopt hackathon

2014-12-02 Thread Ivan St. Ivanov

Hello core libs developers!

I am Ivan from the Bulgarian Java User Group. We are adopting OpenJDK for
quite some time in our JUG. So far we have done some sessions in our
community as well as on conferences in Bulgaria and neighboring countries,
where we just talked about the project and the adoption topic. We also did
some hackathons, where we showed the people how they can download, build,
change and use the changed images of the latest version of OpenJDK. On the
last Java2Days conference in Sofia we even went further and together with
Mani Sarkar from LJC we did Jigsaw project hands on lab, where besides
explaining the roadmap and JEPs for the modularity, we built and ran JBoss
Forge with the image that we built from the latest source from jigsaw/m2
repository!

And now we decided that we want to go one step further. We think that we
are ready for our first real contribution to OpenJDK. Of course, we want to
start small, we don't plan to implement project Valhalla in a day ;)

We took our time and analyzed the open issues that we found in the OpenJDK
JIRA. We selected a few items and before picking one for our hackathon on
11th December, we decided to ask for your opinion. So, this is what we
found appropriate:

https://bugs.openjdk.java.net/browse/JDK-5050783
*Throwable convenience method: String getStackTraceString()*
This stays open for quite some time. Maybe we could add a default method to
java.lang.Throwable or static convenience method to another class?

https://bugs.openjdk.java.net/browse/JDK-6201180
*(coll) Generics and collections support for generic arrays*
We believe that this issue was reported ahead of its time :). It basically
proposes adding new methods to the Collection interface, but the proposal
came in 2004! Now with default methods it is finally possible. We would
argue as well with the syntax: instead of addAll(? extends E[] array), we
would propose to have addAll(E... array) method (the same applies for the
other three proposed new methods as well).

https://bugs.openjdk.java.net/browse/JDK-4974893
*(ch) Retrofit scatter/gather interfaces with varargs where possible*
I guess we need to discuss this more. Especially the comment I think this
job must be generalized to the whole API. We know that Joe Darcy and the
team already ran some effort on cleaning up things like that, but if you
still know about generifying/varargsifying/coinifying/labdaifying (wow! :))
internal APIs, we would be more than happy to help.

We that, I would conclude the list. Do you think that we can work on one of
these next Thursday? If not, what else would you propose?

We really hope that we'll be able to do such hackathons once per two months
at least. And our next goals is to participate in Jigsaw once it gets to
its final straight!

Thanks and regards,
Ivan

Re: RFR: 8065991: LogManager unecessarily calls JavaAWTAccess from within a critical section


On 12/2/14 12:26 PM, Daniel Fuchs wrote:

Hi Mandy,

On 02/12/14 20:24, Mandy Chung wrote:

Hi Daniel,

On 11/26/14 9:11 AM, Daniel Fuchs wrote:

Hi,

Please find below a patch for:

8065991: LogManager unecessarily calls JavaAWTAccess from within
 a critical section
https://bugs.openjdk.java.net/browse/JDK-8065991

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


line 465 can be moved together with line 461.


Good point. It should not matter much however.



For non-applet case, it will also be null (just a fast path).


For the logger created by EventQueue in non-applet env, do we
expect JavaAWTAccess.getAppletContext to return null (as it
should only find the main AppContext)?


The mainAppContext is equivalent to 'null' for us.


As the deadlock reported from JDK-8065709, it hits the race when
main AppContext object has been created and the numAppContexts counter
has been incremented but mainAppContext is not set in which case
even with this patch, it will still call AppContext.getAppContext()
and hit that deadlockon sun.awt.AppContext$GetAppContextLock.


I'm not sure I follow. I don't think it will. Only when the
LogManager has not yet been initialized - because getUserContext()
ends up being called inside the lock held by 
ensureLogManagerInitialized().




I was referring to the original deadlock.


This patch doesn't pretend to solve JDK-8065709. This patch simply
removes one unnecessary lock around getAppContext().


I understand.



Should JavaAWTAccess.getAppletContext simply return null (not calling
getAppContext) if it determines the main AppContext is being 
initialized?


I'd prefer not to touch this code unless it's proven wrong.



That's fair.  The real fix is to fix the synchronization issues with 
AppContext.

To solve the actual deadlock - several options have been proposed - and
I think they should be preferred - or at least experimented.



It's fine with me.  Your patch looks good to go.
Mandy

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


On 12/2/14 12:30 PM, Lance Andersen wrote:



So it may be better to have a getter method to ensure driver classes are
loaded as well as return the registeredDrivers copy-on-write-arraylist.
i.e. you could rename initDriversIfNeeded to getRegisteredDrivers and
replace
   for(DriverInfo aDriver : registeredDrivers) {

with
   for(DriverInfo aDriver : getRegisteredDrivers())

I can do that if you think that is a cleaner approach?  I am flexible :-)


I think so.  It wasn't obvious to me at the beginning when you need to 
ensure drivers are initialized and whether any place needs to call 
initDirversIfNeeded.  IMO, having the getter method helps the reader 
easier to understand and future maintainence.



line 564-567: this comment should belong to loadInitialDrivers right?
I had left it there as both methods kind of flow together, but I moved 
it per your suggestion.  Thank you

Nit line 567 - not align (one more space to the right needed)

fixed

Is driversSync object necessary?
I am not 100% sure but I thought having its own monitor would further 
reduce the  contention possibility and given this was only reported in 
this one case, I was playing it safe :-).



Can you make loadInitialDrivers be
a synchronized method and move line 575-581 except 578 to the
loadInitialDrivers method (driversInitialized is volatile which is
good).
That is doable, I just thought keeping the DCL code together makes it 
easier to read and decided to have its own monitor.  If you feel we 
should go this way, I will, just let me know




The lock is to make sure the drivers are loaded once.  AFAICT, there is 
no other method locking DriverManager class except getConnection and I 
don't see any issue.


I forgot to include this comment in my previous reply:

/*
 * When callerCl is null, we should check the application's
 * (which is invoking this class indirectly)
 * classloader, so that the JDBC driver class outside rt.jar
 * can be loaded from here.
 */
ClassLoader callerCL = caller != null ? caller.getClassLoader() : null;
synchronized(DriverManager.class) {
// synchronize loading of the correct classloader.
if (callerCL == null) {
callerCL = Thread.currentThread().getContextClassLoader();
}
}

I don't understand why this needs to synchronize in order to get TCCL.
You are getting the TCCL of the current thread and callerCL is a local
variable. So I think this can be removed.

Mandy

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


On Dec 2, 2014, at 4:12 PM, Mandy Chung mandy.ch...@oracle.com wrote:

 On 12/2/14 12:30 PM, Lance Andersen wrote:
 
 So it may be better to have a getter method to ensure driver classes are 
 loaded as well as return the registeredDrivers copy-on-write-arraylist.
 i.e. you could rename initDriversIfNeeded to getRegisteredDrivers and
 replace
   for(DriverInfo aDriver : registeredDrivers) {
 
 with 
   for(DriverInfo aDriver : getRegisteredDrivers()) 
 I can do that if you think that is a cleaner approach?  I am flexible :-)
 
 I think so.  It wasn't obvious to me at the beginning when you need to ensure 
 drivers are initialized and whether any place needs to call 
 initDirversIfNeeded.  IMO, having the getter method helps the reader easier 
 to understand and future maintenance.

OK, thank you, done
 
 line 564-567: this comment should belong to loadInitialDrivers right?
 I had left it there as both methods kind of flow together, but I moved it 
 per your suggestion.  Thank you
 Nit line 567 - not align (one more space to the right needed)
 fixed
 Is driversSync object necessary? 
 I am not 100% sure but I thought having its own monitor would further reduce 
 the  contention possibility and given this was only reported in this one 
 case, I was playing it safe :-).
 
 Can you make loadInitialDrivers be
 a synchronized method and move line 575-581 except 578 to the 
 loadInitialDrivers method (driversInitialized is volatile which is 
 good).
 That is doable, I just thought keeping the DCL code together makes it easier 
 to read and decided to have its own monitor.  If you feel we should go this 
 way, I will, just let me know 
 
 
 The lock is to make sure the drivers are loaded once. 
correct
 AFAICT, there is no other method locking DriverManager class except 
 getConnection and I don't see any issue.
Ok
 
 I forgot to include this comment in my previous reply:
 /*
  * When callerCl is null, we should check the application's
  * (which is invoking this class indirectly)
  * classloader, so that the JDBC driver class outside rt.jar
  * can be loaded from here.
  */
 ClassLoader callerCL = caller != null ? caller.getClassLoader() : 
 null;
 synchronized(DriverManager.class) {
 // synchronize loading of the correct classloader.
 if (callerCL == null) {
 callerCL = Thread.currentThread().getContextClassLoader();
 }
 }
 
 I don't understand why this needs to synchronize in order to get TCCL.
 You are getting the TCCL of the current thread and callerCL is a local
 variable. So I think this can be removed.

I removed this as  I am not sure why that change was made (not by me).

I kicked off a build and will run the  jck tests after it completes

The revised webrev is here http://cr.openjdk.java.net/~lancea/8060068/webrev.03/

Note, I also tweaked the  doPriviliged block for the  JDBC property

Best,
Lance
 Mandy



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

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

2014-12-02 Thread Bernd Eckenfels

Hello,

just want to add two somewhat related observations:

we have a virtual JDBC driver which maps back to an real driver (or to
an Pool/Datasource which uses a real driver. This is to allow
JDBC URLs to work in different environments with no config. (Thats is
not the nices solution, but after we resolved the deadlocks it
works stable with the legacy code we need to support).

This does have two consequences which are related to this patch:

a) #getConnection() is used quite often, as it tunnels through to a
high performing pool (already mentioned as a good reason for DCL).

b) there have been some deadlock conditions in the past in this area. I
can try to find the details later on, but it involved class
loading/registration and the driver's synchronized.

So it is a bit risky (assuming we are not the only ones using the SPI
in this creative way) to play with the locking.

At the same time, it is also needed. Maybe the more finegrained lock
and the lock-free happy path help with it. I wonder, is there a
special procedure to warn in the release notes/compatibility guide
other than having the patch listed in a changelog? In order to warn
people switching to 9 (in 5 years or so :).

Gruss
Bernd


 Am Tue, 2 Dec
2014 15:30:16 -0500 schrieb Lance Andersen lance.ander...@oracle.com:

 Hi Mandy,
 
 Thank you for the review, please see below
 
 On Dec 2, 2014, at 2:56 PM, Mandy Chung mandy.ch...@oracle.com
 wrote:
 
  On 12/1/14 8:52 AM, Lance Andersen wrote:
  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/
  
  initDriversIfNeeded() is called to ensure that the drivers are
  registered. 
 This is a one time operation  only those specified via the system
 property of supporting ServiceLoader.  
 
 A user could invoke Class.forName(my java.sql.Driver impl) and
 cause a driver to be registered at any time
  So it may be better to have a getter method to ensure driver
  classes are loaded as well as return the registeredDrivers
  copy-on-write-arraylist. i.e. you could rename initDriversIfNeeded
  to getRegisteredDrivers and replace
for(DriverInfo aDriver : registeredDrivers) {
  
  with 
for(DriverInfo aDriver : getRegisteredDrivers()) 
 I can do that if you think that is a cleaner approach?  I am
 flexible :-)
  
  line 564-567: this comment should belong to loadInitialDrivers
  right?
 I had left it there as both methods kind of flow together, but I
 moved it per your suggestion.  Thank you
  Nit line 567 - not align (one more space to the right needed)
 fixed
  
  Is driversSync object necessary? 
 I am not 100% sure but I thought having its own monitor would further
 reduce the  contention possibility and given this was only reported
 in this one case, I was playing it safe :-).
 
   Can you make loadInitialDrivers be
  a synchronized method and move line 575-581 except 578 to the 
  loadInitialDrivers method (driversInitialized is volatile which is 
  good).
 That is doable, I just thought keeping the DCL code together makes it
 easier to read and decided to have its own monitor.  If you feel we
 should go this way, I will, just let me know 
 
 Best,
 Lance
  
  Mandy
 
 
 
 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering 
 1 Network Drive 
 Burlington, MA 01803
 lance.ander...@oracle.com

More memory-efficient internal representation for Strings: call for more data

2014-12-02 Thread Aleksey Shipilev

Hi,

As you may already know, we are looking into more memory efficient
representation for Strings:
 https://bugs.openjdk.java.net/browse/JDK-8054307

As part of preliminary performance work for this JEP, we have to collect
the empirical data on usual characteristics of Strings and char[]-s
normal applications have, as well as figure out the early estimates for
the improvements based on that data. What we have so far is written up here:
 http://cr.openjdk.java.net/~shade/density/string-density-report.pdf

We would appreciate if people who are interested in this JEP can provide
the additional data on their applications. It is double-interesting to
have the data for the applications that process String data outside
Latin1 plane. Our current data says these cases are rather rare. Please
read the current report draft, and try to process your own heap dumps
using the instructions in the Appendix.

Thanks,
-Aleksey.

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


Hi Bernd,

On 12/2/14 1:49 PM, Bernd Eckenfels wrote:

Hello,

just want to add two somewhat related observations:

we have a virtual JDBC driver which maps back to an real driver (or to
an Pool/Datasource which uses a real driver. This is to allow
JDBC URLs to work in different environments with no config. (Thats is
not the nices solution, but after we resolved the deadlocks it
works stable with the legacy code we need to support).


Would you be able to try this patch and see if the deadlocks are 
reproducible?  Lance has been trying to get customers to verify this 
patch as this code has been deadlock-prone.  Your feedback would be very 
useful.

This does have two consequences which are related to this patch:

a) #getConnection() is used quite often, as it tunnels through to a
high performing pool (already mentioned as a good reason for DCL).


Once the drivers are loaded and initialized (once), getConnection would
not need any locking (it just checks the volatile boolean field).

Do you see any potential performance issue with it?



b) there have been some deadlock conditions in the past in this area. I
can try to find the details later on, but it involved class
loading/registration and the driver's synchronized.


This may be similar to the deadlock problem reported in this bug
during the class initialization of DriverManager class and the
driver classes (that also triggers DriverManager.clinit)

This patch changes the loading/registration of the drivers to be
lazy at the first time when getConnection, getDriver, getDrivers
is called.  No synchronized is done afterward.

I'm foreign to the JDBC spec and implementation and I may miss
some subtle issue here.


So it is a bit risky (assuming we are not the only ones using the SPI
in this creative way) to play with the locking.

At the same time, it is also needed. Maybe the more finegrained lock
and the lock-free happy path help with it. I wonder, is there a
special procedure to warn in the release notes/compatibility guide
other than having the patch listed in a changelog? In order to warn
people switching to 9 (in 5 years or so :).


Yes the current way is to tag the JBS issue with release-note=yes and 
add the suggested release note in the bug.


On the other hand, if this fix is high risk, we should re-evaluate if 
this should be fixed differently.  This fix is not intended to cause any 
behavioral incompatibility.   Is that your concern with this patch?


Mandy

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

Bernd,

Thank you for your input

On Dec 2, 2014, at 4:49 PM, Bernd Eckenfels e...@zusammenkunft.net wrote:

 Hello,
 
 just want to add two somewhat related observations:
 
 we have a virtual JDBC driver which maps back to an real driver (or to
 an Pool/Datasource which uses a real driver. This is to allow
 JDBC URLs to work in different environments with no config. (Thats is
 not the nices solution, but after we resolved the deadlocks it
 works stable with the legacy code we need to support).

A couple of comments

-  DataSources were never meant to register themselves via DriverManager.  
Unfortunately some implementations of DataSource still do. 
-  The changes should reduce the potential  for deadlocks given we no longer 
use a synchronized method for registeredDriver.  We have been using 
CopyOnWriteArrayList since JDK 7
-  The removal of the static block with the removal of having registerDriver 
synchronized will further reduce the probability
 
 This does have two consequences which are related to this patch:
 
 a) #getConnection() is used quite often, as it tunnels through to a
 high performing pool (already mentioned as a good reason for DCL).
 
 b) there have been some deadlock conditions in the past in this area. I
 can try to find the details later on, but it involved class
 loading/registration and the driver's synchronized.
These hopefully are gone with the change, but if you have a simple repro after 
trying the patch, that would be helpful

Best,
Lance
 
 So it is a bit risky (assuming we are not the only ones using the SPI
 in this creative way) to play with the locking.
 
 At the same time, it is also needed. Maybe the more finegrained lock
 and the lock-free happy path help with it. I wonder, is there a
 special procedure to warn in the release notes/compatibility guide
 other than having the patch listed in a changelog? In order to warn
 people switching to 9 (in 5 years or so :).
 
 Gruss
 Bernd
 
 
 Am Tue, 2 Dec
 2014 15:30:16 -0500 schrieb Lance Andersen lance.ander...@oracle.com:
 
 Hi Mandy,
 
 Thank you for the review, please see below
 
 On Dec 2, 2014, at 2:56 PM, Mandy Chung mandy.ch...@oracle.com
 wrote:
 
 On 12/1/14 8:52 AM, Lance Andersen wrote:
 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/
 
 initDriversIfNeeded() is called to ensure that the drivers are
 registered. 
 This is a one time operation  only those specified via the system
 property of supporting ServiceLoader.  
 
 A user could invoke Class.forName(my java.sql.Driver impl) and
 cause a driver to be registered at any time
 So it may be better to have a getter method to ensure driver
 classes are loaded as well as return the registeredDrivers
 copy-on-write-arraylist. i.e. you could rename initDriversIfNeeded
 to getRegisteredDrivers and replace
  for(DriverInfo aDriver : registeredDrivers) {
 
 with 
  for(DriverInfo aDriver : getRegisteredDrivers()) 
 I can do that if you think that is a cleaner approach?  I am
 flexible :-)
 
 line 564-567: this comment should belong to loadInitialDrivers
 right?
 I had left it there as both methods kind of flow together, but I
 moved it per your suggestion.  Thank you
 Nit line 567 - not align (one more space to the right needed)
 fixed
 
 Is driversSync object necessary? 
 I am not 100% sure but I thought having its own monitor would further
 reduce the  contention possibility and given this was only reported
 in this one case, I was playing it safe :-).
 
 Can you make loadInitialDrivers be
 a synchronized method and move line 575-581 except 578 to the 
 loadInitialDrivers method (driversInitialized is volatile which is 
 good).
 That is doable, I just thought keeping the DCL code together makes it
 easier to read and decided to have its own monitor.  If you feel we
 should go this way, I will, just let me know 
 
 Best,
 Lance
 
 Mandy
 
 
 
 Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering 
 1 Network Drive 
 Burlington, MA 01803
 lance.ander...@oracle.com
 
 
 
 



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: More memory-efficient internal representation for Strings: call for more data

2014-12-02 Thread Vitaly Davidovich

Any consideration towards removing the char[] (or byte[]) indirection
altogether? .NET for example stores the bytes inline with the instance.

Sent from my phone
On Dec 2, 2014 4:59 PM, Aleksey Shipilev aleksey.shipi...@oracle.com
wrote:

 Hi,

 As you may already know, we are looking into more memory efficient
 representation for Strings:
  https://bugs.openjdk.java.net/browse/JDK-8054307

 As part of preliminary performance work for this JEP, we have to collect
 the empirical data on usual characteristics of Strings and char[]-s
 normal applications have, as well as figure out the early estimates for
 the improvements based on that data. What we have so far is written up
 here:
  http://cr.openjdk.java.net/~shade/density/string-density-report.pdf

 We would appreciate if people who are interested in this JEP can provide
 the additional data on their applications. It is double-interesting to
 have the data for the applications that process String data outside
 Latin1 plane. Our current data says these cases are rather rare. Please
 read the current report draft, and try to process your own heap dumps
 using the instructions in the Appendix.

 Thanks,
 -Aleksey.

Re: More memory-efficient internal representation for Strings: call for more data

2014-12-02 Thread Aleksey Shipilev

Hi Vitaly,

Please read the JEP proposal. String/char[] fusion (that's what you are
describing) is out of scope for this work. Baby steps. Careful baby steps.

-Aleksey.

On 03.12.2014 01:13, Vitaly Davidovich wrote:
 Any consideration towards removing the char[] (or byte[]) indirection
 altogether? .NET for example stores the bytes inline with the instance.
 
 Sent from my phone
 
 On Dec 2, 2014 4:59 PM, Aleksey Shipilev aleksey.shipi...@oracle.com
 mailto:aleksey.shipi...@oracle.com wrote:
 
 Hi,
 
 As you may already know, we are looking into more memory efficient
 representation for Strings:
  https://bugs.openjdk.java.net/browse/JDK-8054307
 
 As part of preliminary performance work for this JEP, we have to collect
 the empirical data on usual characteristics of Strings and char[]-s
 normal applications have, as well as figure out the early estimates for
 the improvements based on that data. What we have so far is written
 up here:
  http://cr.openjdk.java.net/~shade/density/string-density-report.pdf
 http://cr.openjdk.java.net/%7Eshade/density/string-density-report.pdf
 
 We would appreciate if people who are interested in this JEP can provide
 the additional data on their applications. It is double-interesting to
 have the data for the applications that process String data outside
 Latin1 plane. Our current data says these cases are rather rare. Please
 read the current report draft, and try to process your own heap dumps
 using the instructions in the Appendix.
 
 Thanks,
 -Aleksey.

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

Hi Mandy,
On Dec 2, 2014, at 5:10 PM, Mandy Chung mandy.ch...@oracle.com wrote:

 Hi Bernd,
 
 On 12/2/14 1:49 PM, Bernd Eckenfels wrote:
 Hello,
 
 just want to add two somewhat related observations:
 
 we have a virtual JDBC driver which maps back to an real driver (or to
 an Pool/Datasource which uses a real driver. This is to allow
 JDBC URLs to work in different environments with no config. (Thats is
 not the nices solution, but after we resolved the deadlocks it
 works stable with the legacy code we need to support).
 
 Would you be able to try this patch and see if the deadlocks are 
 reproducible?  Lance has been trying to get customers to verify this patch as 
 this code has been deadlock-prone.  Your feedback would be very useful.
 This does have two consequences which are related to this patch:
 
 a) #getConnection() is used quite often, as it tunnels through to a
 high performing pool (already mentioned as a good reason for DCL).
 
 Once the drivers are loaded and initialized (once), getConnection would
 not need any locking (it just checks the volatile boolean field).
correct as the registeredDrivers arraylist should be OK
 
 Do you see any potential performance issue with it?
 
 
 b) there have been some deadlock conditions in the past in this area. I
 can try to find the details later on, but it involved class
 loading/registration and the driver's synchronized.
 
 This may be similar to the deadlock problem reported in this bug
 during the class initialization of DriverManager class and the
 driver classes (that also triggers DriverManager.clinit)
 
 This patch changes the loading/registration of the drivers to be
 lazy at the first time when getConnection, getDriver, getDrivers
 is called.  No synchronized is done afterward.

This is how it used to be prior to adding the static initializer block..  The 
difference is we do not do this any longer in registerDriver as it was in JDBC 
10 it is not needed.  
The only time it is needed is when you are looking up a URL or return a driver. 
 There was no need to do this in registerDriver ever.
 
 

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 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom

2014-12-02 Thread Martin Buchholz

I support Peter's initiative and am willing to help review if we have
general consensus about the direction.

From superficial review:

+int nread = randomStream.read(bytes);
+if (nread != bytes.length) {
+throw new InternalError(Short read from:  + RANDOM_FILE);

Martin's pet peeve: use readFully (why doesn't InputStream support
that yet?!) copy-paste from elsewhere in the jdk.

---

I'm not sure we need instances that hold on to system resources.  any
particular call site is likely to do a single read of a small number
of random bytes in a clinit method.


On Tue, Dec 2, 2014 at 9:36 AM, Alan Bateman alan.bate...@oracle.com wrote:
 On 02/12/2014 16:42, Peter Levart wrote:

 :

 Here's a proof of concept for an API that just delegates to
 system-provided cryptographically secure (as declared by the system(s))
 pseudo random number generator:

 http://cr.openjdk.java.net/~plevart/jdk9-dev/SystemRandom/webrev.01/

 The API looks reasonable to me too, I'm just not sure that java.util is the
 right place and whether it needs to be a Java SE API.

 -Alan

Re: More memory-efficient internal representation for Strings: call for more data

2014-12-02 Thread Douglas Surber

String construction is a big performance issue for JDBC drivers. Most 
queries return some number of Strings. The overwhelming majority of 
those Strings will be short lived. The cost of constructing these 
Strings from network bytes is a large fraction of total execution 
time. Any increase in the cost of constructing a String will far out 
weigh any reduction in memory use, at least for query results.


All of the proposed compression methods require an additional scan of 
the entire string. That's exactly the wrong direction. Something like 
the following pseudo-code is common inside a driver.


  {
char[] c = new char[n];
for (i = 0; i  n; i++) c[i] = charSource.next();
return new String(c);
  }

The array copy inside the String constructor is a significant 
fraction of JDBC driver execution time. Adding an additional scan on 
top of it is making things worse regardless of the transient benefit 
of more compact storage. In the case of a query result the String 
will be likely never be promoted out of new space; the benefit of 
compression would be minimal.


I don't dispute that Strings occupy a significant fraction of the 
heap or that a lot of those bytes are zero. And I certainly agree 
that reducing memory footprint is valuable, but any worsening of 
String construction time will likely be a problem.


Douglas

At 02:13 PM 12/2/2014, core-libs-dev-requ...@openjdk.java.net wrote:

Date: Wed, 03 Dec 2014 00:59:10 +0300
From: Aleksey Shipilev aleksey.shipi...@oracle.com
To: Java Core Libs core-libs-dev@openjdk.java.net
Cc: charlie hunt charlie.h...@oracle.com
Subject: More memory-efficient internal representation for Strings:
call formore data
Message-ID: 547e362e.5010...@oracle.com
Content-Type: text/plain; charset=utf-8

Hi,

As you may already know, we are looking into more memory efficient
representation for Strings:
 https://bugs.openjdk.java.net/browse/JDK-8054307

As part of preliminary performance work for this JEP, we have to 
collect

the empirical data on usual characteristics of Strings and char[]-s
normal applications have, as well as figure out the early estimates 
for
the improvements based on that data. What we have so far is written 
up here:


http://cr.openjdk.java.net/~shade/density/string-density-report.pdf

We would appreciate if people who are interested in this JEP can 
provide
the additional data on their applications. It is double-interesting 
to

have the data for the applications that process String data outside
Latin1 plane. Our current data says these cases are rather rare. 
Please
read the current report draft, and try to process your own heap 
dumps

using the instructions in the Appendix.

Thanks,
-Aleksey.

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

2014-12-02 Thread Bernd Eckenfels

Hello Mandy and Lance,

(sorry, not a full quote for more focused answers, inline)

Am Tue, 02 Dec 2014 14:10:06 -0800
schrieb Mandy Chung mandy.ch...@oracle.com:

 Would you be able to try this patch and see if the deadlocks are 
 reproducible?  Lance has been trying to get customers to verify this 
 patch as this code has been deadlock-prone.  Your feedback would be
 very useful.

Hm, I might be able to test it with the unit tests (once I get the
binary to build). Currently I cant test the whole system with 8 or 9
(do you think porting it to 7 might be straight forward?). I think the
Unit tests have no reproducers for this issue, will check.

  This does have two consequences which are related to this patch:
 
  a) #getConnection() is used quite often, as it tunnels through to a
  high performing pool (already mentioned as a good reason for DCL).
 
 Once the drivers are loaded and initialized (once), getConnection
 would not need any locking (it just checks the volatile boolean
 field).
 
 Do you see any potential performance issue with it?

No, dont see a problem. I wanted to add another vote pro DCL.

 This may be similar to the deadlock problem reported in this bug
 during the class initialization of DriverManager class and the
 driver classes (that also triggers DriverManager.clinit)

Yes, all related.

 On the other hand, if this fix is high risk, we should re-evaluate if 
 this should be fixed differently.  This fix is not intended to cause
 any behavioral incompatibility.   Is that your concern with this
 patch?

I will have to find the stack traces from well-back-than where we had
the deadlocks and compare it with the proposed patch. Up until now it
is just a mild suspect :)

Greetings
Bernd

Java 8 FilterOutputStream.close() bug

2014-12-02 Thread Nathan Clement

Hi,

I have encountered a bug in FilterOutputStream under Java 8.  The bug is that 
calling close() on the same stream twice can now result in an exception, 
whereas under Java 7 it did not.  The reason is that FilterOutputStream.close() 
calls flush() before calling close() on the underlying stream.  Calling close() 
twice on the FilterOutputStream will thus result in flush() being called twice. 
 Some streams (such as OracleBlobOutputStream) will throw an exception if 
flush() is called after the stream is closed.  In Java 7 any exceptions when 
calling flush() were ignored, but in Java 8 these exceptions are passed on to 
the caller.

This bug appears to have been introduced in 7015589: (spec) 
BufferedWriter.close leaves stream open if close of underlying Writer fails.  
The FilterOutputStream.close() method now does not obey the contract of 
AutoClosable.close(): If the stream is already closed then invoking this 
method has no effect.

Some sample code that illustrates the problem:

try( InputStream bis = new BufferedInputStream( inputStream );
 OutputStream outStream = payloadData.setBinaryStream( 0 );
 BufferedOutputStream bos = new BufferedOutputStream( outStream );
 DeflaterOutputStream deflaterStream = new DeflaterOutputStream(
 bos, new Deflater( 3 ) ) )
{
fileSize = IOUtil.copy( bis, deflaterStream );
}
The try-with-resources mechanism will call close on the DeflaterOutputStream, 
which will in turn call close on the BufferedOutputStream, which will call 
flush() then close() on the blob output stream.  The try-with-resources 
mechanism will then call close on the BufferedOutputStream which again calls 
flush() on the blob output stream, resulting in an exception.

My proposed fix is to change FilterOutputStream.close() as follows:

@Override
@SuppressWarnings(try)
public void close() throws IOException {
if (!closed) {
closed = true;
try (OutputStream ostream = out) {
flush();
}
}
}

private boolean closed = false;

This will prevent flush() being called on an already closed stream.

I have reported this issue via the Oracle bug tracker and got Review ID 
JI-9014085, however I never received any follow up from Oracle.  I have also 
discussed the issue on Stack Overflow: 
http://stackoverflow.com/questions/25175882/java-8-filteroutputstream-exception.
  Do people agree that this is a bug?  If so, what is the process for fixing it?

Thanks,

Nathan Clement

Re: More memory-efficient internal representation for Strings: call for more data

2014-12-02 Thread Aleksey Shipilev

Hi Douglas,

On 12/03/2014 02:24 AM, Douglas Surber wrote:
 String construction is a big performance issue for JDBC drivers. Most
 queries return some number of Strings. The overwhelming majority of
 those Strings will be short lived. The cost of constructing these
 Strings from network bytes is a large fraction of total execution time.
 Any increase in the cost of constructing a String will far out weigh any
 reduction in memory use, at least for query results.

You will also have to take into the account that shorter (compressed)
Strings allow for more efficient operations on them. This is not to
mention the GC costs are also usually hidden from the naive
performance estimations: even though you can perceive the mutator is
spending more time doing work, that might be offset by easier job for GC.

 All of the proposed compression methods require an additional scan of
 the entire string. That's exactly the wrong direction. Something like
 the following pseudo-code is common inside a driver.
 
   {
 char[] c = new char[n];
 for (i = 0; i  n; i++) c[i] = charSource.next();
 return new String(c);
   }

Good to know. We will be assessing the String(char[]) construction
performance in the course of this performance work. What would you say
is a characteristic high-level benchmark for the scenario you are
describing?

 The array copy inside the String constructor is a significant fraction
 of JDBC driver execution time. Adding an additional scan on top of it is
 making things worse regardless of the transient benefit of more compact
 storage. In the case of a query result the String will be likely never
 be promoted out of new space; the benefit of compression would be minimal.

It's hard to say at this point. We want to understand what footprint
improvements we are talking about. I agree that if cost-benefit analysis
will say the performance is degrading beyond the sane limits even if we
are happy with memory savings, there is little reason to push this into
the general JDK.

Thanks,
-Aleksey

Re: More memory-efficient internal representation for Strings: call for more data

2014-12-02 Thread Douglas Surber

The most common operation on most Strings in query results is to do 
nothing. Just construct the String, hold onto it while the rest of 
the transaction completes, then drop it on the floor. Probably the 
next most common is to encode the chars to write them to an 
OutputStream or send them back to the database. I'd be curious how a 
compact representation would help those operations.


SPECjEnterprise is a widely used standard benchmark. It probably uses 
mostly (or even entirely) ASCII characters so it's not representative 
of many customers.


My definition of sane limits might be different than yours. As far 
as I'm concerned String construction is already too slow and should 
be made faster by eliminating the char[] copy when possible.


Douglas

At 03:47 PM 12/2/2014, Aleksey Shipilev wrote:

Hi Douglas,

On 12/03/2014 02:24 AM, Douglas Surber wrote:
 String construction is a big performance issue for JDBC drivers. 
Most
 queries return some number of Strings. The overwhelming majority 
of

 those Strings will be short lived. The cost of constructing these
 Strings from network bytes is a large fraction of total execution 
time.
 Any increase in the cost of constructing a String will far out 
weigh any

 reduction in memory use, at least for query results.

You will also have to take into the account that shorter 
(compressed)

Strings allow for more efficient operations on them. This is not to
mention the GC costs are also usually hidden from the naive
performance estimations: even though you can perceive the mutator is
spending more time doing work, that might be offset by easier job 
for GC.


 All of the proposed compression methods require an additional 
scan of
 the entire string. That's exactly the wrong direction. Something 
like

 the following pseudo-code is common inside a driver.

   {
 char[] c = new char[n];
 for (i = 0; i  n; i++) c[i] = charSource.next();
 return new String(c);
   }

Good to know. We will be assessing the String(char[]) construction
performance in the course of this performance work. What would you 
say

is a characteristic high-level benchmark for the scenario you are
describing?

 The array copy inside the String constructor is a significant 
fraction
 of JDBC driver execution time. Adding an additional scan on top 
of it is
 making things worse regardless of the transient benefit of more 
compact
 storage. In the case of a query result the String will be likely 
never
 be promoted out of new space; the benefit of compression would be 
minimal.


It's hard to say at this point. We want to understand what footprint
improvements we are talking about. I agree that if cost-benefit 
analysis
will say the performance is degrading beyond the sane limits even if 
we
are happy with memory savings, there is little reason to push this 
into

the general JDK.

Thanks,
-Aleksey

Re: More memory-efficient internal representation for Strings: call for more data

2014-12-02 Thread Xueming Shen


On 12/02/2014 04:42 PM, Douglas Surber wrote:

The most common operation on most Strings in query results is to do nothing. 
Just construct the String, hold onto it while the rest of the transaction 
completes, then drop it on the floor. Probably the next most common is to 
encode the chars to write them to an OutputStream or send them back to the 
database. I'd be curious how a compact representation would help those 
operations.



It depends on what inside those query results. If most of them are ascii, 
only a small portion
are double byted user data (for example, it is true for most of the utf8 xml 
files), you might
be able to save the cpu time/throughput by only copying half length of the 
bytes around their
life circle, especially copy around is the only operation they are carrying 
on.

-Sherman



SPECjEnterprise is a widely used standard benchmark. It probably uses mostly 
(or even entirely) ASCII characters so it's not representative of many 
customers.

My definition of sane limits might be different than yours. As far as I'm 
concerned String construction is already too slow and should be made faster by 
eliminating the char[] copy when possible.

Douglas

At 03:47 PM 12/2/2014, Aleksey Shipilev wrote:

Hi Douglas,

On 12/03/2014 02:24 AM, Douglas Surber wrote:
 String construction is a big performance issue for JDBC drivers. Most
 queries return some number of Strings. The overwhelming majority of
 those Strings will be short lived. The cost of constructing these
 Strings from network bytes is a large fraction of total execution time.
 Any increase in the cost of constructing a String will far out weigh any
 reduction in memory use, at least for query results.

You will also have to take into the account that shorter (compressed)
Strings allow for more efficient operations on them. This is not to
mention the GC costs are also usually hidden from the naive
performance estimations: even though you can perceive the mutator is
spending more time doing work, that might be offset by easier job for GC.

 All of the proposed compression methods require an additional scan of
 the entire string. That's exactly the wrong direction. Something like
 the following pseudo-code is common inside a driver.

   {
 char[] c = new char[n];
 for (i = 0; i  n; i++) c[i] = charSource.next();
 return new String(c);
   }

Good to know. We will be assessing the String(char[]) construction
performance in the course of this performance work. What would you say
is a characteristic high-level benchmark for the scenario you are
describing?

 The array copy inside the String constructor is a significant fraction
 of JDBC driver execution time. Adding an additional scan on top of it is
 making things worse regardless of the transient benefit of more compact
 storage. In the case of a query result the String will be likely never
 be promoted out of new space; the benefit of compression would be minimal.

It's hard to say at this point. We want to understand what footprint
improvements we are talking about. I agree that if cost-benefit analysis
will say the performance is degrading beyond the sane limits even if we
are happy with memory savings, there is little reason to push this into
the general JDK.

Thanks,
-Aleksey

Re: Review request for JDK-8051536: Convert JAXP function tests: javax.xml.parsers to jtreg(testng) tests

2014-12-02 Thread huizhe wang

I agree with Lance's suggestion on utilizing the @BeforeMethod methods
to instantiate factories, or DocumentBuilder where there's no change in
state on the DocumentBuilderFactory (that is, no set** in any of the
tests, such as DocumentBuilderImpl01), or using DataProvider.

However, if you do make the changes, you may need to reset the factory
or force the tests to run in order. In case of SAXParserTest03, all
three test cases do spf.setValidating(true) so it can be done in
@BeforeMethod. But then test 02 and 03 test NamespaceAware, without
ordering or setting it to false (default) explicitly in test01, it may
affect test 01.

In other cases, for example DocumentBuilderFactory01, you'd need to set
the feature back to default within a test in order to not cause negative
impact on other tests.

As Lance said, these suggestions should not prevent you pushing it in
the current state. But it may be an helpful exercise if you'd want to
get familiar with the package.

Cheers,
Joe

On 12/2/2014 6:33 AM, Lance Andersen wrote:

Hi Frank,

I think this looks good overall.

I might suggest that you consider utilizing the @BeforeMethod methods
further, For example is SAXParserTest03.java you could move

SAXParserFactory spf = SAXParserFactory.newInstance();
spf.setValidating(true);

to your @BeforeMethod, or consider using a DataProvider that is
accessible from all classes to reduce some code duplication which will
make maintenance a bit easier.

I am not sure how much more additional info failUnexpected adds, but
you could omit this if you choose and just have the test method throw
Exception and you will get your failure.

Again the above are suggestions, but should not prevent you pushing
this to the workspace

Best
Lance
On Dec 1, 2014, at 9:57 PM, Frank Yuan frank.y...@oracle.com
mailto:frank.y...@oracle.com wrote:

Hi, Joe and All

We are working on moving internal jaxp functional tests to open jdk repo.
This is the parsers suite. Would you please review these test? Any
comment

will be appreciated.

bug: https://bugs.openjdk.java.net/browse/JDK-8051536
webrev:
http://cr.openjdk.java.net/~joehw/jdk9/test/Frank/8051536/webrev/
http://cr.openjdk.java.net/%7Ejoehw/jdk9/test/Frank/8051536/webrev/

Thanks,

Frank

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