date:20121211


On 12/11/2012 04:41 AM, David M. Lloyd wrote:
On JDK 8 with your patches, we are loading around 4750 classes and 
there are, as expected, 0 define races (I believe, however, that we're 
getting a false count though whenever defineClass() returns an 
already-defined class - it would be nice if there were *some* way to 
detect that this happened). 


Hi David,

You could, of course this would have a minor impact on the whole thing, 
in your custom ClassLoader, have an instance of:


final ConcurrentMap, Boolean> foundClasses = new 
ConcurrentHashMap<>();


@Override
protected Class findClass(String name) throws ClassNotFoundException {
  ...
  ...
  if (foundClasses.putIfAbsent(clazz, Boolean.TRUE) != null)
  races++;
  return clazz;
}


Regards, Peter

Re: Proposal: Fully Concurrent ClassLoading


On 12/11/2012 03:55 AM, David Holmes wrote:
Question on the source code: registerAsFullyConcurrent has confusing 
comment -
do the super classes all need to be parallel capable? Or do the super 
classes all need

to be FullyConcurrent? I assume the latter, so just fix the comments.


Actually it is the former. There's no reason to require that all 
superclasses be fully-concurrent. Of course a given loaders degree of 
concurrency may be constrained by what it's supertype allows, but 
there's no reason to actually force all the supertypes to be 
fully-concurrent: it is enough that they are at least all parallel 
capable. 


Hi David,

There is one caveat: if ClassLoader X declares that it is 
fully-concurrent and it's superclass Y is only parallel-capable, then X 
will act as fully-concurrent (returning null from 
getClassLoadingLock()). superclass Y might or might not be coded to use 
the getClassLoadingLock(). X therefore has to know how Y is coded. To be 
defensive, X could ask for Y's registration and declare itself as only 
parallel-capable if Y declares the same so that when Y is upgraded to be 
fully-concurrent, X would become fully-concurrent automatically. To 
support situations where the same version of X would work in two 
environments where in one Y is only parallel-capable and in the other Y 
is fully-concurrent, there could be a static API to retrieve the 
registrations of superclasses.


Or, to have less impact on future deprecation of old parallel-capable 
registration API, the fully-concurrent registration API:


protected static boolean registerAsFullyConcurrent()

might take a boolean parameter:

protected static boolean registerAsFullyConcurrent(boolean 
downgradeToPrallelCapableIfAnySuperclassIsNotFullyConcurrent)


and provide no accessible API to find out what the registration actually 
did (register as parallel-capable or fully-concurrent - return true in 
any case).


Since all JDK provided ClassLoaders will be made fully concurrent, this 
might only be relevant if there is vendor A that currently provides only 
parallel-capable ClassLoader implementation and there is vendor B that 
subclasses A's loader and wants to upgrade and be backward compatible at 
the same time.


Does this complicate things to much for no real benefit?

Regards, Peter

Re: Proposal: Fully Concurrent ClassLoading


On 11/12/2012 7:20 PM, Peter Levart wrote:

On 12/11/2012 03:55 AM, David Holmes wrote:

Question on the source code: registerAsFullyConcurrent has confusing
comment -
do the super classes all need to be parallel capable? Or do the super
classes all need
to be FullyConcurrent? I assume the latter, so just fix the comments.


Actually it is the former. There's no reason to require that all
superclasses be fully-concurrent. Of course a given loaders degree of
concurrency may be constrained by what it's supertype allows, but
there's no reason to actually force all the supertypes to be
fully-concurrent: it is enough that they are at least all parallel
capable.


Hi David,

There is one caveat: if ClassLoader X declares that it is
fully-concurrent and it's superclass Y is only parallel-capable, then X
will act as fully-concurrent (returning null from
getClassLoadingLock()). superclass Y might or might not be coded to use
the getClassLoadingLock(). X therefore has to know how Y is coded. To be
defensive, X could ask for Y's registration and declare itself as only
parallel-capable if Y declares the same so that when Y is upgraded to be
fully-concurrent, X would become fully-concurrent automatically. To
support situations where the same version of X would work in two
environments where in one Y is only parallel-capable and in the other Y
is fully-concurrent, there could be a static API to retrieve the
registrations of superclasses.


I don't quite follow this. What code in the superclass are you 
anticipating that the subclass will use which relies on the lock? Or is 
this just an abstract "what if" scenario?


Thanks,
David
-


Or, to have less impact on future deprecation of old parallel-capable
registration API, the fully-concurrent registration API:

protected static boolean registerAsFullyConcurrent()

might take a boolean parameter:

protected static boolean registerAsFullyConcurrent(boolean
downgradeToPrallelCapableIfAnySuperclassIsNotFullyConcurrent)


and provide no accessible API to find out what the registration actually
did (register as parallel-capable or fully-concurrent - return true in
any case).

Since all JDK provided ClassLoaders will be made fully concurrent, this
might only be relevant if there is vendor A that currently provides only
parallel-capable ClassLoader implementation and there is vendor B that
subclasses A's loader and wants to upgrade and be backward compatible at
the same time.

Does this complicate things to much for no real benefit?

Regards, Peter

Re: Proposal: Fully Concurrent ClassLoading


Hi again,

There might be a middle-ground variant. No registration API changes as 
described below. When ClassLoader X declares that it is fully-concurrent 
make it fully-concurrent. But if any superclass registered as only 
parallel-capable, provide getClassLoadingLock() locks nevertheless. Only 
if all the superclasses declare that they are fully-concurrent, make no 
lock map and make getClassLoadingLock() return null.


Regards, Peter

On 12/11/2012 10:20 AM, Peter Levart wrote:

On 12/11/2012 03:55 AM, David Holmes wrote:
Question on the source code: registerAsFullyConcurrent has confusing 
comment -
do the super classes all need to be parallel capable? Or do the 
super classes all need

to be FullyConcurrent? I assume the latter, so just fix the comments.


Actually it is the former. There's no reason to require that all 
superclasses be fully-concurrent. Of course a given loaders degree of 
concurrency may be constrained by what it's supertype allows, but 
there's no reason to actually force all the supertypes to be 
fully-concurrent: it is enough that they are at least all parallel 
capable. 


Hi David,

There is one caveat: if ClassLoader X declares that it is 
fully-concurrent and it's superclass Y is only parallel-capable, then 
X will act as fully-concurrent (returning null from 
getClassLoadingLock()). superclass Y might or might not be coded to 
use the getClassLoadingLock(). X therefore has to know how Y is coded. 
To be defensive, X could ask for Y's registration and declare itself 
as only parallel-capable if Y declares the same so that when Y is 
upgraded to be fully-concurrent, X would become fully-concurrent 
automatically. To support situations where the same version of X would 
work in two environments where in one Y is only parallel-capable and 
in the other Y is fully-concurrent, there could be a static API to 
retrieve the registrations of superclasses.


Or, to have less impact on future deprecation of old parallel-capable 
registration API, the fully-concurrent registration API:


protected static boolean registerAsFullyConcurrent()

might take a boolean parameter:

protected static boolean registerAsFullyConcurrent(boolean 
downgradeToPrallelCapableIfAnySuperclassIsNotFullyConcurrent)



and provide no accessible API to find out what the registration 
actually did (register as parallel-capable or fully-concurrent - 
return true in any case).


Since all JDK provided ClassLoaders will be made fully concurrent, 
this might only be relevant if there is vendor A that currently 
provides only parallel-capable ClassLoader implementation and there is 
vendor B that subclasses A's loader and wants to upgrade and be 
backward compatible at the same time.


Does this complicate things to much for no real benefit?

Regards, Peter

Re: Proposal: Fully Concurrent ClassLoading


On 12/11/2012 10:29 AM, David Holmes wrote:

On 11/12/2012 7:20 PM, Peter Levart wrote:

On 12/11/2012 03:55 AM, David Holmes wrote:

Question on the source code: registerAsFullyConcurrent has confusing
comment -
do the super classes all need to be parallel capable? Or do the super
classes all need
to be FullyConcurrent? I assume the latter, so just fix the comments.


