date:20130415

Re: RFR 8010280: jvm.cfg needs updating for non-server builds

2013-04-15 Thread Erik Joelsson

This looks good to me.

/Erik

On 2013-04-16 00:42, David Holmes wrote:

FYI updated webrev at same location, removing the dead code Erik spotted.

http://cr.openjdk.java.net/~dholmes/8010280/webrev/

On 16/04/2013 2:25 AM, Mike Duigou wrote:

Hi David;

I remember reviewing the jvm.cfg config patch for JDK 7. I had hoped 
to see the "classic" and "green" flags go away and some of the other 
legacy flags like "-hotspot" reduced to WARN. What's the difference 
between removing an entry completely and retaining it with "ERROR"?

Just the nature of the error message:

> java -green
Error: green VM not supported
> java -blue
Unrecognized option: -blue
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

I wasn't touching any of the legacy stuff - though if this needs to go 
to CCC I would suggest removing all the legacy entries.

Additionally I don't like that aliases have differing definitions and 
some confusing ones like "-server ALIASED_TO -client". Is this 
necessary or just historically convenient?

I don't like aliases period! Historically (and this is very recent 
history) it was necessary to deal with the test suites being applied 
to a JDK with, eg, only client VM. Every test that specified -server 
would fail if the alias didn't exist (and as I stated we're moving 
away from that ie the tests don't set -client or -server but the 
complete test suite run does, and it knows what VM is under test.

Personally I'd probably choose WARN for any VM not present.

The problem is that the "right" thing depends on who is building what, 
and how they plan to use it. All I can do is define a not-unreasonable 
default policy. I also have a time constraint as I need to get this in 
before the 23rd to meet an internal deadline.

I've attached all the generated versions below.

Thanks,
David

::
linux-i586-client/jdk/lib/i386/jvm.cfg
::
-client KNOWN
-server ALIASED_TO -client
-hotspot ALIASED_TO -client
-classic WARN
-native ERROR
-green ERROR
::
linux-i586-client-server/jdk/lib/i386/jvm.cfg
::
# Copyright (c) 2001, 2013, Oracle and/or its affiliates. All rights 
reserved.

# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
#
# This code is free software; you can redistribute it and/or modify it
# under the terms of the GNU General Public License version 2 only, as
# published by the Free Software Foundation.  Oracle designates this
# particular file as subject to the "Classpath" exception as provided
# by Oracle in the LICENSE file that accompanied this code.
#
# This code is distributed in the hope that it will be useful, but 
WITHOUT

# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
# version 2 for more details (a copy is included in the LICENSE file that
# accompanied this code).
#
# You should have received a copy of the GNU General Public License 
version

# 2 along with this work; if not, write to the Free Software Foundation,
# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
#
# Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
# or visit www.oracle.com if you need additional information or have any
# questions.
#
# List of JVMs that can be used as an option to java, javac, etc.
# Order is important -- first in this list is the default JVM.
# NOTE that this both this file and its format are UNSUPPORTED and
# WILL GO AWAY in a future release.
#
# You may also select a JVM in an arbitrary location with the
# "-XXaltjvm=" option, but that too is unsupported
# and may not be available in a future release.
#
-client IF_SERVER_CLASS -server
-server KNOWN
-hotspot ALIASED_TO -client
-classic WARN
-native ERROR
-green ERROR
::
linux-i586-client-server-minimal1/jdk/lib/i386/jvm.cfg
::
# Copyright (c) 2001, 2013, Oracle and/or its affiliates. All rights 
reserved.

# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
#
# This code is free software; you can redistribute it and/or modify it
# under the terms of the GNU General Public License version 2 only, as
# published by the Free Software Foundation.  Oracle designates this
# particular file as subject to the "Classpath" exception as provided
# by Oracle in the LICENSE file that accompanied this code.
#
# This code is distributed in the hope that it will be useful, but 
WITHOUT

# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
# version 2 for more details (a copy is included in the LICENSE file that
# accompanied this code).
#
# You should have received a copy of the GNU General Public License 
version

# 2 along with this work; if not, write to the Free Software Foundation,
# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
#
# Please contact Oracle, 500 Oracle Parkway, Redw

Re: RFR JDK-8011426: java.util collection Spliterator implementations

2013-04-15 Thread Mike Duigou

I went back and started again with the 8010096 webrev.

Spliterators::

- some implementations are private and some are package private. All package 
private?

List/Set/Iterator/SortedSet::

- Include the same interface level @implSpec warning as Collection?

Spliterator::

- "Spliterators also report ..." You may want to avoid the plural form since 
there's also a class of that name.

Spliterator.NONNULL - "This applies, for example, to ...". I might like the 
name Spliterator.NONULLS better.

Spliterator.IMMUTABLE - this name doesn't quite capture what's allowed and 
what's prohibited. An Arrays.asList() list is IMMUTABLE but elements can still 
be replaced in place. Collections.unmodifiableList(Array.asList(..)) is 
entirely immutable. Is the distinction ever important?

- I guess the issue is that some of the flags describe characteristics of the 
source and some describe characteristics of the elements.

- "A diagnostic warning that boxing of primitives values is occurring of can be 
requested by setting the boolean system property {@code 
org.openjdk.java.util.stream.tripwire} is to {@code true}."

- Neither forEachREmaining on Iterator or tryAdvance on Spliterator say whether 
it's possible (or advisable) to continue advancing following an exception being 
thrown. Will calling again continue with the next element? The same element? 
Unspecified? Is calling again after an exception prohibited?

- getExactSizeIfKnown() - use hasCharacteristics?

- The note in CONCURRENT "Such a Spliterator is
 * inconsistent and no guarantees can be made about any computation using
 * that Spliterator." Is this necessary or just confusing. Users won't 
encounter this.

- Same with the similar note in SUBSIZED. 

Mike



On Apr 10 2013, at 06:50 , Paul Sandoz wrote:

> Hi,
> 
> Following up from JDK-8010096 [1] here is a webrev for spliterator 
> implementations of collection classes in java.util:
> 
>  http://cr.openjdk.java.net/~psandoz/lambda/spliterator/jdk-8011426/webrev/
> 
> This is dependent on [1].
> 
> --
> 
> Note that for some reason the webrev script creates the jdk changeset file 
> for my complete hg patch queue and not from the revision i specify. Anyone 
> know how to change that?
> 
> Paul.
> 
> 
> [1] http://cr.openjdk.java.net/~psandoz/lambda/spliterator/jdk-8010096/webrev/

hg: jdk8/tl/jdk: 8011800: Add java.util.Objects.requireNonNull(T, Supplier)

2013-04-15 Thread joe . darcy

Changeset: baaa706d7677
Author:darcy
Date:  2013-04-15 18:31 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/baaa706d7677

8011800: Add java.util.Objects.requireNonNull(T, Supplier)
Reviewed-by: alanb, dholmes, mduigou

! src/share/classes/java/util/Objects.java
! test/java/util/Objects/BasicObjectsTest.java

Re: RFR: 8004518 & 8010122 : Default methods on Map

2013-04-15 Thread Ulf Zibis


Am 15.04.2013 23:06, schrieb Mike Duigou:

That's because I'm not the only author. I get to fix it though, the glamourous 
life of a JDK janitor. :-)


;-)


HashTable line 917, 938, 984 etc.
HashMap line 588 etc.
Collections line 1402 etc.

Should be more consistent now. I'm not willing to attempt correct this 
non-problem more broadly.


HashTable line 972, 992, 1011, 1035, 1064, 1099,  etc.


Collections:
lines 1442... + 3900... , indentation for follow up line should be 8

There are still lines 976, 1062

Of which file? According to 
http://cr.openjdk.java.net/~mduigou/JDK-8010122/5/webrev/src/share/classes/java/util/Collections.java.html

neither of those lines seems applicable.


Oops I'm sorry, I meant lines 976, 1062 of Map.
In Collections I've found additional inconsistent indentations, lines 2836, 
2837, 2848, 2853, 2897
Old code: 262, 270, 292, 1248, 1304, 1528, 1580, 1611, 1784, 1873, 1995, 2036, 2092, 2304, 2414, 
2460, 2694, 2760, 2770, 2819, 3032, 3071, 3118, 3155, 3164, 3214, 3273, 3318, 3322, 3358, 3397, 
3446, 3559, 3633, 3756, 3790, 3809, 3832, 3867, 3965, 4014, 4070, 4109, 4133, 4301, 4381, 4413, 4445.



-Ulf

RFR : 8010953: Add primitive summary statistics utils

2013-04-15 Thread Mike Duigou

Hello all;

Another review in the JSR-335 libraries series. These three classes provide a 
utility for conveniently finding count, sum, min,  max and average of ints, 
longs or doubles. They can be used with existing code but will most likely be 
used with the Collectors utilities or directly with primitive or boxed streams.

http://cr.openjdk.java.net/~mduigou/JDK-8010953/0/webrev/

Mike

Re: Proxy.isProxyClass scalability

2013-04-15 Thread Mandy Chung



On 4/13/2013 2:59 PM, Peter Levart wrote:




I also devised an alternative caching mechanism with scalability in 
mind which uses WeakReferences for keys (for example ClassLoader) 
and values (for example Class) that could be used in this situation 
in case adding a field to ClassLoader is not an option:




I would also consider any alternative to avoid adding the 
proxyClassCache field in ClassLoader as Alan commented previously.


My observation of the typical usage of proxies is to use the 
interface's class loader to define the proxy class. So is it 
necessary to maintain a per-loader cache?  The per-loader cache maps 
from the interface names to a proxy class defined by one loader. I 
would think it's reasonable to assume the number of loaders to define 
proxy class with the same set of interfaces is small.  What if we 
make the cache as "interface names" as the key to a set of proxy 
class suppliers that can have only one proxy class per one unique 
defining loader.  If the proxy class is being generated i.e. 
ProxyClassFactory supplier, the loader is available for comparison. 
When there are more than one matching proxy classes, it would have to 
iterate all in the set.


I would assume yes, proxy class for a particular set of interfaces is 
typically defined by one classloader only. But the API allows to 
specify different loaders as long as the interfaces implemented by 
proxy class are "visible" from the loader that defines the proxy 
class. If we're talking about interface names - as opposed to 
interfaces - then the possibility that a particular set of interface 
names would want to be used to define proxy classes with different 
loaders is even bigger, since an interface name can refer to different 
interfaces with same name (think of interfaces deployed as part of an 
app in an application server, say a set of annotations used by 
different apps but deployed as part of each individual app).




Agree.  I was tempted to consider making weak reference to the interface 
classes as the key but in any case the overhead of 
Class.getClassLoader() is still a performance hog.   Let's move forward 
with the alternative you propose.