Actually it is the former. There's no reason to require that all
superclasses be fully-concurrent. Of course a given loaders degree of
concurrency may be constrained by what it's supertype allows, but
there's no reason to actually force all the supertypes to be
fully-concurrent: it is enough that they are at least all parallel
capable.


Hi David,

There is one caveat: if ClassLoader X declares that it is
fully-concurrent and it's superclass Y is only parallel-capable, then X
will act as fully-concurrent (returning null from
getClassLoadingLock()). superclass Y might or might not be coded to use
the getClassLoadingLock(). X therefore has to know how Y is coded. To be
defensive, X could ask for Y's registration and declare itself as only
parallel-capable if Y declares the same so that when Y is upgraded to be
fully-concurrent, X would become fully-concurrent automatically. To
support situations where the same version of X would work in two
environments where in one Y is only parallel-capable and in the other Y
is fully-concurrent, there could be a static API to retrieve the
registrations of superclasses.


I don't quite follow this. What code in the superclass are you 
anticipating that the subclass will use which relies on the lock? Or 
is this just an abstract "what if" scenario?


This is more or less "what if". There might be a subclass Y of say 
java.lang.ClassLoader that overrides loadClass or findClass, declares 
that it is parallel-capable and in the implementation of it's loadClass 
or findClass, uses getClassLoadingLock() to synchronize access to it's 
internal state. Now there comes class X extends Y that declares that it 
is fully-concurrent. Of course this will not work, X has to declare that 
it is parallel-capable, because Y uses getClassLoadingLock().


What I suggested in the next message is to not change the registration 
API but rather provide getClassLoadingLock() that returns non-null locks 
when any of the superclasses declare that they are only 
parallel-capable, not fully-concurrent.


Regards, Peter



Thanks,
David
-


Or, to have less impact on future deprecation of old parallel-capable
registration API, the fully-concurrent registration API:

protected static boolean registerAsFullyConcurrent()

might take a boolean parameter:

protected static boolean registerAsFullyConcurrent(boolean
downgradeToPrallelCapableIfAnySuperclassIsNotFullyConcurrent)


and provide no accessible API to find out what the registration actually
did (register as parallel-capable or fully-concurrent - return true in
any case).

Since all JDK provided ClassLoaders will be made fully concurrent, this
might only be relevant if there is vendor A that currently provides only
parallel-capable ClassLoader implementation and there is vendor B that
subclasses A's loader and wants to upgrade and be backward compatible at
the same time.

Does this complicate things to much for no real benefit?

Regards, Peter

Re: Proposal: Fully Concurrent ClassLoading


Peter,

You are convincing me that all superclasses must be fully concurrent 
too. Otherwise we are just trying to second-guess a whole bunch of 
what-ifs. :)


Thanks,
David

On 11/12/2012 7:44 PM, Peter Levart wrote:

On 12/11/2012 10:29 AM, David Holmes wrote:

On 11/12/2012 7:20 PM, Peter Levart wrote:

On 12/11/2012 03:55 AM, David Holmes wrote:

Question on the source code: registerAsFullyConcurrent has confusing
comment -
do the super classes all need to be parallel capable? Or do the super
classes all need
to be FullyConcurrent? I assume the latter, so just fix the comments.


Actually it is the former. There's no reason to require that all
superclasses be fully-concurrent. Of course a given loaders degree of
concurrency may be constrained by what it's supertype allows, but
there's no reason to actually force all the supertypes to be
fully-concurrent: it is enough that they are at least all parallel
capable.


Hi David,

There is one caveat: if ClassLoader X declares that it is
fully-concurrent and it's superclass Y is only parallel-capable, then X
will act as fully-concurrent (returning null from
getClassLoadingLock()). superclass Y might or might not be coded to use
the getClassLoadingLock(). X therefore has to know how Y is coded. To be
defensive, X could ask for Y's registration and declare itself as only
parallel-capable if Y declares the same so that when Y is upgraded to be
fully-concurrent, X would become fully-concurrent automatically. To
support situations where the same version of X would work in two
environments where in one Y is only parallel-capable and in the other Y
is fully-concurrent, there could be a static API to retrieve the
registrations of superclasses.


I don't quite follow this. What code in the superclass are you
anticipating that the subclass will use which relies on the lock? Or
is this just an abstract "what if" scenario?


This is more or less "what if". There might be a subclass Y of say
java.lang.ClassLoader that overrides loadClass or findClass, declares
that it is parallel-capable and in the implementation of it's loadClass
or findClass, uses getClassLoadingLock() to synchronize access to it's
internal state. Now there comes class X extends Y that declares that it
is fully-concurrent. Of course this will not work, X has to declare that
it is parallel-capable, because Y uses getClassLoadingLock().

What I suggested in the next message is to not change the registration
API but rather provide getClassLoadingLock() that returns non-null locks
when any of the superclasses declare that they are only
parallel-capable, not fully-concurrent.

Regards, Peter



Thanks,
David
-


Or, to have less impact on future deprecation of old parallel-capable
registration API, the fully-concurrent registration API:

protected static boolean registerAsFullyConcurrent()

might take a boolean parameter:

protected static boolean registerAsFullyConcurrent(boolean
downgradeToPrallelCapableIfAnySuperclassIsNotFullyConcurrent)


and provide no accessible API to find out what the registration actually
did (register as parallel-capable or fully-concurrent - return true in
any case).

Since all JDK provided ClassLoaders will be made fully concurrent, this
might only be relevant if there is vendor A that currently provides only
parallel-capable ClassLoader implementation and there is vendor B that
subclasses A's loader and wants to upgrade and be backward compatible at
the same time.

Does this complicate things to much for no real benefit?

Regards, Peter

Re: RFR: 8001647: In-place methods on Collection/List


On 11/12/2012 7:59 AM, Mike Duigou wrote:


On Dec 10 2012, at 05:28 , Alan Bateman wrote:


On 08/12/2012 01:42, Akhil Arora wrote:

As part of the Library Lambdafication, this patch adds the following
default methods to Collections -


This may be bikeshed territory but we usually don't use the "public" modifier 
on methods defined by interfaces as they are public anyway.


Adding "public" was at my suggestion.


It seems inconsistent to me to have it on the default methods. Perhaps this has 
been discussed before, in which case ignore this. BTW: The only reason I'm 
bringing this up is because there are lots of default methods to come and it 
would be nice to establish a convention and consistency from the start.


Agreed that we should be consistent. The suggestion to add "public" isn't related specifically to the default 
but anticipates, perhaps prematurely, future addition of "package" "module" and 
"protected" modifiers. When those modifiers are added it will make sense to be explicit about the access of a 
method. Additionally, some have complained about the difference in default access between interfaces and classes 
(public vs package) and prefer to explicit so that the intent is obvious and that method signature text in interfaces 
and classes look the same.

So, worthwhile or not?


I think not. No matter what modifiers come in the future the default 
will always have to remain "public".


But if you are going to add it then add it to all the methods in the 
interfaces being updated.


David
-


Mike

Re: Proposal: Fully Concurrent ClassLoading


On 12/11/2012 12:27 PM, David Holmes wrote:

Peter,

You are convincing me that all superclasses must be fully concurrent 
too. Otherwise we are just trying to second-guess a whole bunch of 
what-ifs. :)


If you think some more, yes. The superclass might not use 
getClassLoadingLock() but rely on the fact that findClass() is allways 
called under a guard of per-class-name lock, for example. It's a matter 
of how far to go to prevent such miss-behaving fully-concurrent 
subclasses. So far to also prevent fully-concurrent subclasses that 
would otherwise be perfectly correct?


Maybe not. Creating custom ClassLoaders is not an average programmer's 
job. Those that do this things will of course study the implementations 
of superclasses they extend and do the right thing. And it's reasonable 
to expect that they more or less will only extend JDK's ClassLoaders - 
but on the other hand if they only extend JDK's class loaders, they are 
not prevented to be fully-concurrent either way. Hm...


Peter



Thanks,
David

On 11/12/2012 7:44 PM, Peter Levart wrote:

On 12/11/2012 10:29 AM, David Holmes wrote:

On 11/12/2012 7:20 PM, Peter Levart wrote:

On 12/11/2012 03:55 AM, David Holmes wrote:

Question on the source code: registerAsFullyConcurrent has confusing
comment -
do the super classes all need to be parallel capable? Or do the 
super

classes all need
to be FullyConcurrent? I assume the latter, so just fix the 
comments.


Actually it is the former. There's no reason to require that all
superclasses be fully-concurrent. Of course a given loaders degree of
concurrency may be constrained by what it's supertype allows, but
there's no reason to actually force all the supertypes to be
fully-concurrent: it is enough that they are at least all parallel
capable.


Hi David,

There is one caveat: if ClassLoader X declares that it is
fully-concurrent and it's superclass Y is only parallel-capable, 
then X

will act as fully-concurrent (returning null from
getClassLoadingLock()). superclass Y might or might not be coded to 
use
the getClassLoadingLock(). X therefore has to know how Y is coded. 
To be

defensive, X could ask for Y's registration and declare itself as only
parallel-capable if Y declares the same so that when Y is upgraded 
to be

fully-concurrent, X would become fully-concurrent automatically. To
support situations where the same version of X would work in two
environments where in one Y is only parallel-capable and in the 
other Y

is fully-concurrent, there could be a static API to retrieve the
registrations of superclasses.


I don't quite follow this. What code in the superclass are you
anticipating that the subclass will use which relies on the lock? Or
is this just an abstract "what if" scenario?


This is more or less "what if". There might be a subclass Y of say
java.lang.ClassLoader that overrides loadClass or findClass, declares
that it is parallel-capable and in the implementation of it's loadClass
or findClass, uses getClassLoadingLock() to synchronize access to it's
internal state. Now there comes class X extends Y that declares that it
is fully-concurrent. Of course this will not work, X has to declare that
it is parallel-capable, because Y uses getClassLoadingLock().

What I suggested in the next message is to not change the registration
API but rather provide getClassLoadingLock() that returns non-null locks
when any of the superclasses declare that they are only
parallel-capable, not fully-concurrent.

Regards, Peter



Thanks,
David
-


Or, to have less impact on future deprecation of old parallel-capable
registration API, the fully-concurrent registration API:

protected static boolean registerAsFullyConcurrent()

might take a boolean parameter:

protected static boolean registerAsFullyConcurrent(boolean
downgradeToPrallelCapableIfAnySuperclassIsNotFullyConcurrent)


and provide no accessible API to find out what the registration 
actually

did (register as parallel-capable or fully-concurrent - return true in
any case).

Since all JDK provided ClassLoaders will be made fully concurrent, 
this
might only be relevant if there is vendor A that currently provides 
only

parallel-capable ClassLoader implementation and there is vendor B that
subclasses A's loader and wants to upgrade and be backward 
compatible at

the same time.

Does this complicate things to much for no real benefit?

Regards, Peter

Re: 8004371: (props) Properties.loadFromXML needs small footprint XML parser as fallback when JAXP is not present



Joe sent me an update with changes to address issues discussed so far, 
I've put the webrev here:


http://cr.openjdk.java.net/~alanb/8004371/webrev.02/

Joe - the classes you sent me were in different packages, also the 
formatting was a bit messed up in several classes, I've fixed up those 
issues so you may need to re-base from the patch file included in the 
webrev.


-Alan.

Re: RFR: 8001647: In-place methods on Collection/List


On 10/12/2012 21:59, Mike Duigou wrote:

:

Adding "public" was at my suggestion.


It seems inconsistent to me to have it on the default methods. Perhaps this has 
been discussed before, in which case ignore this. BTW: The only reason I'm 
bringing this up is because there are lots of default methods to come and it 
would be nice to establish a convention and consistency from the start.

Agreed that we should be consistent. The suggestion to add "public" isn't related specifically to the default 
but anticipates, perhaps prematurely, future addition of "package" "module" and 
"protected" modifiers. When those modifiers are added it will make sense to be explicit about the access of a 
method. Additionally, some have complained about the difference in default access between interfaces and classes 
(public vs package) and prefer to explicit so that the intent is obvious and that method signature text in interfaces 
and classes look the same.

So, worthwhile or not?
It's probably not worth spending time on now but if we going to be 
explicit with new methods then existing methods should be updated too.


-Alan.

Re: CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)


On 10/12/2012 17:12, Daniel Fuchs wrote:

Hi,

After further discussion with Joe & Alan, I changed the call
to ServiceLoader to simply return the first provider it finds,
or null.

This is closer to what was present in JDK 7 - and looping is
not necessary in JDK 8 since the default implementation is
not returned as a service provider.

So here is a new - and hopefully simpler webrev:

 



best regards,

-- daniel

This looks fine to me, just a stray  at L64.

-Alan

Re: CFR: javax.xml.parsers: Using ServiceLoader to load JAXP parser factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-11 Thread Daniel Fuchs


On 12/11/12 2:01 PM, Alan Bateman wrote:

On 10/12/2012 17:12, Daniel Fuchs wrote:

[...]

So here is a new - and hopefully simpler webrev:




best regards,

-- daniel

This looks fine to me, just a stray  at L64.


Right - I originally did it on purpose because the doc was
looking weird without it. But now I regenerated the doc and
I don't think it looks better with it - so I will remove
the stray . I'll just refresh webrev.04 after that
(I assume there's no need for a webrev.05 - but I'd like
to keep a public copy of the 'final' patch).

-- daniel



-Alan

Review request 8004357: Implement various methods in SerialBlob/Clob/Array and specify Thread Safety

2012-12-11 Thread Lance Andersen - Oracle


Need a reviewer for 8004357:Implement various methods in 
SerialBlob/Clob/Array and specify Thread Safety

This defines thread safety adds missing methods to SerialBlob/Clob/Array


The CCC request has been reviewed.  The changes uncovered a couple of bugs in 
the JCK which the JCK team is going to address in the JCK and RowSet TCK.  JDBC 
Unit tests and SQE tests all continue to pass.

http://cr.openjdk.java.net/~lancea/8004357/webrev.00/

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: Proposal: Fully Concurrent ClassLoading

2012-12-11 Thread David M. Lloyd


No problem.  What do you mean by "instrumentation"?

On 12/10/2012 11:18 PM, David Holmes wrote:

David,

Many thanks for trialling this so promptly!

Do you have any suggestions for what instrumentation you would like to
see accompany this?

David

On 11/12/2012 1:41 PM, David M. Lloyd wrote:

On 12/10/2012 06:36 PM, David Holmes wrote:

On 10/12/2012 11:53 PM, David M. Lloyd wrote:

On 12/09/2012 10:03 PM, David Holmes wrote:

That sounds promising. Are you in a position to try out this
proposal on
a testbed with your app server?


Sure, I'd love to. What tree should I build?


Please apply the patches from the webrevs here:

http://cr.openjdk.java.net/~dholmes/concurrent-loaders/webrev.hotspot/
http://cr.openjdk.java.net/~dholmes/concurrent-loaders/webrev.jdk/

They should apply to a jdk8 or tl forest. (And I hope I didn't mess
something up generating the webrev ;-) )


Well, I've just gotten everything working and done some cursory testing
on a startup of an "empty" JBoss AS 7 instance, and then reviewing the
metrics reported by our class loader. Our timing metrics are not really
great for general-purpose benchmarking (for various uninteresting
reasons), but I can at least get enough information to know everything
is working more or less as expected:

On JDK 6 with our "unsafe" lockless modification, we're typically
loading 4707 classes, and I'm seeing anywhere from 0 to 5 define races
that were automatically resolved.

On JDK 7 using the standard registerAsParallelCapable() mechanism, we
are loading 4711 classes and I'm seeing 35-50 define races that were
automatically resolved - the overhead of locking starts to come to the
fore I think.