The scheme you're proposing might be possible, though not simple: The 
factory Supplier would become a Function 
and would have to maintain it's own set of cached proxy classes. There 
would be a single ConcurrentMap, FunctionClass>> to map sets of interface names to factory Functions, but the 
cached classes in a particular factory Function would still have to be 
weakly referenced. I see some difficulties in implementing such a scheme:
- expunging cleared WeakReferences could only reliably clear the cache 
inside each factory Function but removing the entry from the map of  
factory Functions when last proxy class for a particular set of 
interface names is expunged  would become a difficult task if not 
impossible with all the scalability constraints in mind (just thinking 
about concurrent requests into same factory Function where one is 
requesting new proxy class and the other is expunging cleared 
WeakReference which represents the last element in the set of cached 
proxy classes).
- one of my past ideas of implementing scalable Proxy.isProxyClass() 
was to maintain a Set in each ClassLoader populated with all 
the proxy classes defined by a particular ClassLoader. Benchmarking 
such solution showed that Class.getClassLoader() is a peformance hog, 
so I scraped it in favor of ClassValue that is now 
incorporated in the patch. In order to "choose" the right proxy class 
from the set of proxy classes inside a particular factory Function, 
the Class.getClassLoader() method would have to be used, or entries 
would have to (weakly) reference a particular ClassLoader associated 
with each proxy class.




Thanks for reminding me your earlier prototype.  I suspect the cost of 
Class.getClassLoader() is due to its lookup of the caller class every 
time it's called.


Considering all that, such solution starts to look unappealing. It 
might even be more space-hungry then the presented WeakCache.


WeakCache is currently the following:

ConcurrentMap, 
WeakReference>


another alternative would be:

ConcurrentMap, 
ConcurrentMap>>


...which might need a little less space than WeakCache (only one 
WeakReference per proxy class + one per ClassLoader instead of two 
WeakReferences per proxy class) but would require two map lookups 
during fast-path retrieval. It might not be performance critical and 
the expunging could be performed easily too.




I am fine with either of these alternatives.  As you noted, the latter 
one would save little bit of memory for the cases when several proxy 
classes are defined per loader e.g. one per each annotation type.


Mandy

Re: RFR 8010280: jvm.cfg needs updating for non-server builds

2013-04-15 Thread David Holmes

FYI updated webrev at same location, removing the dead code Erik spotted.

http://cr.openjdk.java.net/~dholmes/8010280/webrev/

On 16/04/2013 2:25 AM, Mike Duigou wrote:

Hi David;

I remember reviewing the jvm.cfg config patch for JDK 7. I had hoped to see the "classic" and
"green" flags go away and some of the other legacy flags like "-hotspot" reduced to WARN. What's
the difference between removing an entry completely and retaining it with "ERROR"?

Just the nature of the error message:

> java -green
Error: green VM not supported
> java -blue
Unrecognized option: -blue
Error: Could not create the Java Virtual Machine.
Error: A fatal exception has occurred. Program will exit.

I wasn't touching any of the legacy stuff - though if this needs to go
to CCC I would suggest removing all the legacy entries.

Additionally I don't like that aliases have differing definitions and some confusing ones
like "-server ALIASED_TO -client". Is this necessary or just historically
convenient?

I don't like aliases period! Historically (and this is very recent
history) it was necessary to deal with the test suites being applied to
a JDK with, eg, only client VM. Every test that specified -server would
fail if the alias didn't exist (and as I stated we're moving away from
that ie the tests don't set -client or -server but the complete test
suite run does, and it knows what VM is under test.

Personally I'd probably choose WARN for any VM not present.

The problem is that the "right" thing depends on who is building what,
and how they plan to use it. All I can do is define a not-unreasonable
default policy. I also have a time constraint as I need to get this in
before the 23rd to meet an internal deadline.

I've attached all the generated versions below.

Thanks,
David

::
linux-i586-client/jdk/lib/i386/jvm.cfg
::
-client KNOWN
-server ALIASED_TO -client
-hotspot ALIASED_TO -client
-classic WARN
-native ERROR
-green ERROR
::
linux-i586-client-server/jdk/lib/i386/jvm.cfg
::
# Copyright (c) 2001, 2013, Oracle and/or its affiliates. All rights
reserved.

# DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
#
# This code is free software; you can redistribute it and/or modify it
# under the terms of the GNU General Public License version 2 only, as
# published by the Free Software Foundation. Oracle designates this
# particular file as subject to the "Classpath" exception as provided
# by Oracle in the LICENSE file that accompanied this code.
#
# This code is distributed in the hope that it will be useful, but WITHOUT
# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
# version 2 for more details (a copy is included in the LICENSE file that
# accompanied this code).
#
# You should have received a copy of the GNU General Public License version
# 2 along with this work; if not, write to the Free Software Foundation,
# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
#
# Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
# or visit www.oracle.com if you need additional information or have any
# questions.
#
# List of JVMs that can be used as an option to java, javac, etc.
# Order is important -- first in this list is the default JVM.
# NOTE that this both this file and its format are UNSUPPORTED and
# WILL GO AWAY in a future release.
#
# You may also select a JVM in an arbitrary location with the
# "-XXaltjvm=" option, but that too is unsupported
# and may not be available in a future release.
#
-client IF_SERVER_CLASS -server
-server KNOWN
-hotspot ALIASED_TO -client
-classic WARN
-native ERROR
-green ERROR
::
linux-i586-client-server-minimal1/jdk/lib/i386/jvm.cfg
::
# Copyright (c) 2001, 2013, Oracle and/or its affiliates. All rights
reserved.

Re: RFR: String.join(), StringJoiner additions

2013-04-15 Thread Mike Duigou


On Apr 15 2013, at 13:03 , Steven Schlansker wrote:

> 
> On Apr 15, 2013, at 12:21 PM, Martin Buchholz  wrote:
> 
>> On Mon, Apr 15, 2013 at 11:31 AM, Martin Buchholz wrote:
>> 
>>> 
>>> OTOH, I'm guessing you are trying to improve the performance of operations
>>> like List.toString.
>>> More efficient (single copy char[]) would be to collect all the
>>> sub-CharSequences in a CharSequence[],  pre-compute the final length of the
>>> char[], allocate an array of exactly the required length, and create the
>>> final string directly from that using the package-private constructor (but
>>> in the unlikely event that a subsequence changed in size while concat'ing,
>>> be prepared to resize the array).
>>> 
>> 
>> Proceeding further along this train of thought, I might start with
>> AbstractCollection.toString() (and similar methods) and attempt to make it
>> maximally efficient.
>> Maybe add a method to JavaLangAccess to make a String directly from a
>> perfectly sized array (as needed elsewhere?).  Maybe create a
>> StringBuilder-like class that works better for typical use cases?
> 
> For what it's worth, a patch that I contributed and Mike (and others) then 
> rewrote
> contains this functionality already:
> 
> http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/src/share/classes/sun/misc/JavaLangAccess.java.patch
> 
> It's not merged yet though.

It is not forgotten. :-)



>

Re: RFR: 8004518 & 8010122 : Default methods on Map

2013-04-15 Thread Mike Duigou

Another updated webrev:

http://cr.openjdk.java.net/~mduigou/JDK-8010122/6/webrev/

I've started pre-integration final testing on this patch and plan to push it 
tomorrow unless something significant comes up.

Mike

On Apr 15 2013, at 14:31 , Mike Duigou wrote:

> 
> On Apr 14 2013, at 11:35 , Peter Levart wrote:
> 
>> 
>> On 04/14/2013 07:54 PM, Peter Levart wrote:
>>> Hi Mike,
>>> 
>>> Just a nit: The order of boolean sub-expressions in Map.replace(key, 
>>> oldValue, newValue):
>>> 
>>> 740 if (!containsKey(key) || !Objects.equals(get(key), oldValue))
>>> 
>>> ...would be more optimal if reversed (like in Map.remove(key, value)).
>>> 
>>> Regards, Peter
>> 
>> I think the most optimal is actually this:
>> 
>> default boolean remove((Object key, Object value) {
>>Object curValue = get(key);
>>if (curValue == null && (value != null || !containsKey(key)) ||
>>curValue != value && !curValue.equals(value)) {
>>return false;
>>}
>>remove(key);
>>return true;
>> }
>> 
>> ...and the same for replace(key, oldValue, newValue)...
> 
> In the current patch I've used:
> 
>default boolean remove(Object key, Object value) {
>Object curValue = get(key);
>if (!Objects.equals(curValue, value) ||
>(curValue == null && !containsKey(key))) {
>return false;
>}
>remove(key);
>return true;
>}
> 
> and similar for replace(K,V,V)
> 
> I rearranged it mostly so that calling containsKey is a last resort.
> 
> Mike
> 
>> 
>> Regards, Peter
>> 
>>> 
>>> On 04/13/2013 12:02 AM, Mike Duigou wrote:
 I have updated the webrev with these changes and a few more.
 
 merge() required an update to it's specification to correctly account for 
 null values.
 
 http://cr.openjdk.java.net/~mduigou/JDK-8010122/5/webrev/
 
 This version is currently undergoing final pre-integration testing. Unless 
 additional problems are found or the testing fails this version will be 
 integrated.
 
 Mike
 
 On Apr 12 2013, at 12:53 , Mike Duigou wrote:
 
> Thanks for the corrections. I have incorporated all of these into the 
> integration version of the patch.
> 
> On Apr 12 2013, at 12:50 , Akhil Arora wrote:
> 
>> Hi Mike,
>> 
>> a few small things -
>> 
>> UnmodifiableMap.forEach
>> is missing Objects.requireNonNull(action);
> Added.
> 
>> EmptyMap.replaceAll(BiFunction)
>> should just return instead of throwing UnsupportedOpEx
>> particularly since EmptyList.replaceAll also returns silently
>> after checking if function is null to fulfil the NPE contract
> I agree. 
> 
>> Hashtable
>> is missing a synchronized override of forEach
> added.
> 
>> CheckedMap.typeCheck(BiFunction)
>> is missing from the patch (won't compile without it)
> Apparently I forgot a qrefresh before generating the webrev. I had this 
> in my local copy as it's necessary.
> 
>> On 04/11/2013 01:03 PM, Mike Duigou wrote:
>>> Another revision incorporating primarily documentation feedback.
>>> 
>>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/2/webrev/
>>> 
>>> I've also included the java.util.Collections overrides for the default 
>>> methods. All of these are performance enhancements--the semantics were 
>>> already correct because the defaults use only public methods.
>>> 
>>> This is likely, modulo formatting of the source examples, the version 
>>> that will be pushed to TL on Friday unless somebody squawks.
>>> 
>>> Moving the current compute, computeIfPresent, computeIfAbsent and merge 
>>> defaults to ConcurrentMap and replacing the current Map defaults with 
>>> versions that throw ConcurrentModificationException will likely be 
>>> resolved in a future issue if the EG determines it to be important.
>>> 
>>> Mike
>>> 
>>> 
>>> On Apr 10 2013, at 22:42 , Mike Duigou wrote:
>>> 
 I've posted an updated webrev with the review comments I have received.
 
 http://cr.openjdk.java.net/~mduigou/JDK-8010122/1/webrev/
 
 One important point to consider:
 
 - The current implementations of compute, computeIfPresent, 
 computeIfAbsent, merge are implemented so that they can work correctly 
 for ConcurrentMap instances. For non-concurrent implementations it 
 might be better to fail and throw ConcurrentModification exception 
 whenever concurrent modification is detected. For regular Map 
 implementations the retry behaviour will only serve to mask errors.
 
 Thoughts?
 
 Mike
 
 On Apr 8 2013, at 11:07 , Mike Duigou wrote:
 
> Hello all;
> 
> This is a combined review for the new default methods on the 
> java.util.Map interface being a

Re: RFR: 8004518 & 8010122 : Default methods on Map

2013-04-15 Thread Mike Duigou


On Apr 14 2013, at 11:35 , Peter Levart wrote:

> 
> On 04/14/2013 07:54 PM, Peter Levart wrote:
>> Hi Mike,
>> 
>> Just a nit: The order of boolean sub-expressions in Map.replace(key, 
>> oldValue, newValue):
>> 
>>  740 if (!containsKey(key) || !Objects.equals(get(key), oldValue))
>> 
>> ...would be more optimal if reversed (like in Map.remove(key, value)).
>> 
>> Regards, Peter
> 
> I think the most optimal is actually this:
> 
> default boolean remove((Object key, Object value) {
> Object curValue = get(key);
> if (curValue == null && (value != null || !containsKey(key)) ||
> curValue != value && !curValue.equals(value)) {
> return false;
> }
> remove(key);
> return true;
> }
> 
> ...and the same for replace(key, oldValue, newValue)...

In the current patch I've used:

default boolean remove(Object key, Object value) {
Object curValue = get(key);
if (!Objects.equals(curValue, value) ||
(curValue == null && !containsKey(key))) {
return false;
}
remove(key);
return true;
}

and similar for replace(K,V,V)

I rearranged it mostly so that calling containsKey is a last resort.

Mike

> 
> Regards, Peter
> 
>> 
>> On 04/13/2013 12:02 AM, Mike Duigou wrote:
>>> I have updated the webrev with these changes and a few more.
>>> 
>>> merge() required an update to it's specification to correctly account for 
>>> null values.
>>> 
>>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/5/webrev/
>>> 
>>> This version is currently undergoing final pre-integration testing. Unless 
>>> additional problems are found or the testing fails this version will be 
>>> integrated.
>>> 
>>> Mike
>>> 
>>> On Apr 12 2013, at 12:53 , Mike Duigou wrote:
>>> 
 Thanks for the corrections. I have incorporated all of these into the 
 integration version of the patch.
 
 On Apr 12 2013, at 12:50 , Akhil Arora wrote:
 
> Hi Mike,
> 
> a few small things -
> 
> UnmodifiableMap.forEach
> is missing Objects.requireNonNull(action);
 Added.
 
> EmptyMap.replaceAll(BiFunction)
> should just return instead of throwing UnsupportedOpEx
> particularly since EmptyList.replaceAll also returns silently
> after checking if function is null to fulfil the NPE contract
 I agree. 
 
> Hashtable
> is missing a synchronized override of forEach
 added.
 
> CheckedMap.typeCheck(BiFunction)
> is missing from the patch (won't compile without it)
 Apparently I forgot a qrefresh before generating the webrev. I had this in 
 my local copy as it's necessary.
 
> On 04/11/2013 01:03 PM, Mike Duigou wrote:
>> Another revision incorporating primarily documentation feedback.
>> 
>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/2/webrev/
>> 
>> I've also included the java.util.Collections overrides for the default 
>> methods. All of these are performance enhancements--the semantics were 
>> already correct because the defaults use only public methods.
>> 
>> This is likely, modulo formatting of the source examples, the version 
>> that will be pushed to TL on Friday unless somebody squawks.
>> 
>> Moving the current compute, computeIfPresent, computeIfAbsent and merge 
>> defaults to ConcurrentMap and replacing the current Map defaults with 
>> versions that throw ConcurrentModificationException will likely be 
>> resolved in a future issue if the EG determines it to be important.
>> 
>> Mike
>> 
>> 
>> On Apr 10 2013, at 22:42 , Mike Duigou wrote:
>> 
>>> I've posted an updated webrev with the review comments I have received.
>>> 
>>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/1/webrev/
>>> 
>>> One important point to consider:
>>> 
>>> - The current implementations of compute, computeIfPresent, 
>>> computeIfAbsent, merge are implemented so that they can work correctly 
>>> for ConcurrentMap instances. For non-concurrent implementations it 
>>> might be better to fail and throw ConcurrentModification exception 
>>> whenever concurrent modification is detected. For regular Map 
>>> implementations the retry behaviour will only serve to mask errors.
>>> 
>>> Thoughts?
>>> 
>>> Mike
>>> 
>>> On Apr 8 2013, at 11:07 , Mike Duigou wrote:
>>> 
 Hello all;
 
 This is a combined review for the new default methods on the 
 java.util.Map interface being added for the JSR-335 lambda libraries. 
 The reviews are being combined because they share a common unit test.
 
 http://cr.openjdk.java.net/~mduigou/JDK-8010122/0/webrev/
 
 8004518: Add in-place operations to Map
 forEach()
 replaceAll()
 
 8010122: Add atomic operations to Map
 getOrDefault()
 putIfAbsent()  *

hg: jdk8/tl/jdk: 8008509: 6588413 changed JNIEXPORT visibility for GCC on HSX, jdk's jni_md.h needs similar change

2013-04-15 Thread martinrb

Changeset: 4ed143ddbb8a
Author:martin
Date:  2013-04-15 14:07 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/4ed143ddbb8a

8008509: 6588413 changed JNIEXPORT visibility for GCC on HSX, jdk's jni_md.h 
needs similar change
Summary: Define JNIEXPORT to use "default" visibility where possible.
Reviewed-by: coleenp, ddehaven, dcubed, anthony

! src/share/native/sun/java2d/loops/GraphicsPrimitiveMgr.h
! src/share/npt/npt.h
! src/solaris/javavm/export/jni_md.h
! src/solaris/native/sun/awt/awt_LoadLibrary.c

Re: RFR: 8004518 & 8010122 : Default methods on Map

2013-04-15 Thread Mike Duigou

On Apr 13 2013, at 09:34 , Ulf Zibis wrote:

> Am 12.04.2013 23:36, schrieb Mike Duigou:
>> On Apr 11 2013, at 15:15 , Ulf Zibis wrote:
>> 
>>> There is still a yoda style in ConcurrentMap line 72, HashMap line 361
>> Fixed
> 
> I still see no change in webrev 5 in HashMap line 361

Now corrected.

> 
>> 
>>> To be in line with old habits, please remove space after casts. See also: 
>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6939278
>> I am not going to do this.
> 
> Well in Hashtable, existing code doesn't have the space after cast. If you 
> insert space in new code, you introduce an inconsistency in style.

It seems that, per your bug, everyone has given up hope that spacing after 
casts will be done consistently. I've corrected a few cases in new code but am 
not terribly concerned about this.

> Even your new code is inconsistent, compare:

That's because I'm not the only author. I get to fix it though, the glamourous 
life of a JDK janitor. :-)

> HashTable line 917, 938, 984 etc.
> HashMap line 588 etc.
> Collections line 1402 etc.

Should be more consistent now. I'm not willing to attempt correct this 
non-problem more broadly.

> 
> I also notice, you introduce a new style ? in for( ; ... ; ... ;) expressions 
> with space before ';'.

Fixed in HashMap. I don't see this anywhere else.

> 
>>> Collections:
>>> lines 1442... + 3900... , indentation for follow up line should be 8
> 
> There are still lines 976, 1062

Of which file? According to 
http://cr.openjdk.java.net/~mduigou/JDK-8010122/5/webrev/src/share/classes/java/util/Collections.java.html

neither of those lines seems applicable.

> 
>>> Map:
>>> lines 599..600 bad indentation
>> Corrected.
> 
> Hm, I see still 3 instead 4 spaces and also in lines 545..546

Both corrected. 

> 
>> 
>>> Use consistent formatted code examples in javadoc. I like the style from 
>>> lines 567 to 570, but e.g. from line 622 to 626:
>>> - 2 spaces before 
>>> - indentation should be 4
>>> - line break before }
>> I had left these along in the previous round. Should be fixed now.
> 
> Great, but see also code samples in ConcurrentMap.

I'm not touching those. We are dependent upon syncs from JSR-166 repo for this 
class and don't want to introduce any extraneous changes. Try to convince Doug 
if you want.

Mike

Re: RFR: String.join(), StringJoiner additions

2013-04-15 Thread Martin Buchholz

Thanks for the pointer.  Yeah, that's one the pieces I think we should have
to do an optimal job of rewriting collection toString methods.


On Mon, Apr 15, 2013 at 1:03 PM, Steven Schlansker <
stevenschlans...@gmail.com> wrote:

>
> On Apr 15, 2013, at 12:21 PM, Martin Buchholz  wrote:
>
> > On Mon, Apr 15, 2013 at 11:31 AM, Martin Buchholz  >wrote:
> >
> >>
> >> OTOH, I'm guessing you are trying to improve the performance of
> operations
> >> like List.toString.
> >> More efficient (single copy char[]) would be to collect all the
> >> sub-CharSequences in a CharSequence[],  pre-compute the final length of
> the
> >> char[], allocate an array of exactly the required length, and create the
> >> final string directly from that using the package-private constructor
> (but
> >> in the unlikely event that a subsequence changed in size while
> concat'ing,
> >> be prepared to resize the array).
> >>
> >
> > Proceeding further along this train of thought, I might start with
> > AbstractCollection.toString() (and similar methods) and attempt to make
> it
> > maximally efficient.
> > Maybe add a method to JavaLangAccess to make a String directly from a
> > perfectly sized array (as needed elsewhere?).  Maybe create a
> > StringBuilder-like class that works better for typical use cases?
>
> For what it's worth, a patch that I contributed and Mike (and others) then
> rewrote
> contains this functionality already:
>
>
> http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/src/share/classes/sun/misc/JavaLangAccess.java.patch
>
> It's not merged yet though.
>
>
>

Re: RFR: String.join(), StringJoiner additions

2013-04-15 Thread Steven Schlansker


On Apr 15, 2013, at 12:21 PM, Martin Buchholz  wrote:

> On Mon, Apr 15, 2013 at 11:31 AM, Martin Buchholz wrote:
> 
>> 
>> OTOH, I'm guessing you are trying to improve the performance of operations
>> like List.toString.
>> More efficient (single copy char[]) would be to collect all the
>> sub-CharSequences in a CharSequence[],  pre-compute the final length of the
>> char[], allocate an array of exactly the required length, and create the
>> final string directly from that using the package-private constructor (but
>> in the unlikely event that a subsequence changed in size while concat'ing,
>> be prepared to resize the array).
>> 
> 
> Proceeding further along this train of thought, I might start with
> AbstractCollection.toString() (and similar methods) and attempt to make it
> maximally efficient.
> Maybe add a method to JavaLangAccess to make a String directly from a
> perfectly sized array (as needed elsewhere?).  Maybe create a
> StringBuilder-like class that works better for typical use cases?

For what it's worth, a patch that I contributed and Mike (and others) then 
rewrote
contains this functionality already:

http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/src/share/classes/sun/misc/JavaLangAccess.java.patch

It's not merged yet though.

Re: Use of super in type parameters

2013-04-15 Thread Zhong Yu

I have encountered on stackoverflow.com several legit use cases of
lower bound. And the other day Ali Lahijani raised the question that
Stream.reduce(BinaryOperator) breaks convariance of Stream, and I
thought that the root problem is lack of lower bound - the method
would have had a better signature
Stream
 Optional reduce(BinaryOperator accumulator)
So I don't think there's a lack of use cases for lower bound. (But I
have no idea how difficult it is to support it)

Zhong Yu


On Mon, Apr 15, 2013 at 2:37 PM, Martin Buchholz  wrote:
> CompletableFuture currently has a method like this:
>
> public CompletableFuture acceptEither
> (CompletableFuture other,
>  Consumer block) {
> return doAcceptEither(other, block, null);
> }
>
> But that signature is not quite correct (not as general as it could be).
>  The "correct" signature is
>
> public  CompletableFuture acceptEither
> (CompletableFuture other,
>  Consumer block) {
> return doAcceptEither(other, block, null);
> }
>
> but that fails to compile, because type parameters can only be constrained
> by extends, not super.  Is implementing this on the radar?  Angelika claims
> "lower bounds for type parameters make no sense"
> http://www.angelikalanger.com/GenericsFAQ/FAQSections/TypeParameters.html#Why%20is%20there%20no%20lower%20bound%20for%20type%20parameters
> ?
>
> but I am finding that hard to believe.  Is she right? For comparison,  the
> equivalent static method can be made to do what we want:
>
> public static  CompletableFuture acceptEither
> (CompletableFuture f,
>  CompletableFuture other,
>  Consumer block) {
> return ...
> }

Use of super in type parameters

2013-04-15 Thread Martin Buchholz

CompletableFuture currently has a method like this:

public CompletableFuture acceptEither
(CompletableFuture other,
 Consumer block) {
return doAcceptEither(other, block, null);
}

But that signature is not quite correct (not as general as it could be).
 The "correct" signature is

public  CompletableFuture acceptEither
(CompletableFuture other,
 Consumer block) {
return doAcceptEither(other, block, null);
}

but that fails to compile, because type parameters can only be constrained
by extends, not super.  Is implementing this on the radar?  Angelika claims
"lower bounds for type parameters make no sense"
http://www.angelikalanger.com/GenericsFAQ/FAQSections/TypeParameters.html#Why%20is%20there%20no%20lower%20bound%20for%20type%20parameters
?

but I am finding that hard to believe.  Is she right? For comparison,  the
equivalent static method can be made to do what we want:

public static  CompletableFuture acceptEither
(CompletableFuture f,
 CompletableFuture other,
 Consumer block) {
return ...
}

Re: RFR: String.join(), StringJoiner additions

2013-04-15 Thread Martin Buchholz

On Mon, Apr 15, 2013 at 11:31 AM, Martin Buchholz wrote:

>
> OTOH, I'm guessing you are trying to improve the performance of operations
> like List.toString.
> More efficient (single copy char[]) would be to collect all the
> sub-CharSequences in a CharSequence[],  pre-compute the final length of the
> char[], allocate an array of exactly the required length, and create the
> final string directly from that using the package-private constructor (but
> in the unlikely event that a subsequence changed in size while concat'ing,
> be prepared to resize the array).
>

 Proceeding further along this train of thought, I might start with
AbstractCollection.toString() (and similar methods) and attempt to make it
maximally efficient.
Maybe add a method to JavaLangAccess to make a String directly from a
perfectly sized array (as needed elsewhere?).  Maybe create a
StringBuilder-like class that works better for typical use cases?

Software is hard.

Re: Review request: 8004260: dynamic proxy class should have same class access as proxy interfaces

2013-04-15 Thread Mandy Chung


On 4/15/13 11:13 AM, Alan Bateman wrote:
I went through the webrev and it looks good to me and good to get this 
aligned and specified clearly.




thanks for the review.

A minor comment is that in the save for debugging code in 
generateProxyClass then you could use Files.write to write the class 
file in one method if you want. 

sure.  I can clean that up before my push.

Mandy

Re: RFR: 8004518 & 8010122 : Default methods on Map

2013-04-15 Thread Mike Duigou


On Apr 15 2013, at 00:39 , Peter Levart wrote:

> Hi Mike,
> 
> Another thing to note: Some new methods in HashMap need to call 
> inflateTable(), since patch for 8011200 has already been pushed to tl...

Yes. I actually had an off-to-the-side patch which added these and tested with 
it last week. The missing checks are now in lambda repo and part of the 8010122 
patch.

Thanks.

Mike

> 
> Regards, Peter
> 
> On 04/14/2013 08:35 PM, Peter Levart wrote:
>> 
>> On 04/14/2013 07:54 PM, Peter Levart wrote:
>>> Hi Mike,
>>> 
>>> Just a nit: The order of boolean sub-expressions in Map.replace(key, 
>>> oldValue, newValue):
>>> 
>>>  740 if (!containsKey(key) || !Objects.equals(get(key), oldValue))
>>> 
>>> ...would be more optimal if reversed (like in Map.remove(key, value)).
>>> 
>>> Regards, Peter
>> 
>> I think the most optimal is actually this:
>> 
>> default boolean remove((Object key, Object value) {
>> Object curValue = get(key);
>> if (curValue == null && (value != null || !containsKey(key)) ||
>> curValue != value && !curValue.equals(value)) {
>> return false;
>> }
>> remove(key);
>> return true;
>> }
>> 
>> ...and the same for replace(key, oldValue, newValue)...
>> 
>> Regards, Peter
>> 
>>> 
>>> On 04/13/2013 12:02 AM, Mike Duigou wrote:
 I have updated the webrev with these changes and a few more.
 
 merge() required an update to it's specification to correctly account for 
 null values.
 
 http://cr.openjdk.java.net/~mduigou/JDK-8010122/5/webrev/
 
 This version is currently undergoing final pre-integration testing. Unless 
 additional problems are found or the testing fails this version will be 
 integrated.
 
 Mike
 
 On Apr 12 2013, at 12:53 , Mike Duigou wrote:
 
> Thanks for the corrections. I have incorporated all of these into the 
> integration version of the patch.
> 
> On Apr 12 2013, at 12:50 , Akhil Arora wrote:
> 
>> Hi Mike,
>> 
>> a few small things -
>> 
>> UnmodifiableMap.forEach
>> is missing Objects.requireNonNull(action);
> Added.
> 
>> EmptyMap.replaceAll(BiFunction)
>> should just return instead of throwing UnsupportedOpEx
>> particularly since EmptyList.replaceAll also returns silently
>> after checking if function is null to fulfil the NPE contract
> I agree. 
> 
>> Hashtable
>> is missing a synchronized override of forEach
> added.
> 
>> CheckedMap.typeCheck(BiFunction)
>> is missing from the patch (won't compile without it)
> Apparently I forgot a qrefresh before generating the webrev. I had this 
> in my local copy as it's necessary.
> 
>> On 04/11/2013 01:03 PM, Mike Duigou wrote:
>>> Another revision incorporating primarily documentation feedback.
>>> 
>>> http://cr.openjdk.java.net/~mduigou/JDK-8010122/2/webrev/
>>> 
>>> I've also included the java.util.Collections overrides for the default 
>>> methods. All of these are performance enhancements--the semantics were 
>>> already correct because the defaults use only public methods.
>>> 
>>> This is likely, modulo formatting of the source examples, the version 
>>> that will be pushed to TL on Friday unless somebody squawks.
>>> 
>>> Moving the current compute, computeIfPresent, computeIfAbsent and merge 
>>> defaults to ConcurrentMap and replacing the current Map defaults with 
>>> versions that throw ConcurrentModificationException will likely be 
>>> resolved in a future issue if the EG determines it to be important.
>>> 
>>> Mike
>>> 
>>> 
>>> On Apr 10 2013, at 22:42 , Mike Duigou wrote:
>>> 
 I've posted an updated webrev with the review comments I have received.
 
 http://cr.openjdk.java.net/~mduigou/JDK-8010122/1/webrev/
 
 One important point to consider:
 
 - The current implementations of compute, computeIfPresent, 
 computeIfAbsent, merge are implemented so that they can work correctly 
 for ConcurrentMap instances. For non-concurrent implementations it 
 might be better to fail and throw ConcurrentModification exception 
 whenever concurrent modification is detected. For regular Map 
 implementations the retry behaviour will only serve to mask errors.
 
 Thoughts?
 
 Mike
 
 On Apr 8 2013, at 11:07 , Mike Duigou wrote:
 
> Hello all;
> 
> This is a combined review for the new default methods on the 
> java.util.Map interface being added for the JSR-335 lambda libraries. 
> The reviews are being combined because they share a common unit test.
> 
> http://cr.openjdk.java.net/~mduigou/JDK-8010122/0/webrev/
> 
> 8004518: Add in-place operations to Map
> forEach()
> replaceAll()

Re: RFR: String.join(), StringJoiner additions

2013-04-15 Thread Martin Buchholz

It is natural to compare StringJoiner with guava Joiner.
http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Joiner.html
Joiner is popular and has stood the test of time.

Joiner designers chose not to include a prefix and suffix, presumably
because that is an independent concern - it's not a "joining" activity.

OTOH, I'm guessing you are trying to improve the performance of operations
like List.toString.
More efficient (single copy char[]) would be to collect all the
sub-CharSequences in a CharSequence[],  pre-compute the final length of the
char[], allocate an array of exactly the required length, and create the
final string directly from that using the package-private constructor (but
in the unlikely event that a subsequence changed in size while concat'ing,
be prepared to resize the array).

On Thu, Apr 11, 2013 at 3:33 PM, Jim Gish  wrote:

> Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-**
> 7172553/ <
> http://cr.openjdk.java.net/%**7Ejgish/Bugs-5015163-7175206-**7172553/
> >
>
> These are changes that we made in lambda that we're now bringing into JDK8.
>
> I've made a couple of additions - making StringJoiner final and adding a
> couple of constructors to set the emptyOutput chars.
>
> Thanks,
>Jim
>
> --
> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
> Oracle Java Platform Group | Core Libraries Team
> 35 Network Drive
> Burlington, MA 01803
> jim.g...@oracle.com
>
>

Re: Review request: 8004260: dynamic proxy class should have same class access as proxy interfaces

2013-04-15 Thread Alan Bateman


On 12/04/2013 19:36, Mandy Chung wrote:

Dynamic proxy class is specified to be public, final, and not
abstract.  For a proxy class that implements a non-public interface,
it will be in the same package as the non-public interface
but the proxy class is accessible outside of the non-public
interface's runtime package. This change will make dynamic proxy
class to have the same Java language access as the proxy interfaces
so that creating a proxy instance to implement a non-public interface
will be guarded by the Java language access check.

Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8004260/webrev.00/

Specdiff at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/8004260/specdiff/overview-summary.html 



This change also updates the spec to specify the permission checks
by Proxy.getProxyClass and Proxy.newProxyInstance methods and removes
the system properties which provide a workaround for 7u releases and
not needed in jdk8.

Thanks
Mandy
I went through the webrev and it looks good to me and good to get this 
aligned and specified clearly.


A minor comment is that in the save for debugging code in 
generateProxyClass then you could use Files.write to write the class 
file in one method if you want.


-Alan.

Re: RFR: String.join(), StringJoiner additions

2013-04-15 Thread Martin Buchholz

Is "i+1" really preferred to "i + 1" ?  I thought it was the reverse, and
that "i+1" was merely tolerated.

1570 if (value[i] == hi && value[i+1] == lo) {

---

2425  * @throws NullPointerException If {@code delimiter} is {@code null}

you also throw NPE for element null, but you didn't specify that.
*
*
*---*



On Thu, Apr 11, 2013 at 3:33 PM, Jim Gish  wrote:

> Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-**
> 7172553/ <
> http://cr.openjdk.java.net/%**7Ejgish/Bugs-5015163-7175206-**7172553/
> >
>
> These are changes that we made in lambda that we're now bringing into JDK8.
>
> I've made a couple of additions - making StringJoiner final and adding a
> couple of constructors to set the emptyOutput chars.
>
> Thanks,
>Jim
>
> --
> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
> Oracle Java Platform Group | Core Libraries Team
> 35 Network Drive
> Burlington, MA 01803
> jim.g...@oracle.com
>
>

Re: RFR: String.join(), StringJoiner additions

2013-04-15 Thread Martin Buchholz

You are fiddling with the javadoc for getChars, which is an independent
change.  (I am also fiddling with getChars in another ongoing change).  I
don't think closing html tags for  are required in javadoc.  If you are
going to change the exception javadoc, then also change @exception to
@throws.

On Thu, Apr 11, 2013 at 3:33 PM, Jim Gish  wrote:

> Please review http://cr.openjdk.java.net/~**jgish/Bugs-5015163-7175206-**
> 7172553/ <
> http://cr.openjdk.java.net/%**7Ejgish/Bugs-5015163-7175206-**7172553/
> >
>
> These are changes that we made in lambda that we're now bringing into JDK8.
>
> I've made a couple of additions - making StringJoiner final and adding a
> couple of constructors to set the emptyOutput chars.
>
> Thanks,
>Jim
>
> --
> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
> Oracle Java Platform Group | Core Libraries Team
> 35 Network Drive
> Burlington, MA 01803
> jim.g...@oracle.com
>
>

Re: RFR: String.join(), StringJoiner additions

2013-04-15 Thread Alan Bateman


On 12/04/2013 16:02, Alan Bateman wrote:

On 11/04/2013 23:33, Jim Gish wrote:
Please review 
http://cr.openjdk.java.net/~jgish/Bugs-5015163-7175206-7172553/ 



These are changes that we made in lambda that we're now bringing into 
JDK8.


I've made a couple of additions - making StringJoiner final and 
adding a couple of constructors to set the emptyOutput chars.


Thanks,
   Jim


:

I plan to look at StringJoiner in more detail later.


Just to follow up with a few comments on StringJoiner.

I don't know how "final" this is but I wonder if you've already 
experimented (and rejected) having a smaller set of constructors? I will 
guess that the most popular usage will be the simple 1-arg constructor 
to just specify the delimiter. There will likely be some cases where you 
want a prefix and suffix too. I bring this up because it seems a bit 
inconsistent to just have a setter for the default result when one could 
as easily have a method to set the prefix/suffix too. Clearly it would 
complicate the implementation a bit but it could be optimized for the 
case that these are set before any elements are added. Anyway, I'm not 
trying to re-open discussions on this, just trying to understand if what 
you are proposing is already close to final.


On method names then "setEmptyOutput" doesn't seem quite right, I wonder 
if you've considered others, like setEmptyValue or setDefaultResult.


Minor nits:

- The javadoc for "add" starts with "add the supplied", should be "Add".

- The @param in the 1-arg constructor is indented inconsistently to the 
other methods


- The this(...) usage in the 3-arg constructor has spaces around it so 
it is inconsistent with the other usages.


- In the class description it reads "Prior to adding something to the 
StringJoiner, {@code sj.toString()}  will, by default, return {@code 
prefix+suffix}". This might be better as "Prior to adding elements to a 
StringJoiner, its toString() method, if invoked, will ...".


- The comma in "For example," set expectations that there will be more 
text after the example but this isn't so.


- As with the comments on String.join then I assume the test should have 
1 bug number (not 3). Also to be consistent with the existing 
organization it would be better to move it down to 
test/java/util/StringJoiner (I know we have to come up with a solution 
for tests with package names).


- The test has two @summary lines, I assume this is a mistake.

- In terms of code coverage then it looks like the only method that is 
tested for NPE is setEmptyOutput.


That's all I have.

-Alan

Re: 8012237: CompletableFuture/Basic.java still fails intermittently

2013-04-15 Thread Chris Hegarty

On 15/04/2013 18:32, Martin Buchholz wrote:

Thanks. This looks good.

I might additionally:
- rename "phaser" to something more evocative, like "cf3Done"
- add a checkCompletedXXX for the tardy future after the phaser arrives
- check that cf3 result does not change when tardy future completes.

Thanks Martin, I'll make these changes before the pushing.

-Chris.

On Mon, Apr 15, 2013 at 10:03 AM, Chris Hegarty
mailto:chris.hega...@oracle.com>> wrote:

On 15/04/2013 17:32, Martin Buchholz wrote:

This looks good, in that it fixes the flakiness.

Thanks Martin.

I don't think we have tests yet that ensure Either completion
when only
one task completes.
Consider writing one normal async supplier and one that waits on a
latch; ensure that the Either future completes with the normal
value,
then trip the latch to allow the second one to finish.

Great idea. Here you go:

http://cr.openjdk.java.net/~__chegar/8012237/webrev.01/__webrev/test/java/util/__concurrent/CompletableFuture/__Basic.java.udiff.html

-Chris.

On Mon, Apr 15, 2013 at 7:09 AM, Chris Hegarty
mailto:chris.hega...@oracle.com>
>> wrote:

I missed three cases in the previous change [1]. That will
teach me
for working on the weekend ;-)

A full audit of the use of the XxxEitherXxx methods in the
test has
been done, and there are still three particular checks that are
possibly incorrect. The failure is not always seen as this
is racy code.

http://cr.openjdk.java.net/~chegar/8012237/webrev.00/webrev/test/java/util/concurrent/CompletableFuture/Basic.java.udiff.html

Thanks,
-Chris.

[1]

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/016013.html

Re: 8012237: CompletableFuture/Basic.java still fails intermittently

2013-04-15 Thread Martin Buchholz

Thanks.  This looks good.

I might additionally:
- rename "phaser" to something more evocative, like "cf3Done"
- add a checkCompletedXXX for the tardy future after the phaser arrives
- check that cf3 result does not change when tardy future completes.


On Mon, Apr 15, 2013 at 10:03 AM, Chris Hegarty wrote:

> On 15/04/2013 17:32, Martin Buchholz wrote:
>
>> This looks good, in that it fixes the flakiness.
>>
>
> Thanks Martin.
>
>
>  I don't think we have tests yet that ensure Either completion when only
>> one task completes.
>> Consider writing one normal async supplier and one that waits on a
>> latch; ensure that the Either future completes with the normal value,
>> then trip the latch to allow the second one to finish.
>>
>
> Great idea. Here you go:
>
> http://cr.openjdk.java.net/~**chegar/8012237/webrev.01/**
> webrev/test/java/util/**concurrent/CompletableFuture/**
> Basic.java.udiff.html
>
> -Chris.
>
>
>>
>> On Mon, Apr 15, 2013 at 7:09 AM, Chris Hegarty > > wrote:
>>
>> I missed three cases in the previous change [1]. That will teach me
>> for working on the weekend ;-)
>>
>> A full audit of the use of the XxxEitherXxx methods in the test has
>> been done, and there are still three particular checks that are
>> possibly incorrect. The failure is not always seen as this is racy
>> code.
>>
>> http://cr.openjdk.java.net/~__**chegar/8012237/webrev.00/__**
>> webrev/test/java/util/__**concurrent/CompletableFuture/_**
>> _Basic.java.udiff.html
>>
>> > webrev/test/java/util/**concurrent/CompletableFuture/**
>> Basic.java.udiff.html
>> >
>>
>> Thanks,
>> -Chris.
>>
>> [1]
>> http://mail.openjdk.java.net/_**_pipermail/core-libs-dev/2013-**
>> __April/016013.html
>> > April/016013.html
>> >
>>
>>
>>

Re: 8012237: CompletableFuture/Basic.java still fails intermittently

2013-04-15 Thread Martin Buchholz

This looks good, in that it fixes the flakiness.

I don't think we have tests yet that ensure Either completion when only one
task completes.
Consider writing one normal async supplier and one that waits on a latch;
ensure that the Either future completes with the normal value, then trip
the latch to allow the second one to finish.

On Mon, Apr 15, 2013 at 7:09 AM, Chris Hegarty wrote:

> I missed three cases in the previous change [1]. That will teach me for
> working on the weekend ;-)
>
> A full audit of the use of the XxxEitherXxx methods in the test has been
> done, and there are still three particular checks that are possibly
> incorrect. The failure is not always seen as this is racy code.
>
> http://cr.openjdk.java.net/~**chegar/8012237/webrev.00/**
> webrev/test/java/util/**concurrent/CompletableFuture/**
> Basic.java.udiff.html
>
> Thanks,
> -Chris.
>
> [1] http://mail.openjdk.java.net/**pipermail/core-libs-dev/2013-**
> April/016013.html
>

Re: 8012237: CompletableFuture/Basic.java still fails intermittently

2013-04-15 Thread Chris Hegarty


On 15/04/2013 17:32, Martin Buchholz wrote:

This looks good, in that it fixes the flakiness.


Thanks Martin.


I don't think we have tests yet that ensure Either completion when only
one task completes.
Consider writing one normal async supplier and one that waits on a
latch; ensure that the Either future completes with the normal value,
then trip the latch to allow the second one to finish.


Great idea. Here you go:

http://cr.openjdk.java.net/~chegar/8012237/webrev.01/webrev/test/java/util/concurrent/CompletableFuture/Basic.java.udiff.html

-Chris.




On Mon, Apr 15, 2013 at 7:09 AM, Chris Hegarty mailto:chris.hega...@oracle.com>> wrote:

I missed three cases in the previous change [1]. That will teach me
for working on the weekend ;-)

A full audit of the use of the XxxEitherXxx methods in the test has
been done, and there are still three particular checks that are
possibly incorrect. The failure is not always seen as this is racy code.


http://cr.openjdk.java.net/~__chegar/8012237/webrev.00/__webrev/test/java/util/__concurrent/CompletableFuture/__Basic.java.udiff.html



Thanks,
-Chris.

[1]

http://mail.openjdk.java.net/__pipermail/core-libs-dev/2013-__April/016013.html

Re: JEP 180: Handle Frequent HashMap Collisions with Balanced Trees

2013-04-15 Thread Tom Hawtin


On 05/04/2013 19:08, mark.reinh...@oracle.com wrote:

Posted: http://openjdk.java.net/jeps/180


If you're using WeakHashMap with types that (may) override 
equals/hashCode you have bigger problems. You almost certainly want 
"identity" semantics (violating the java.util.Map contract) and quite 
possibly concurrency.


Tom

Re: RFR 8010280: jvm.cfg needs updating for non-server builds

2013-04-15 Thread Mike Duigou

Hi David;

I remember reviewing the jvm.cfg config patch for JDK 7. I had hoped to see the 
"classic" and "green" flags go away and some of the other legacy flags like 
"-hotspot" reduced to WARN. What's the difference between removing an entry 
completely and retaining it with "ERROR"?

Additionally I don't like that aliases have differing definitions and some 
confusing ones like "-server ALIASED_TO -client". Is this necessary or just 
historically convenient?

Mike

On Apr 14 2013, at 17:18 , David Holmes wrote:

> Some background.
> 
> The jvm.cfg file, for which there is a per-architecture committed file in the 
> repository, controls which VM's (client, server, minimal) are known, which is 
> the default, whether there are other aliases and whether ergonomic selection 
> is used.
> 
> Historically things were simple:
> - 64-bit platforms had server only
> - 32-bit platforms had client and server
> 
> then we acknowledged that some platforms may be client only and we added some 
> support (originally in the old build then converted to the new build) for 
> dynamically creating a jvm.cfg for the case of building client only.
> 
> Then the minimal VM was introduced and we potentially have three VMs to 
> handle. To address this we initially added "-minimal KNOWN" to all the 
> jvm.cfg files for platforms known to support the minimal VM - this was done 
> under JDK-7198815 (and those changes are now reversed by this changeset.)
> 
> The problem after minimal was introduced was that the logic for "building 
> client only" didn't account for building minimal (only or combined with 
> client) and we need support for not-building-server. And that is what this 
> changeset does.
> 
> This only affects 32-bit builds as there is no client nor minimal VM on 
> 64-bit. The basic operation is as follows:
> 
> - If building client+server then we use the committed jvm.cfg (which handles 
> ergonomics if applicable), adding a "-minimal KNOWN" line if minimal is also 
> selected;
> - Otherwise we dynamically generate a jvm.cfg for the set of VMs being built, 
> using these simple rules:
>  - if client or server are present they are default
>  - if client and/or server is absent then the absent VM is aliased to the 
> default VM in that config
>  - if minimal is not selected then it is absent from the jvm.cfg (we do not 
> add any aliases for minimal**).
> 
> ** The alias mechanism is useful for deprecating legacy VM names, and has 
> also made testing more convenient. However I think it is a flawed mechanism 
> for testing and our internal test infrastructure is moving away from 
> arbitrarily using -client/-server when actually running server/client. If you 
> ask for the minimal VM and it is not available I think you should get an 
> error not silent use of a different VM. (Note: this selection doesn't affect 
> SE Embedded as it defines jvm.cfg files using it's own rules/preferences.)
> 
> webrev:
> 
> http://cr.openjdk.java.net/~dholmes/8010280/webrev/
> 
> Thanks,
> David

Suggestion to improve profile/package in javadoc

2013-04-15 Thread Paul Benedict

I was looking at b85 javadocs, and saw this at the page's top for a class:

compact1, compact2, compact3
java.lang

I don't think it's clear what is being shown. I understand it -- but I am
following the feature closely. I recommend prefixing the data because the
package is no longer the sole item:

Profiles: compact1, compact2, compact3
Package: java.lang

Paul

RFR: JDK-8008738 - Issue in com.sun.org.apache.xml.internal.serializer.Encodings causes some JCK tests to fail intermittently

2013-04-15 Thread Daniel Fuchs


Hi,

This a fix for:

JDK-8008738 - Issue in 
com.sun.org.apache.xml.internal.serializer.Encodings causes some JCK 
tests to fail intermittently.




The issue here is that com.sun.org.apache.xml.internal.serializer.Encodings
tries to implement a double mapping:

   Java Charset Name => Preferred XML Mime Name

and

   XML Mime Name => Java Charset Name

from a specification that it reads from an Encoding.properties file.

However, there can be several 'Java' names (aliases) corresponding to
a given Charset, and there could also be several XML Mime Names,
corresponding to that same charset.

The Encodings.properties files uses 'Java Names' as keys, which it
can map to one - or more - XML mime names, where the first XML name
in the list stands for the 'preferred' mime name.

The trouble is that some of the Java names present in the
Encodings.properties files are not recognized by the Charset API,
although some of the corresponding XML mime names, can be.

This resulted in the creation of EncodingInfo objects whose charset
name was not recognized by the Charset API. However, since there can
be several Java Names specified with the same XML name, this did
not always lead to errors:
when 'converting' an XML name to a Java Name, if the first
mapping picked up had a recognized Java Name - everything went well.
If however - the first mapping picked up had an unrecognized Java Name,
then this lead to failure to encode characters properly.
Since the order in which the EncodingInfo where registered depended on
the order of keys/values returned by HashMap/Properties - it could
work in one run and fail in another.

The proposed changeset is reworking the algorithm that
creates EncodingInfo objects and perform the mapping
Java Name <-> Mime Name:

* Parse the whole line and consider both Java Names & Mime Names when
  trying to instantiate EncodingInfo objects for a Java Name,
* Make sure the javaName recorded in EncodingInfo is one that is
  recognized by Charset.forName().

The changeset has a unit test (under jdk/test/javax/xml/jaxp) which

1. verifies that the Encodings.properties file has no inconsistencies

2. verifies the mapping implemented by Encodings.java - by calling
   Encodings.convertMime2JavaEncoding() and
   Encodings.getMimeEncoding()
   (skipping those names for which no charset could be loaded, as
   not all charset are necessarily available on all machines).

-- daniel

8012237: CompletableFuture/Basic.java still fails intermittently

2013-04-15 Thread Chris Hegarty

I missed three cases in the previous change [1]. That will teach me for 
working on the weekend ;-)


A full audit of the use of the XxxEitherXxx methods in the test has been 
done, and there are still three particular checks that are possibly 
incorrect. The failure is not always seen as this is racy code.


http://cr.openjdk.java.net/~chegar/8012237/webrev.00/webrev/test/java/util/concurrent/CompletableFuture/Basic.java.udiff.html

Thanks,
-Chris.

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/016013.html

hg: jdk8/tl/nashorn: 9 new changesets

2013-04-15 Thread sundararajan . athijegannathan

Changeset: 635a93b61d34
Author:hannesw
Date:  2013-04-10 14:00 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/635a93b61d34

8011714: Regexp decimal escape handling still not correct
Reviewed-by: lagergren, attila

! src/jdk/nashorn/internal/runtime/regexp/RegExpScanner.java
+ test/script/basic/JDK-8011714.js
+ test/script/basic/JDK-8011714.js.EXPECTED

Changeset: b4ea8678bf15
Author:hannesw
Date:  2013-04-10 14:05 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/b4ea8678bf15

8011749: Bugs with empty character class handling
Reviewed-by: lagergren, attila

! src/jdk/nashorn/internal/runtime/regexp/RegExpScanner.java
+ test/script/basic/JDK-8011749.js
+ test/script/basic/JDK-8011749.js.EXPECTED

Changeset: 8ae9ed1ac1e2
Author:hannesw
Date:  2013-04-10 14:08 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/8ae9ed1ac1e2

8011756: Wrong characters supported in RegExp \c escape
Reviewed-by: lagergren, attila

! src/jdk/nashorn/internal/runtime/regexp/RegExpScanner.java
+ test/script/basic/JDK-8011756.js
+ test/script/basic/JDK-8011756.js.EXPECTED

Changeset: 571e06d5d23c
Author:sundar
Date:  2013-04-11 13:20 +0530
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/571e06d5d23c

8011960: [2,1].sort(null) should throw TypeError
Reviewed-by: hannesw, lagergren

! src/jdk/nashorn/internal/objects/NativeArray.java
+ test/script/basic/JDK-8011960.js

Changeset: 256bb030ce0a
Author:sundar
Date:  2013-04-11 15:04 +0530
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/256bb030ce0a

8011974: Comparator function returning negative and positive Infinity does not 
work as expected with Array.prototype.sort
Reviewed-by: hannesw, lagergren

! src/jdk/nashorn/internal/objects/NativeArray.java
+ test/script/basic/JDK-8011974.js

Changeset: a3fc89d33072
Author:hannesw
Date:  2013-04-11 12:16 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/a3fc89d33072

8011980: Allow NUL character in character class
Reviewed-by: sundar, lagergren

! src/jdk/nashorn/internal/runtime/regexp/RegExpScanner.java
+ test/script/basic/JDK-8011980.js
+ test/script/basic/JDK-8011980.js.EXPECTED

Changeset: ed4293ceec0e
Author:hannesw
Date:  2013-04-12 16:31 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/ed4293ceec0e

8011884: Regexp literals are compiled twice
Reviewed-by: lagergren, sundar

! src/jdk/nashorn/internal/runtime/regexp/joni/Analyser.java
! src/jdk/nashorn/internal/runtime/regexp/joni/Regex.java
! src/jdk/nashorn/internal/runtime/regexp/joni/ast/QuantifierNode.java

Changeset: 36e36a2d4312
Author:hannesw
Date:  2013-04-12 16:32 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/36e36a2d4312

8011885: Switch to Joni as default Regexp engine
Reviewed-by: lagergren, sundar

! src/jdk/nashorn/internal/runtime/regexp/RegExpFactory.java

Changeset: e70e6b38826b
Author:jlaskey
Date:  2013-04-15 08:39 -0300
URL:   http://hg.openjdk.java.net/jdk8/tl/nashorn/rev/e70e6b38826b

Merge

hg: jdk8/tl/langtools: 6 new changesets

2013-04-15 Thread maurizio . cimadamore

Changeset: b26f36a7ae3b
Author:mcimadamore
Date:  2013-04-15 14:11 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/b26f36a7ae3b

8011383: Symbol.getModifiers omits ACC_ABSTRACT from interface with default 
methods
Summary: Fixup for default method modifiers erroneously applies to class-level 
modifiers
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/code/Symbol.java
+ test/tools/javac/defaultMethods/DefaultMethodFlags.java

Changeset: c430f1cde21c
Author:mcimadamore
Date:  2013-04-15 14:12 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/c430f1cde21c

8011377: Javac crashes when multiple lambdas are defined in an array
Summary: Wrong attribution environment used by DeferredAttr
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/comp/Flow.java
+ test/tools/javac/lambda/TargetType71.java

Changeset: 083c6b199e2f
Author:mcimadamore
Date:  2013-04-15 14:15 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/083c6b199e2f

8011376: Spurious checked exception errors in nested method call
Summary: Fallback attribution logic doesn't work properly when lambda throws 
checked exceptions
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/comp/Attr.java
+ test/tools/javac/lambda/TargetType72.java

Changeset: 6dacab087652
Author:mcimadamore
Date:  2013-04-15 14:16 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/6dacab087652

8011028: lang/INFR/infr001/infr00101md/infr00101md.java fails to compile after 
switch to JDK8-b82
Summary: Fix bug in Types.removeWildcards
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/code/Types.java
! test/tools/javac/lambda/TargetType69.java
+ test/tools/javac/lambda/TargetType70.java

Changeset: c2315af9cc28
Author:mcimadamore
Date:  2013-04-15 14:17 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/c2315af9cc28

8011392: Missing checkcast when casting to intersection type
Summary: javac should emit a checkcast for each additional target type 
specified in an intersection type cast
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/comp/TransTypes.java
+ test/tools/javac/lambda/Intersection03.java

Changeset: 950e8ac120f0
Author:mcimadamore
Date:  2013-04-15 14:18 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/950e8ac120f0

8010923: Avoid redundant speculative attribution
Summary: Add optimization to avoid speculative attribution for certain argument 
expressions
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/comp/Attr.java
! src/share/classes/com/sun/tools/javac/comp/DeferredAttr.java
! src/share/classes/com/sun/tools/javac/comp/Resolve.java
! src/share/classes/com/sun/tools/javac/tree/TreeInfo.java

RE: Code review request for 8012044: Give more information about self-suppression from Throwable.addSuppressed

2013-04-15 Thread Jason Mehrens

David,

The last two paragraphs of Throwable.addSuppressed cover this.  The first is 
your argument bellow and does not forbid anything it claims.  The last 
paragraph explicitly enables this patch.

Jason

> Date: Mon, 15 Apr 2013 12:56:42 +1000
> From: david.hol...@oracle.com
> To: joe.da...@oracle.com
> CC: jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net
> Subject: Re: Code review request for 8012044: Give more information about 
> self-suppression from Throwable.addSuppressed
> 
> On 13/04/2013 5:08 AM, Joe Darcy wrote:
> > On 04/12/2013 11:22 AM, Jason Mehrens wrote:
> >> The landmines are the retrofitted exception classes as shown here
> >> https://netbeans.org/bugzilla/show_bug.cgi?id=150969 and
> >> https://issues.jboss.org/browse/JBREM-552. Really, if the ISE or IAE
> >> is thrown it is going to suppress 'this' and 'cause'. It would be
> >> nice to see the given 'cause' show up in a log file when tracking down
> >> this type of bug.
> >
> > Okay; fair enough. Updated webrev covering initCause too at
> >
> > http://cr.openjdk.java.net/~darcy/8012044.1/
> >
> > New patch below.
> >
> > (It is a bit of stretch to have this in initiCause by listed as the
> > "cause" of the IllegalStateException; as an alternative, the
> > IllegalStateException could have both this and the cause as suppressed
> > exceptions.)
> 
> I don't think that is valid for initCause. If I call initCause twice in 
> succession there need not even be any exception propagation in progress. 
> The ISE that gets thrown is not suppressing anything.
> 
> For setSuppressed this might make sense for the compiler generated 
> sequences, but again if I just called setSuppressed with an invalid 
> value, the ISE is not suppressing any existing exception.
> 
> David

Re: RFR 8010280: jvm.cfg needs updating for non-server builds

2013-04-15 Thread David Holmes


On 15/04/2013 7:03 PM, Erik Joelsson wrote:

Hello,

It looks to me like you are covering the case of both server and client
being true twice. How could line 319 ever evaluate to true? Otherwise it
looks ok.


You are right - that was a leftover from an initial approach that only 
looked for client+server but excluded client+server+minimal. Back to the 
drawing board.


Thanks,
David


/Erik

On 2013-04-15 02:18, David Holmes wrote:

Some background.

The jvm.cfg file, for which there is a per-architecture committed file
in the repository, controls which VM's (client, server, minimal) are
known, which is the default, whether there are other aliases and
whether ergonomic selection is used.

Historically things were simple:
- 64-bit platforms had server only
- 32-bit platforms had client and server

then we acknowledged that some platforms may be client only and we
added some support (originally in the old build then converted to the
new build) for dynamically creating a jvm.cfg for the case of building
client only.

Then the minimal VM was introduced and we potentially have three VMs
to handle. To address this we initially added "-minimal KNOWN" to all
the jvm.cfg files for platforms known to support the minimal VM - this
was done under JDK-7198815 (and those changes are now reversed by this
changeset.)

The problem after minimal was introduced was that the logic for
"building client only" didn't account for building minimal (only or
combined with client) and we need support for not-building-server. And
that is what this changeset does.

This only affects 32-bit builds as there is no client nor minimal VM
on 64-bit. The basic operation is as follows:

- If building client+server then we use the committed jvm.cfg (which
handles ergonomics if applicable), adding a "-minimal KNOWN" line if
minimal is also selected;
- Otherwise we dynamically generate a jvm.cfg for the set of VMs being
built, using these simple rules:
  - if client or server are present they are default
  - if client and/or server is absent then the absent VM is aliased to
the default VM in that config
  - if minimal is not selected then it is absent from the jvm.cfg (we
do not add any aliases for minimal**).

** The alias mechanism is useful for deprecating legacy VM names, and
has also made testing more convenient. However I think it is a flawed
mechanism for testing and our internal test infrastructure is moving
away from arbitrarily using -client/-server when actually running
server/client. If you ask for the minimal VM and it is not available I
think you should get an error not silent use of a different VM. (Note:
this selection doesn't affect SE Embedded as it defines jvm.cfg files
using it's own rules/preferences.)

webrev:

http://cr.openjdk.java.net/~dholmes/8010280/webrev/

Thanks,
David

Re: [OpenJDK 2D-Dev] sun.java2D.Pisces renderer Performance and Memory enhancements

2013-04-15 Thread Laurent Bourgès

Jim, Andrea,

I updated MapBench to provide test statistics (avg, median, stddev, rms,
med + stddev, min, max) and CSV output (tab separator):
http://jmmc.fr/~bourgesl/share/java2d-pisces/MapBench/


Here are the results (OpenJDK8 Ref vs Patched):
http://jmmc.fr/~bourgesl/share/java2d-pisces/ref_det.log
http://jmmc.fr/~bourgesl/share/java2d-pisces/patch_det.log

   test threads ops Tavg Tmed stdDev rms Med+Stddev min max  boulder_17 1 20
180,22% 181,08% 1186,01% 181,17% 185,92% 176,35% 170,36%  boulder_17 2 20
183,15% 183,80% 162,68% 183,78% 183,17% 174,01% 169,89%  boulder_17 4 20
216,62% 218,03% 349,31% 218,87% 226,68% 172,15% 167,54%  shp_alllayers_47 1
20 243,90% 244,86% 537,92% 244,87% 246,39% 240,64% 231,00%  shp_alllayers_47
2 20 286,42% 287,07% 294,87% 287,07% 287,23% 277,19% 272,23%
shp_alllayers_47 4 20 303,08% 302,15% 168,19% 301,90% 295,90% 462,70%
282,41%

PATCH:
   test threads ops Tavg Tmed stdDev rms Med+Stddev min max  boulder_17 1 20
110,196 109,244 0,529 109,246 109,773 108,197 129,327  boulder_17 2 40
127,916 127,363 3,899 127,423 131,262 125,262 151,561  boulder_17 4 80
213,085 212,268 14,988 212,796 227,256 155,512 334,407  shp_alllayers_47 1
20 1139,452 1134,858 5,971 1134,873 1140,829 1125,859 1235,746
shp_alllayers_47 2 40 1306,889 1304,598 28,157 1304,902 1332,755 1280,49
1420,351  shp_alllayers_47 4 80 2296,487 2303,81 112,816 2306,57 2416,626
1390,31 2631,455

REF:
   test threads ops Tavg Tmed stdDev rms Med+Stddev min max  boulder_17 1 20
198,591 197,816 6,274 197,916 204,091 190,805 220,319  boulder_17 2 40
234,272 234,09 6,343 234,176 240,433 217,967 257,485  boulder_17 4 80
461,579 462,8 52,354 465,751 515,153 267,712 560,254  shp_alllayers_47 1 20
2779,133 2778,823 32,119 2779,009 2810,943 2709,285 2854,557
shp_alllayers_47 2 40 3743,255 3745,111 83,027 3746,031 3828,138 3549,364
3866,612  shp_alllayers_47 4 80 6960,23 6960,948 189,75 6963,533 7150,698
6432,945 7431,541
Linux 64 server vm
JVM: -Xms128m -Xmx128m (low mem)

Laurent

2013/4/14 Andrea Aime 

> On Tue, Apr 9, 2013 at 3:02 PM, Laurent Bourgès  > wrote:
>
>> Dear Java2D members,
>>
>> Could someone review the following webrev concerning Java2D Pisces to
>> enhance its performance and reduce its memory footprint (RendererContext
>> stored in thread local or concurrent queue):
>> http://jmmc.fr/~bourgesl/share/java2d-pisces/webrev-1/
>>
>> FYI I fixed file headers in this patch and signed my OCA 3 weeks ago.
>>
>> Remaining work:
>> - cleanup (comments ...)
>> - statistics to perform auto-tuning
>> - cache / memory cleanup (SoftReference ?): use hints or System
>> properties to adapt it to use cases
>> - another problem: fix clipping performance in Dasher / Stroker for
>> segments out of bounds
>>
>> Could somebody support me ? ie help me working on these tasks or just to
>> discuss on Pisces algorithm / implementation ?
>>
>
> Hi,
> I would like to express my support for this patch.
> Given that micro-benchmarks have already been run, I took the patch for a
> spin in a large, real world benchmark instead,
> the OSGeo WMS Shootout 2010 benchmark, for which you can see the results
> here:
>
> http://www.slideshare.net/gatewaygeomatics.com/wms-performance-shootout-2010
>
> The presentation is long, but suffice it to say all Java based
> implementations took quite the beating due to the
> poor scalability of Ductus with antialiased rendering of vector data (for
> an executive summary just look
> at slide 27 and slide 66, where GeoServer, Oracle MapViewer and
> Constellation SDI were the
> Java based ones)
>
> I took the same tests and run them again on my machine (different hardware
> than the tests, don't try to compare
> the absolute values), using Oracle JDK 1.7.0_17, OpenJDK 8 (a checkout a
> couple of weeks old) and the
> same, but with Laurent's patches applied.
> Here are the results, throughput (in maps generated per second) with the
> load generator (JMeter) going
> up from one client to 64 concurrent clients:
>
>*Threads* *JDK 1.7.0_17* *OpenJDK 8, vanilla* *OpenJDK 8 + pisces
> renderer improvements* *Pisces renderer performance gain, %*  1 13,97
> 12,43 13,03 4,75%  2 22,08 20,60 20,77 0,81%  4 34,36 33,15 33,36 0,62%  8
> 39,39 40,51 41,71 2,96%  16 40,61 44,57 46,98 5,39%  32 41,41 44,73 48,16
> 7,66%  64 37,09 42,19 45,28 7,32%
>
> Well, first of all, congratulations to the JDK developers, don't know what
> changed in JDK 8, but
> GeoServer seems to like it quite a bit :-).
> That said, Laurent's patch also gives a visible boost, especially when
> several concurrent clients are
> asking for the maps.
>
> Mind, as I said, this is no micro-benchmark, it is a real application
> loading doing a lot of I/O
> (from the operating system file cache), other processing before the data
> reaches the rendering
> pipeline, and then has to PNG encode the output BufferedImage (PNG
> encoding being rather
> expensive), so getting this speedup from just a change in the rendering
> pipeline is significant.
>
> Long s

Re: JDK-8011653: Upgrade to JAXP 1.5

2013-04-15 Thread Alan Bateman


On 15/04/2013 08:48, Joe Wang wrote:

:



For the new properties then it specifies that a "a runtime exception" 
will be thrown. Can this be more specific?


They can't be in XMLConstants, but they are in the specific Factories. 
The properties may be supported by factories that may throw different 
exceptions.
I think it would be help if this were expanded to something like "a 
runtime exception that is specific to the context is thrown" and give an 
example so that it's clear what it saying.




Webrevs updated:
http://cr.openjdk.java.net/~joehw/jdk8/8011653/webrev/
This looks much better. For now, I've stayed focused on the javadoc/spec 
for now as we have to get that right.


The wording "\"jar\" plus the scheme portion" suggests it matches "jar" 
exactly and maybe this could be clearer because this is also case 
insensitive.


@since on the new properties 1.7. I don't know if this should have 1.8 
or JAXP 1.5.


The intending of the  and  looks a bit odd when the paragraphs 
aren't indented. This doesn't impact the generated javadoc of course, 
just looks odd in the source code.


Otherwise I think the javadoc looks okay to me.

-Alan

Re: RFR 8010280: jvm.cfg needs updating for non-server builds

2013-04-15 Thread Erik Joelsson


Hello,

It looks to me like you are covering the case of both server and client 
being true twice. How could line 319 ever evaluate to true? Otherwise it 
looks ok.


/Erik

On 2013-04-15 02:18, David Holmes wrote:

Some background.

The jvm.cfg file, for which there is a per-architecture committed file 
in the repository, controls which VM's (client, server, minimal) are 
known, which is the default, whether there are other aliases and 
whether ergonomic selection is used.


Historically things were simple:
- 64-bit platforms had server only
- 32-bit platforms had client and server

then we acknowledged that some platforms may be client only and we 
added some support (originally in the old build then converted to the 
new build) for dynamically creating a jvm.cfg for the case of building 
client only.


Then the minimal VM was introduced and we potentially have three VMs 
to handle. To address this we initially added "-minimal KNOWN" to all 
the jvm.cfg files for platforms known to support the minimal VM - this 
was done under JDK-7198815 (and those changes are now reversed by this 
changeset.)


The problem after minimal was introduced was that the logic for 
"building client only" didn't account for building minimal (only or 
combined with client) and we need support for not-building-server. And 
that is what this changeset does.


This only affects 32-bit builds as there is no client nor minimal VM 
on 64-bit. The basic operation is as follows:


- If building client+server then we use the committed jvm.cfg (which 
handles ergonomics if applicable), adding a "-minimal KNOWN" line if 
minimal is also selected;
- Otherwise we dynamically generate a jvm.cfg for the set of VMs being 
built, using these simple rules:

  - if client or server are present they are default
  - if client and/or server is absent then the absent VM is aliased to 
the default VM in that config
  - if minimal is not selected then it is absent from the jvm.cfg (we 
do not add any aliases for minimal**).


** The alias mechanism is useful for deprecating legacy VM names, and 
has also made testing more convenient. However I think it is a flawed 
mechanism for testing and our internal test infrastructure is moving 
away from arbitrarily using -client/-server when actually running 
server/client. If you ask for the minimal VM and it is not available I 
think you should get an error not silent use of a different VM. (Note: 
this selection doesn't affect SE Embedded as it defines jvm.cfg files 
using it's own rules/preferences.)


webrev:

http://cr.openjdk.java.net/~dholmes/8010280/webrev/

Thanks,
David

Re: JDK-8011653: Upgrade to JAXP 1.5

2013-04-15 Thread Joe Wang



On 4/8/2013 5:24 AM, Alan Bateman wrote:

On 08/04/2013 08:39, huizhe wang wrote:

Hi Lance, Alan,

As I mentioned, I'd like to propose an integration of JAXP 1.5 into 
JDK8.  JAXP 1.5 adds a new feature to control connections.


Here's the webrev:
http://cr.openjdk.java.net/~joehw/jdk8/8011653/webrev/

Thanks,
Joe
I've done a first pass over the spec/javadoc changes (not the 
implementation yet).


Thanks.


Are you planning to add @since to each of the new constants in 
XMLConstants?


Added.


For the new properties then it specifies that a "a runtime exception" 
will be thrown. Can this be more specific?


They can't be in XMLConstants, but they are in the specific Factories. 
The properties may be supported by factories that may throw different 
exceptions.




The URI scheme is specified in terms of the obsolete RFC 2396 whereas 
I think it just needs to be a String that is equal (ignoring case) to 
the protocol of the connection that is attempting.


The reference to RFC 2396 is replaced with a detailed description.


For jaxp.properties then it reads "and property XXX is specified". I'd 
probably change this to "the property".


Ok.



For the existing FEATURE_SECURE_PROCESSING then "accordance with the 
letter" is a bit unusual, I would be "the letter" could be dropped.


Fixed.


For each of the factories then it specifies that all implementation 
are required to support the new property but this would seem to 
invalidate all existing provider implementations. I don't think 
providers are versioned so all I can suggest is that this be worded to 
make it clear that all implementations that implement JAXP 1.5 or 
newer support this property.


Ok.


A small comment is that there seems to have been previous attempts to 
keep some of the files at 80 or thereabouts columns. The new javadoc 
requires a bit of horizontal scrolling.


Corrected.

Webrevs updated:
http://cr.openjdk.java.net/~joehw/jdk8/8011653/webrev/

Thanks,
Joe



-Alan.

Re: RFR: 8004518 & 8010122 : Default methods on Map

2013-04-15 Thread Peter Levart


Hi Mike,

Another thing to note: Some new methods in HashMap need to call 
inflateTable(), since patch for 8011200 has already been pushed to tl...


Regards, Peter

On 04/14/2013 08:35 PM, Peter Levart wrote:


On 04/14/2013 07:54 PM, Peter Levart wrote:

Hi Mike,

Just a nit: The order of boolean sub-expressions in Map.replace(key, 
oldValue, newValue):


  740 if (!containsKey(key) || !Objects.equals(get(key), oldValue))

...would be more optimal if reversed (like in Map.remove(key, value)).

Regards, Peter


I think the most optimal is actually this:

default boolean remove((Object key, Object value) {
Object curValue = get(key);
if (curValue == null && (value != null || !containsKey(key)) ||
curValue != value && !curValue.equals(value)) {
return false;
}
remove(key);
return true;
}

...and the same for replace(key, oldValue, newValue)...

Regards, Peter



On 04/13/2013 12:02 AM, Mike Duigou wrote:

I have updated the webrev with these changes and a few more.

merge() required an update to it's specification to correctly account for null 
values.

http://cr.openjdk.java.net/~mduigou/JDK-8010122/5/webrev/

This version is currently undergoing final pre-integration testing. Unless 
additional problems are found or the testing fails this version will be 
integrated.

Mike

On Apr 12 2013, at 12:53 , Mike Duigou wrote:


Thanks for the corrections. I have incorporated all of these into the 
integration version of the patch.

On Apr 12 2013, at 12:50 , Akhil Arora wrote:


Hi Mike,

a few small things -

UnmodifiableMap.forEach
is missing Objects.requireNonNull(action);

Added.


EmptyMap.replaceAll(BiFunction)
should just return instead of throwing UnsupportedOpEx
particularly since EmptyList.replaceAll also returns silently
after checking if function is null to fulfil the NPE contract

I agree.


Hashtable
is missing a synchronized override of forEach

added.


CheckedMap.typeCheck(BiFunction)
is missing from the patch (won't compile without it)

Apparently I forgot a qrefresh before generating the webrev. I had this in my 
local copy as it's necessary.


On 04/11/2013 01:03 PM, Mike Duigou wrote:

Another revision incorporating primarily documentation feedback.

http://cr.openjdk.java.net/~mduigou/JDK-8010122/2/webrev/

I've also included the java.util.Collections overrides for the default methods. 
All of these are performance enhancements--the semantics were already correct 
because the defaults use only public methods.

This is likely, modulo formatting of the source examples, the version that will 
be pushed to TL on Friday unless somebody squawks.

Moving the current compute, computeIfPresent, computeIfAbsent and merge 
defaults to ConcurrentMap and replacing the current Map defaults with versions 
that throw ConcurrentModificationException will likely be resolved in a future 
issue if the EG determines it to be important.

Mike


On Apr 10 2013, at 22:42 , Mike Duigou wrote:


I've posted an updated webrev with the review comments I have received.

http://cr.openjdk.java.net/~mduigou/JDK-8010122/1/webrev/

One important point to consider:

- The current implementations of compute, computeIfPresent, computeIfAbsent, 
merge are implemented so that they can work correctly for ConcurrentMap 
instances. For non-concurrent implementations it might be better to fail and 
throw ConcurrentModification exception whenever concurrent modification is 
detected. For regular Map implementations the retry behaviour will only serve 
to mask errors.

Thoughts?

Mike

On Apr 8 2013, at 11:07 , Mike Duigou wrote:


Hello all;

This is a combined review for the new default methods on the java.util.Map 
interface being added for the JSR-335 lambda libraries. The reviews are being 
combined because they share a common unit test.

http://cr.openjdk.java.net/~mduigou/JDK-8010122/0/webrev/

8004518: Add in-place operations to Map
forEach()
replaceAll()

8010122: Add atomic operations to Map
getOrDefault()
putIfAbsent()  *
remove(K, V)
replace(K, V)
replace(K, V, V)
compute()  *
merge()*
computeIfAbsent()  *
computeIfPresent() *

The * operations treat null values as being absent. (ie. the same as there 
being no mapping for the specified key).

The default implementations provided in Map are overridden in HashMap for 
performance purposes, in Hashtable for atomicity and performance purposes and 
in Collections for atomicity.

Mike

42 matches

Mail list logo