On JDK 8 with your patches, we are loading around 4750 classes and there
are, as expected, 0 define races (I believe, however, that we're getting
a false count though whenever defineClass() returns an already-defined
class - it would be nice if there were *some* way to detect that this
happened).

On our class loader implementations, I'm initializing this way:

static {
try {
ClassLoader.registerAsFullyConcurrent();
} catch (Throwable ignored) {
try {
ClassLoader.registerAsParallelCapable();
} catch (Throwable ignored2) {
}
}
}

The debugging message confirms that our class loaders are successfully
registered as fully concurrent, and the fact that it doesn't hang or
crash on JDK 7 indicates that they are still properly being registered
as parallel-capable on that platform.




--
- DML

Re: Proposal: Fully Concurrent ClassLoading

2012-12-11 Thread David M. Lloyd


On 12/11/2012 02:12 AM, Peter Levart wrote:

On 12/11/2012 04:41 AM, David M. Lloyd wrote:

On JDK 8 with your patches, we are loading around 4750 classes and
there are, as expected, 0 define races (I believe, however, that we're
getting a false count though whenever defineClass() returns an
already-defined class - it would be nice if there were *some* way to
detect that this happened).


Hi David,

You could, of course this would have a minor impact on the whole thing,
in your custom ClassLoader, have an instance of:

final ConcurrentMap, Boolean> foundClasses = new
ConcurrentHashMap<>();

@Override
protected Class findClass(String name) throws ClassNotFoundException {
   ...
   ...
   if (foundClasses.putIfAbsent(clazz, Boolean.TRUE) != null)
   races++;
   return clazz;
}


Sure, but having this map puts me back in the same situation I was in 
before where we have another concurrent map with a key per class.


--
- DML

RFR: javax.xml.datatype: Using ServiceLoader to load JAXP datatype factories (7169894: JAXP Plugability Layer: using service loader)

2012-12-11 Thread Daniel Fuchs


Hi,

Here is a new webrev in the series that addresses using ServiceLoader in
JAXP for JDK 8.

7169894: JAXP Plugability Layer: using service loader

This changeset addresses modification in the javax.xml.datatype
package.
It is similar to changes proposed for the javax.xml.parsers
package [1], with a few differences due to the specificities of
javax.xml.datatype.

Namely:

1. The documentation that describes the loading mechanism is in the
   class header rather than in the method documentation - which leads
   to some wording changes.

2. The DatatypeFactory is specified to throw a
   DatatypeConfigurationException - which is a checked exception,
   instead of an Error - as was FactoryConfigurationError

3. DatatypeConfigurationException allows to wrap
   ServiceConfigurationError directly - so the additional layer
   of RuntimeException is not needed here.



-- daniel

[1] javax.xml.parsers:

RFR: Trivial patch for javadoc warnings in Base64

2012-12-11 Thread Chris Hegarty



hg diff Base64.java
diff -r d206e52bf8a6 src/share/classes/java/util/Base64.java
--- a/src/share/classes/java/util/Base64.java   Tue Dec 11 13:14:56 2012 
+0800
+++ b/src/share/classes/java/util/Base64.java   Tue Dec 11 18:05:30 2012 
+

@@ -289,8 +289,8 @@ public class Base64 {
  *
  *  This method first encodes all input bytes into a base64 
encoded
  * byte array and then constructs a new String by using the 
encoded byte
- * array and the {@link 
java.nio.charset.StandardCharsets.ISO_8859_1 ISO-8859-1}

- * charset.
+ * array and the {@link 
java.nio.charset.StandardCharsets#ISO_8859_1

+ * ISO-8859-1} charset.
  *
  *  In other words, an invocation of this method has 
exactly the same

  * effect as invoking
@@ -358,9 +358,9 @@ public class Base64 {
  * to encode any more input bytes. The encoding operation can be
  * continued, if there is more bytes in input buffer to be 
encoded,
  * by invoking this method again with an output buffer that 
has more

- * {@linkplain Buffer#remaining remaining} bytes. This is typically
- * done by draining any encoded bytes from the output buffer. The
- * value returned from last invocation needs to be passed in as the
+ * {@linkplain java.nio.Buffer#remaining remaining} bytes. This is
+ * typically done by draining any encoded bytes from the output 
buffer.
+ * The value returned from last invocation needs to be passed 
in as the
  * third parameter {@code bytesOut} if it is to continue an 
unfinished

  * encoding, 0 otherwise.
  *
@@ -806,9 +806,9 @@ public class Base64 {
  * buffer has insufficient space to decode any more input bytes.
  * The decoding operation can be continued, if there is more bytes
  * in input buffer to be decoded, by invoking this method 
again with
- * an output buffer that has more {@linkplain Buffer#remaining 
remaining}
- * bytes.This is typically done by draining any decoded bytes 
from the

- * output buffer.
+ * an output buffer that has more {@linkplain 
java.nio.Buffer#remaining

+ * remaining} bytes. This is typically done by draining any decoded
+ * bytes from the output buffer.
  *
  * Recommended Usage Example
  * 


-Chris.

Re: RFR: Trivial patch for javadoc warnings in Base64

2012-12-11 Thread Joe Darcy


Looks good Chris -- approved!

Cheers,

-Joe

On 12/11/2012 10:10 AM, Chris Hegarty wrote:


hg diff Base64.java
diff -r d206e52bf8a6 src/share/classes/java/util/Base64.java
--- a/src/share/classes/java/util/Base64.java   Tue Dec 11 13:14:56 
2012 +0800
+++ b/src/share/classes/java/util/Base64.java   Tue Dec 11 18:05:30 
2012 +

@@ -289,8 +289,8 @@ public class Base64 {
  *
  *  This method first encodes all input bytes into a 
base64 encoded
  * byte array and then constructs a new String by using the 
encoded byte
- * array and the {@link 
java.nio.charset.StandardCharsets.ISO_8859_1 ISO-8859-1}

- * charset.
+ * array and the {@link 
java.nio.charset.StandardCharsets#ISO_8859_1

+ * ISO-8859-1} charset.
  *
  *  In other words, an invocation of this method has 
exactly the same

  * effect as invoking
@@ -358,9 +358,9 @@ public class Base64 {
  * to encode any more input bytes. The encoding operation can be
  * continued, if there is more bytes in input buffer to be 
encoded,
  * by invoking this method again with an output buffer that 
has more
- * {@linkplain Buffer#remaining remaining} bytes. This is 
typically
- * done by draining any encoded bytes from the output buffer. 
The
- * value returned from last invocation needs to be passed in 
as the
+ * {@linkplain java.nio.Buffer#remaining remaining} bytes. 
This is
+ * typically done by draining any encoded bytes from the 
output buffer.
+ * The value returned from last invocation needs to be passed 
in as the
  * third parameter {@code bytesOut} if it is to continue an 
unfinished

  * encoding, 0 otherwise.
  *
@@ -806,9 +806,9 @@ public class Base64 {
  * buffer has insufficient space to decode any more input bytes.
  * The decoding operation can be continued, if there is more 
bytes
  * in input buffer to be decoded, by invoking this method 
again with
- * an output buffer that has more {@linkplain 
Buffer#remaining remaining}
- * bytes.This is typically done by draining any decoded bytes 
from the

- * output buffer.
+ * an output buffer that has more {@linkplain 
java.nio.Buffer#remaining
+ * remaining} bytes. This is typically done by draining any 
decoded

+ * bytes from the output buffer.
  *
  * Recommended Usage Example
  * 


-Chris.

Re: Review Request: 8004201: add reducers to primitive type wrappers

2012-12-11 Thread Akhil Arora


http://cr.openjdk.java.net/~akhil/8004201.3/webrev/

- removed these operators on Byte and Short
- javadoc improvements based on CCC review

On 12/10/2012 03:45 PM, Akhil Arora wrote:

http://cr.openjdk.java.net/~akhil/8004201.2/webrev/

- replaced "Suitable for conversion as a method reference to functional
interfaces such as ..." with @see java.util.function.BinaryOperator

- javadoc - replaced 'a  argument'/'another  argument' with
'the first operand'/'the second operand'

- did not widen return types - widening the return type makes these
methods unusable as reducers, since BinaryOperator is declared
   T operate(T left, T right)

Thanks

On 12/05/2012 03:44 PM, Joseph Darcy wrote:

Akhil,

In javadoc like this

298  * as {@code BinaryOperator}.

you don't have to use "<" and the like inside {@code}; please
change to

298  * as {@code BinaryOperator}.

As a general comment, if the operations for primitive type Foo are put
into java.lang.Foo, then the type of the operation needs to be
explicitly represented in the expression calling the method (unless
static imports are used, etc.).  Therefore, I suggest putting these sort
of static methods all into one class to allow overloading to do its
thing (java.lang.Operators?).  Also, for methods like sum, I think it is
worth considering returning a larger type than the operands to avoid
problems from integer or floating-point overflow. For example, sum on
byte and short would return int, sun on int would return long, etc.

As an aside, to get a numerically robust result, the summation logic
over a set of doubles needs to be more than just a set of raw double
additions, but that can be tackled later.

Cheers,

-Joe


On 12/5/2012 1:27 PM, Akhil Arora wrote:

Updated - http://cr.openjdk.java.net/~akhil/8004201.1/webrev/

- delegate to Math.min/max for int/long/float/double
- rename Boolean.and/or/xor to logicalAnd/logicalOr/logicalXor
- removed Character variants of min/max/sum

On 12/02/2012 05:50 PM, David Holmes wrote:

Hi Akhil,

Is it really necessary/desirable to flag all of these as " Suitable for
conversion as a method reference to functional interfaces such as
..." ?

Not necessary, but it does provide a hint as to their intended use to a
casual browser of these docs.


This style:

+ * @param   a   a boolean argument.
+ * @param   b   another boolean argument.

is at odds with the style used elsewhere for new Functional APIs, and
with the style of other methods in these classes. Can we just use
"first
operand" and "second operand" for all of these?

It is consistent with Math.min/max, which use the a/b style. Since these
methods are not in one of the functional package, is'nt it better to
stick to the local style?


Character.sum does not make sense to me. Who adds together characters?
I'm not even sure min and max are worth supporting for Character.

Good point - removed these methods for Character.


I disagree with other suggestions to use the Math functions for
float/double. I think all these methods should use the underlying
primitive operator regardless of type.

Are you disagreeing only for float/double or for int/long also? Can you
provide more information as to why you disagree?

Thanks


Thanks,
David
-

On 1/12/2012 4:44 AM, Akhil Arora wrote:

Hi

Requesting review for some basic functionality related to lambdas -

Add min, max, sum methods to the primitive wrapper classes - Byte,
Short, Integer, Long, Float, Double and Character so as to be able to
use them as reducers in lambda expressions. Add and, or, xor
methods to
Boolean.

http://cr.openjdk.java.net/~akhil/8004201.0/webrev/
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004201

Thanks

8004874: (profiles) Reduce dependency on java.beans to only add/removePropertyChangeListener



Those tracking the work to get us to a modular JDK will know that the 
java.beans package is highly problematic.


For the "core" modules then  j.u.l.LogManager and j.u.jar.Pack200 have 
support for PropertyChangeListener and that means edges in the module 
graph that are highly undesirable. As I've mentioned in other mails 
here, the plan to address this is to deprecate the 
add/removePropertyChangeListener methods defined by these classes in JDK 
8 and remove them outright in JDK 9.


In the mean-time we have Compact Profiles coming in JDK 8 so we need a 
solution that allows the java.util.logging and java.util.jar packages be 
included in the profiles without java.beans. As we are only talking 
about 6 methods in non-mainstream areas then the proposal is that these 
methods are not present in the subset Profiles of Java SE. This may be a 
surprise but it avoids a long of list of complications that would 
otherwise arise if there are references to types that do not exist. In 
implementation terms they will be removed in the build of the profiles, 
that's a minor detail.


The changes proposed are just minor updates to LogManager and the 
Pack200.Packer and Pack200.Unpacker implementations so that the only 
dependencies on PropertyChangeListener and PropertyChangeEvent are in 
the addPropertyChangeListener and removePropertyChangeListener methods. 
Once they get to the profiles forest then we can put changes in to 
remove these methods (and maybe GC the constant pool too).


One other thing to point out is that Packer and Unpacker are interfaces 
and so removing methods means that implementations that compile against 
the subset will not compile when the add/removePropertyChangeListener 
methods are present.  As it should be very rare to implement Packer and 
Unpacker then it's probably not a big deal but we can use default 
methods to eliminate the concern so that implementations that compile 
against compact1 (for example) will also compile on the full platform.


The webrev with the changes is here. Note that only changes to 
LogManager and pack200's PropMap are proposed to be included for now, 
changing the methods to default methods will come later along with 
javadoc updates, perhaps via the profiles forest. One other thing is 
that the Beans supporting class is duplicated between the two, this is 
mostly to avoid it getting used more widely. It will of course be 
removed once these methods are removed from the full platform, as per 
the original plan.


http://cr.openjdk.java.net/~alanb/8004874/webrev/

Thanks,

Alan.

Re: 8004874: (profiles) Reduce dependency on java.beans to only add/removePropertyChangeListener

2012-12-11 Thread Kumar Srinivasan


Alan,

PropMap.java, Nit: typo PropertyChangeListern's similarly in the other.

Nice test, but since it is specific to pack200 and all pack200 tests
are in jdk/test/tools/pack200,  perhaps move it there ?

Besides that I could not find anything else wrong, the
users of these ProperyChangeListeners are the deployment
technologies, though.

Kumar






Those tracking the work to get us to a modular JDK will know that the 
java.beans package is highly problematic.


For the "core" modules then  j.u.l.LogManager and j.u.jar.Pack200 have 
support for PropertyChangeListener and that means edges in the module 
graph that are highly undesirable. As I've mentioned in other mails 
here, the plan to address this is to deprecate the 
add/removePropertyChangeListener methods defined by these classes in 
JDK 8 and remove them outright in JDK 9.


In the mean-time we have Compact Profiles coming in JDK 8 so we need a 
solution that allows the java.util.logging and java.util.jar packages 
be included in the profiles without java.beans. As we are only talking 
about 6 methods in non-mainstream areas then the proposal is that 
these methods are not present in the subset Profiles of Java SE. This 
may be a surprise but it avoids a long of list of complications that 
would otherwise arise if there are references to types that do not 
exist. In implementation terms they will be removed in the build of 
the profiles, that's a minor detail.


The changes proposed are just minor updates to LogManager and the 
Pack200.Packer and Pack200.Unpacker implementations so that the only 
dependencies on PropertyChangeListener and PropertyChangeEvent are in 
the addPropertyChangeListener and removePropertyChangeListener 
methods. Once they get to the profiles forest then we can put changes 
in to remove these methods (and maybe GC the constant pool too).


One other thing to point out is that Packer and Unpacker are 
interfaces and so removing methods means that implementations that 
compile against the subset will not compile when the 
add/removePropertyChangeListener methods are present.  As it should be 
very rare to implement Packer and Unpacker then it's probably not a 
big deal but we can use default methods to eliminate the concern so 
that implementations that compile against compact1 (for example) will 
also compile on the full platform.


The webrev with the changes is here. Note that only changes to 
LogManager and pack200's PropMap are proposed to be included for now, 
changing the methods to default methods will come later along with 
javadoc updates, perhaps via the profiles forest. One other thing is 
that the Beans supporting class is duplicated between the two, this is 
mostly to avoid it getting used more widely. It will of course be 
removed once these methods are removed from the full platform, as per 
the original plan.


http://cr.openjdk.java.net/~alanb/8004874/webrev/

Thanks,

Alan.

Re: RFR: 8004651 - TEST: java/util/logging/CheckLockLocationTest.java failed to delete file (win)

2012-12-11 Thread Jim Gish

A bit more cleanup as suggested:

http://cr.openjdk.java.net/~jgish/Bug8004651-CheckLockLocationTest-Windows-delete-file-fix/

Thanks,
Jim

On 12/10/2012 07:47 PM, Stuart Marks wrote:

Hi Jim,

Catching IOException from delete() is a bit odd. The only thing in the
delete() method that throws an IOE is the explicit throw of
FileNotFoundException... so in that case we'd throw FNFE and then
catch the IOE at the caller and print a warning. Would it be better to
just print a warning from within the delete() method, and remove
"throws IOException" ? There's only one other caller to delete() and
it seems indifferent to this change.

Now that we're no longer checking the message of FileSystemException,
it's possible to change the instanceof check into a separate
catch-clause of FileSystemException, which simply ignores that
exception. The catch clause for IOException can be simplified to
unconditionally wrap the IOE in a RuntimeException and rethrow it.
Actually it's not clear to me that's even necessary since runTests()
is declared to throw IOException, so we might not even need to catch
IOE here at all; we can just let it propagate to the caller.

Looks like similar simplifications apply to tests 2 and 4 as well.

s'marks

On 12/7/12 11:18 AM, Jim Gish wrote:

Please review
http://cr.openjdk.java.net/~jgish/Bug8004651-CheckLockLocationTest-Windows-delete-file-fix/

Summary -- failure to delete a test log should be a warning instead of a
failure. Also, while fixing this problem another one popped up --
not all

platforms generate the same message in the FileSystemException ("Not a
directory"), so removing the exception content check.

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

hg: jdk8/tl/langtools: 8004828: refactor init of *DocImpl classes

2012-12-11 Thread jonathan . gibbons

Changeset: cfde9737131e
Author:jjg
Date:  2012-12-11 15:05 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/cfde9737131e

8004828: refactor init of *DocImpl classes
Reviewed-by: darcy

! src/share/classes/com/sun/tools/javadoc/AnnotationTypeDocImpl.java
! src/share/classes/com/sun/tools/javadoc/AnnotationTypeElementDocImpl.java
! src/share/classes/com/sun/tools/javadoc/ClassDocImpl.java
! src/share/classes/com/sun/tools/javadoc/ConstructorDocImpl.java
! src/share/classes/com/sun/tools/javadoc/DocEnv.java
! src/share/classes/com/sun/tools/javadoc/DocImpl.java
! src/share/classes/com/sun/tools/javadoc/ExecutableMemberDocImpl.java
! src/share/classes/com/sun/tools/javadoc/FieldDocImpl.java
! src/share/classes/com/sun/tools/javadoc/JavadocEnter.java
! src/share/classes/com/sun/tools/javadoc/JavadocMemberEnter.java
! src/share/classes/com/sun/tools/javadoc/MemberDocImpl.java
! src/share/classes/com/sun/tools/javadoc/MethodDocImpl.java
! src/share/classes/com/sun/tools/javadoc/PackageDocImpl.java
! src/share/classes/com/sun/tools/javadoc/ProgramElementDocImpl.java
! src/share/classes/com/sun/tools/javadoc/RootDocImpl.java

Re: 8004874: (profiles) Reduce dependency on java.beans to only add/removePropertyChangeListener

2012-12-11 Thread Mandy Chung


Alan,

Looks good to me.  Duplicating the Beans supporting class is fine as 
they will be removed when the deprecated addPropertyChangeListener and 
removePropertyChangeListener methods are removed in a future release.


Mandy

On 12/11/12 11:55 AM, Alan Bateman wrote:


Those tracking the work to get us to a modular JDK will know that the 
java.beans package is highly problematic.


For the "core" modules then  j.u.l.LogManager and j.u.jar.Pack200 have 
support for PropertyChangeListener and that means edges in the module 
graph that are highly undesirable. As I've mentioned in other mails 
here, the plan to address this is to deprecate the 
add/removePropertyChangeListener methods defined by these classes in 
JDK 8 and remove them outright in JDK 9.


In the mean-time we have Compact Profiles coming in JDK 8 so we need a 
solution that allows the java.util.logging and java.util.jar packages 
be included in the profiles without java.beans. As we are only talking 
about 6 methods in non-mainstream areas then the proposal is that 
these methods are not present in the subset Profiles of Java SE. This 
may be a surprise but it avoids a long of list of complications that 
would otherwise arise if there are references to types that do not 
exist. In implementation terms they will be removed in the build of 
the profiles, that's a minor detail.


The changes proposed are just minor updates to LogManager and the 
Pack200.Packer and Pack200.Unpacker implementations so that the only 
dependencies on PropertyChangeListener and PropertyChangeEvent are in 
the addPropertyChangeListener and removePropertyChangeListener 
methods. Once they get to the profiles forest then we can put changes 
in to remove these methods (and maybe GC the constant pool too).


One other thing to point out is that Packer and Unpacker are 
interfaces and so removing methods means that implementations that 
compile against the subset will not compile when the 
add/removePropertyChangeListener methods are present.  As it should be 
very rare to implement Packer and Unpacker then it's probably not a 
big deal but we can use default methods to eliminate the concern so 
that implementations that compile against compact1 (for example) will 
also compile on the full platform.


The webrev with the changes is here. Note that only changes to 
LogManager and pack200's PropMap are proposed to be included for now, 
changing the methods to default methods will come later along with 
javadoc updates, perhaps via the profiles forest. One other thing is 
that the Beans supporting class is duplicated between the two, this is 
mostly to avoid it getting used more widely. It will of course be 
removed once these methods are removed from the full platform, as per 
the original plan.


http://cr.openjdk.java.net/~alanb/8004874/webrev/

Thanks,

Alan.

hg: jdk8/tl/jdk: 8003246: Add InitialValue Supplier to ThreadLocal

2012-12-11 Thread mike . duigou

Changeset: c4bd81de2868
Author:akhil
Date:  2012-12-11 15:33 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c4bd81de2868

8003246: Add InitialValue Supplier to ThreadLocal
Reviewed-by: mduigou, forax, dl, chegar, briangoetz

! src/share/classes/java/lang/ThreadLocal.java
+ test/java/lang/ThreadLocal/ThreadLocalSupplierTest.java

RFR: 8004748: clean up @build tags in RMI tests

2012-12-11 Thread Stuart Marks


Hi all,

Please review the following gigantic webrev [1] to clean up @build tags in RMI 
tests. Details underlying this change are in the bug report [2].


Briefly, if test classes listed in @build tags are in the wrong order, this 
trips over a jtreg problem that in turn causes a cascade of subsequent tests to 
fail. It's sensitive to the order in which tests run. The problem currently 
occurs in the jdk7u repo. It doesn't happen in jdk8 right now, but as things 
shift around it might occur in the future.


Naturally, I intend to backport this to 7u once it's in 8.

Shifting the @build tags in the test is a workaround for the jtreg bug, but 
jtreg isn't going to be fixed soon. Besides, the @build tags in the RMI tests 
needed to be cleaned up anyway. In particular, consolidating multiple @build 
tags into a single tag speeds up the RMI test run by about 2.5%.


I've also taken the opportunity to do a couple of related cleanups in a few 
places, such as fixing typos, rearranging tags to be in a more consistent 
order, removing unnecessary classes from @build lines, adding necessary ones, 
and in one case renaming a file that was spelled differently from the class 
that it contained. (CheckUnmarshall.java -> CheckUnmarshal.java; there are no 
textual changes to this file.)


Needless to say, all tests pass. In addition, I've run each test individually 
(i.e., with a clean JTwork directory) to ensure that there were no occurrences 
of the library class ordering issue that triggers the jtreg bug.


Thanks,

s'marks


[1] http://cr.openjdk.java.net/~smarks/reviews/8004748/webrev.0/

[2] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004748

Re: RFR: 8004651 - TEST: java/util/logging/CheckLockLocationTest.java failed to delete file (win)

2012-12-11 Thread Stuart Marks

Looks good!

Do you need someone to push this for you?

s'marks

On 12/11/12 3:04 PM, Jim Gish wrote:

A bit more cleanup as suggested:

http://cr.openjdk.java.net/~jgish/Bug8004651-CheckLockLocationTest-Windows-delete-file-fix/

Thanks,
Jim

On 12/10/2012 07:47 PM, Stuart Marks wrote:

Hi Jim,

Catching IOException from delete() is a bit odd. The only thing in the
delete() method that throws an IOE is the explicit throw of
FileNotFoundException... so in that case we'd throw FNFE and then catch the
IOE at the caller and print a warning. Would it be better to just print a
warning from within the delete() method, and remove "throws IOException" ?
There's only one other caller to delete() and it seems indifferent to this
change.

Now that we're no longer checking the message of FileSystemException, it's
possible to change the instanceof check into a separate catch-clause of
FileSystemException, which simply ignores that exception. The catch clause
for IOException can be simplified to unconditionally wrap the IOE in a
RuntimeException and rethrow it. Actually it's not clear to me that's even
necessary since runTests() is declared to throw IOException, so we might not
even need to catch IOE here at all; we can just let it propagate to the caller.

Looks like similar simplifications apply to tests 2 and 4 as well.

s'marks

On 12/7/12 11:18 AM, Jim Gish wrote:

Please review
http://cr.openjdk.java.net/~jgish/Bug8004651-CheckLockLocationTest-Windows-delete-file-fix/

Summary -- failure to delete a test log should be a warning instead of a
failure. Also, while fixing this problem another one popped up -- not all
platforms generate the same message in the FileSystemException ("Not a
directory"), so removing the exception content check.

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: 8004748: clean up @build tags in RMI tests

2012-12-11 Thread Joe Darcy


Looks fine; approved,

-Joe

On 12/11/2012 03:53 PM, Stuart Marks wrote:

Hi all,

Please review the following gigantic webrev [1] to clean up @build 
tags in RMI tests. Details underlying this change are in the bug 
report [2].


Briefly, if test classes listed in @build tags are in the wrong order, 
this trips over a jtreg problem that in turn causes a cascade of 
subsequent tests to fail. It's sensitive to the order in which tests 
run. The problem currently occurs in the jdk7u repo. It doesn't happen 
in jdk8 right now, but as things shift around it might occur in the 
future.


Naturally, I intend to backport this to 7u once it's in 8.

Shifting the @build tags in the test is a workaround for the jtreg 
bug, but jtreg isn't going to be fixed soon. Besides, the @build tags 
in the RMI tests needed to be cleaned up anyway. In particular, 
consolidating multiple @build tags into a single tag speeds up the RMI 
test run by about 2.5%.


I've also taken the opportunity to do a couple of related cleanups in 
a few places, such as fixing typos, rearranging tags to be in a more 
consistent order, removing unnecessary classes from @build lines, 
adding necessary ones, and in one case renaming a file that was 
spelled differently from the class that it contained. 
(CheckUnmarshall.java -> CheckUnmarshal.java; there are no textual 
changes to this file.)


Needless to say, all tests pass. In addition, I've run each test 
individually (i.e., with a clean JTwork directory) to ensure that 
there were no occurrences of the library class ordering issue that 
triggers the jtreg bug.


Thanks,

s'marks


[1] http://cr.openjdk.java.net/~smarks/reviews/8004748/webrev.0/

[2] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004748

Re: Proposal: Fully Concurrent ClassLoading


On 12/12/2012 1:10 AM, David M. Lloyd wrote:

No problem. What do you mean by "instrumentation"?


Any means of "seeing" when parallel define class actually occurred:

-  -verbose:class logging ?
-  jvmstat counter ?
- -XX:+TraceParallelDefine ?
- ???

Thanks,
David


On 12/10/2012 11:18 PM, David Holmes wrote:

David,

Many thanks for trialling this so promptly!

Do you have any suggestions for what instrumentation you would like to
see accompany this?

David

On 11/12/2012 1:41 PM, David M. Lloyd wrote:

On 12/10/2012 06:36 PM, David Holmes wrote:

On 10/12/2012 11:53 PM, David M. Lloyd wrote:

On 12/09/2012 10:03 PM, David Holmes wrote:

That sounds promising. Are you in a position to try out this
proposal on
a testbed with your app server?


Sure, I'd love to. What tree should I build?


Please apply the patches from the webrevs here:

http://cr.openjdk.java.net/~dholmes/concurrent-loaders/webrev.hotspot/
http://cr.openjdk.java.net/~dholmes/concurrent-loaders/webrev.jdk/

They should apply to a jdk8 or tl forest. (And I hope I didn't mess
something up generating the webrev ;-) )


Well, I've just gotten everything working and done some cursory testing
on a startup of an "empty" JBoss AS 7 instance, and then reviewing the
metrics reported by our class loader. Our timing metrics are not really
great for general-purpose benchmarking (for various uninteresting
reasons), but I can at least get enough information to know everything
is working more or less as expected:

On JDK 6 with our "unsafe" lockless modification, we're typically
loading 4707 classes, and I'm seeing anywhere from 0 to 5 define races
that were automatically resolved.

On JDK 7 using the standard registerAsParallelCapable() mechanism, we
are loading 4711 classes and I'm seeing 35-50 define races that were
automatically resolved - the overhead of locking starts to come to the
fore I think.

On JDK 8 with your patches, we are loading around 4750 classes and there
are, as expected, 0 define races (I believe, however, that we're getting
a false count though whenever defineClass() returns an already-defined
class - it would be nice if there were *some* way to detect that this
happened).

On our class loader implementations, I'm initializing this way:

static {
try {
ClassLoader.registerAsFullyConcurrent();
} catch (Throwable ignored) {
try {
ClassLoader.registerAsParallelCapable();
} catch (Throwable ignored2) {
}
}
}

The debugging message confirms that our class loaders are successfully
registered as fully concurrent, and the fact that it doesn't hang or
crash on JDK 7 indicates that they are still properly being registered
as parallel-capable on that platform.

Re: Proposal: Fully Concurrent ClassLoading


On 11/12/2012 9:58 PM, Peter Levart wrote:

On 12/11/2012 12:27 PM, David Holmes wrote:

Peter,

You are convincing me that all superclasses must be fully concurrent
too. Otherwise we are just trying to second-guess a whole bunch of
what-ifs. :)


If you think some more, yes. The superclass might not use
getClassLoadingLock() but rely on the fact that findClass() is allways
called under a guard of per-class-name lock, for example. It's a matter
of how far to go to prevent such miss-behaving fully-concurrent
subclasses. So far to also prevent fully-concurrent subclasses that
would otherwise be perfectly correct?

Maybe not. Creating custom ClassLoaders is not an average programmer's
job. Those that do this things will of course study the implementations
of superclasses they extend and do the right thing. And it's reasonable
to expect that they more or less will only extend JDK's ClassLoaders -
but on the other hand if they only extend JDK's class loaders, they are
not prevented to be fully-concurrent either way. Hm...


Again I think it is just too hard to try and second-guess how a 
parallel-loader might rely on the per-class locks (I actually don't see 
any reasonable use for them beyond flow-control), and then how a 
concurrent loader subclass might need to modify things.


If we simply disallow this then we can relax that constraint in the 
future if valid use-cases turn up for that capability. Of course if 
someone has a valid use-case during this discussion phase then of course 
that will influence the decision.


Thanks,
David


Peter



Thanks,
David

On 11/12/2012 7:44 PM, Peter Levart wrote:

On 12/11/2012 10:29 AM, David Holmes wrote:

On 11/12/2012 7:20 PM, Peter Levart wrote:

On 12/11/2012 03:55 AM, David Holmes wrote:

Question on the source code: registerAsFullyConcurrent has confusing
comment -
do the super classes all need to be parallel capable? Or do the
super
classes all need
to be FullyConcurrent? I assume the latter, so just fix the
comments.


Actually it is the former. There's no reason to require that all
superclasses be fully-concurrent. Of course a given loaders degree of
concurrency may be constrained by what it's supertype allows, but
there's no reason to actually force all the supertypes to be
fully-concurrent: it is enough that they are at least all parallel
capable.


Hi David,

There is one caveat: if ClassLoader X declares that it is
fully-concurrent and it's superclass Y is only parallel-capable,
then X
will act as fully-concurrent (returning null from
getClassLoadingLock()). superclass Y might or might not be coded to
use
the getClassLoadingLock(). X therefore has to know how Y is coded.
To be
defensive, X could ask for Y's registration and declare itself as only
parallel-capable if Y declares the same so that when Y is upgraded
to be
fully-concurrent, X would become fully-concurrent automatically. To
support situations where the same version of X would work in two
environments where in one Y is only parallel-capable and in the
other Y
is fully-concurrent, there could be a static API to retrieve the
registrations of superclasses.


I don't quite follow this. What code in the superclass are you
anticipating that the subclass will use which relies on the lock? Or
is this just an abstract "what if" scenario?


This is more or less "what if". There might be a subclass Y of say
java.lang.ClassLoader that overrides loadClass or findClass, declares
that it is parallel-capable and in the implementation of it's loadClass
or findClass, uses getClassLoadingLock() to synchronize access to it's
internal state. Now there comes class X extends Y that declares that it
is fully-concurrent. Of course this will not work, X has to declare that
it is parallel-capable, because Y uses getClassLoadingLock().

What I suggested in the next message is to not change the registration
API but rather provide getClassLoadingLock() that returns non-null locks
when any of the superclasses declare that they are only
parallel-capable, not fully-concurrent.

Regards, Peter



Thanks,
David
-


Or, to have less impact on future deprecation of old parallel-capable
registration API, the fully-concurrent registration API:

protected static boolean registerAsFullyConcurrent()

might take a boolean parameter:

protected static boolean registerAsFullyConcurrent(boolean
downgradeToPrallelCapableIfAnySuperclassIsNotFullyConcurrent)


and provide no accessible API to find out what the registration
actually
did (register as parallel-capable or fully-concurrent - return true in
any case).

Since all JDK provided ClassLoaders will be made fully concurrent,
this
might only be relevant if there is vendor A that currently provides
only
parallel-capable ClassLoader implementation and there is vendor B that
subclasses A's loader and wants to upgrade and be backward
compatible at
the same time.

Does this complicate things to much for no real benefit?

Regards, Peter

Re: Proposal: Fully Concurrent ClassLoading

2012-12-11 Thread David M. Lloyd

Ah, well for our concurrent class loader we just keep simple counters 
for all that stuff.  If we can boost that up to the ClassLoader level 
that'd be nifty.  The stats we track that have been useful are:


- Number of defined classes
- Number of 'races' i.e. define when already existent
- Number of modules (== class loaders) created in the module loader
- CPU time spent defining classes (though this is currently hard to 
capture accurately)


The platform ClassLoadingMXBean has a couple useful stats already; I 
could see adding a counter for the number of duplicate defines, and 
maybe adding some basic timing information in there too.  One other 
metrics that could be useful: the number of failed class load attempts, 
number of failed due to being missing, and the number failed due to 
linkage problems.


On 12/11/2012 07:32 PM, David Holmes wrote:

On 12/12/2012 1:10 AM, David M. Lloyd wrote:

No problem. What do you mean by "instrumentation"?


Any means of "seeing" when parallel define class actually occurred:

-  -verbose:class logging ?
-  jvmstat counter ?
- -XX:+TraceParallelDefine ?
- ???

Thanks,
David


On 12/10/2012 11:18 PM, David Holmes wrote:

David,

Many thanks for trialling this so promptly!

Do you have any suggestions for what instrumentation you would like to
see accompany this?

David

On 11/12/2012 1:41 PM, David M. Lloyd wrote:

On 12/10/2012 06:36 PM, David Holmes wrote:

On 10/12/2012 11:53 PM, David M. Lloyd wrote:

On 12/09/2012 10:03 PM, David Holmes wrote:

That sounds promising. Are you in a position to try out this
proposal on
a testbed with your app server?


Sure, I'd love to. What tree should I build?


Please apply the patches from the webrevs here:

http://cr.openjdk.java.net/~dholmes/concurrent-loaders/webrev.hotspot/
http://cr.openjdk.java.net/~dholmes/concurrent-loaders/webrev.jdk/

They should apply to a jdk8 or tl forest. (And I hope I didn't mess
something up generating the webrev ;-) )


Well, I've just gotten everything working and done some cursory testing
on a startup of an "empty" JBoss AS 7 instance, and then reviewing the
metrics reported by our class loader. Our timing metrics are not really
great for general-purpose benchmarking (for various uninteresting
reasons), but I can at least get enough information to know everything
is working more or less as expected:

On JDK 6 with our "unsafe" lockless modification, we're typically
loading 4707 classes, and I'm seeing anywhere from 0 to 5 define races
that were automatically resolved.

On JDK 7 using the standard registerAsParallelCapable() mechanism, we
are loading 4711 classes and I'm seeing 35-50 define races that were
automatically resolved - the overhead of locking starts to come to the
fore I think.

On JDK 8 with your patches, we are loading around 4750 classes and
there
are, as expected, 0 define races (I believe, however, that we're
getting
a false count though whenever defineClass() returns an already-defined
class - it would be nice if there were *some* way to detect that this
happened).

On our class loader implementations, I'm initializing this way:

static {
try {
ClassLoader.registerAsFullyConcurrent();
} catch (Throwable ignored) {
try {
ClassLoader.registerAsParallelCapable();
} catch (Throwable ignored2) {
}
}
}

The debugging message confirms that our class loaders are successfully
registered as fully concurrent, and the fact that it doesn't hang or
crash on JDK 7 indicates that they are still properly being registered
as parallel-capable on that platform.







--
- DML

Re: RFR: 8004748: clean up @build tags in RMI tests

2012-12-11 Thread Mandy Chung


Looks fine with me.

I notice that there are cases that still require @build the main class 
when it contains the source for other interfaces/classes that another 
class depends on - not a problem.


Mandy

On 12/11/2012 3:53 PM, Stuart Marks wrote:

Hi all,

Please review the following gigantic webrev [1] to clean up @build 
tags in RMI tests. Details underlying this change are in the bug 
report [2].


Briefly, if test classes listed in @build tags are in the wrong order, 
this trips over a jtreg problem that in turn causes a cascade of 
subsequent tests to fail. It's sensitive to the order in which tests 
run. The problem currently occurs in the jdk7u repo. It doesn't happen 
in jdk8 right now, but as things shift around it might occur in the 
future.


Naturally, I intend to backport this to 7u once it's in 8.

Shifting the @build tags in the test is a workaround for the jtreg 
bug, but jtreg isn't going to be fixed soon. Besides, the @build tags 
in the RMI tests needed to be cleaned up anyway. In particular, 
consolidating multiple @build tags into a single tag speeds up the RMI 
test run by about 2.5%.


I've also taken the opportunity to do a couple of related cleanups in 
a few places, such as fixing typos, rearranging tags to be in a more 
consistent order, removing unnecessary classes from @build lines, 
adding necessary ones, and in one case renaming a file that was 
spelled differently from the class that it contained. 
(CheckUnmarshall.java -> CheckUnmarshal.java; there are no textual 
changes to this file.)


Needless to say, all tests pass. In addition, I've run each test 
individually (i.e., with a clean JTwork directory) to ensure that 
there were no occurrences of the library class ordering issue that 
triggers the jtreg bug.


Thanks,

s'marks


[1] http://cr.openjdk.java.net/~smarks/reviews/8004748/webrev.0/

[2] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8004748

Re: 8004874: (profiles) Reduce dependency on java.beans to only add/removePropertyChangeListener