Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Mandy Chung]

> On Oct 12, 2016, at 11:31 AM, Brent Christian  
> wrote:
> 
> On 10/11/16 8:25 AM, Alan Bateman wrote:
> >
>> One thing that would be good is to beef up
>> the test to cover more scenarios, esp. loader L1 extends loader L2 where
>> you've got 4 combination of capable/non-capable to test.
> 
> I updated the test case to provide better coverage:
> 
> http://cr.openjdk.java.net/~bchristi/8165793/webrev.01/
> 

+1.

Formatting nit: you could break .forEach to the next line

  96 ParaSubCL.class).forEach(IsParallelCapable::testClassLoaderClass);

Mandy

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Brent Christian]

On 10/11/16 8:25 AM, Alan Bateman wrote:
>

One thing that would be good is to beef up
the test to cover more scenarios, esp. loader L1 extends loader L2 where
you've got 4 combination of capable/non-capable to test.


I updated the test case to provide better coverage:

http://cr.openjdk.java.net/~bchristi/8165793/webrev.01/

Thanks,
-Brent

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Mandy Chung]

> On Oct 10, 2016, at 11:03 PM, Peter Levart  wrote:
> 
> Hi Brent,
> 
> 
> On 10/11/2016 12:44 AM, Brent Christian wrote:
>> On 10/10/16 2:30 PM, Mandy Chung wrote:
>>> 
>>> The patch looks fine.  It would be good to add @see
>>> #registerAsParallelCapable in this new method.  Also the first
>>> “parallel capable” occurrance in the class spec and the
>>> registerAsParallelCapable method spec to @linkplain
>>> #isParallelCapable.
>> 
>> Thanks for having a look, and for the suggestions.  I also added an @see 
>> #isParallelCapable in registerAsParallelCapable().
>> 
>> Webrev updated in place:
>> http://cr.openjdk.java.net/~bchristi/8165793/webrev.00/
>> 
>> -Brent
> 
> It would be marginally more efficient to check for parallelLockMap != null 
> instead of looking up the CL implementation class in the WeakHashMap within a 
> synchronized block, but I guess the performance of this method is not very 
> important, is it?

parallelLockMap != null is another way to check that.  Sinc this method isn’t 
performance critical, calling ParallelLoaders::isRegistered is easier to 
understand.

Mandy

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Alan Bateman]

On 10/10/2016 23:44, Brent Christian wrote:


On 10/10/16 2:30 PM, Mandy Chung wrote:


The patch looks fine.  It would be good to add @see
#registerAsParallelCapable in this new method.  Also the first
“parallel capable” occurrance in the class spec and the
registerAsParallelCapable method spec to @linkplain
#isParallelCapable.


Thanks for having a look, and for the suggestions.  I also added an 
@see #isParallelCapable in registerAsParallelCapable().


Webrev updated in place:
http://cr.openjdk.java.net/~bchristi/8165793/webrev.00/
The javadoc looks okay to me. One thing that would be good is to beef up 
the test to cover more scenarios, esp. loader L1 extends loader L2 where 
you've got 4 combination of capable/non-capable to test.


-Alan

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Peter Levart]

Hi Brent,


On 10/11/2016 12:44 AM, Brent Christian wrote:

On 10/10/16 2:30 PM, Mandy Chung wrote:


The patch looks fine.  It would be good to add @see
#registerAsParallelCapable in this new method.  Also the first
“parallel capable” occurrance in the class spec and the
registerAsParallelCapable method spec to @linkplain
#isParallelCapable.


Thanks for having a look, and for the suggestions.  I also added an 
@see #isParallelCapable in registerAsParallelCapable().


Webrev updated in place:
http://cr.openjdk.java.net/~bchristi/8165793/webrev.00/

-Brent


It would be marginally more efficient to check for parallelLockMap != 
null instead of looking up the CL implementation class in the 
WeakHashMap within a synchronized block, but I guess the performance of 
this method is not very important, is it?


Regards, Peter

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Brent Christian]

On 10/10/16 3:51 PM, Mandy Chung wrote:


Do you mind fixing @return in the registerAsParallelCapable method
with {@code true} amd {@code false} since you are in this method.  No
need for a new webrev.



Sure, no problem.

Thanks,
-Brent

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Mandy Chung]

> On Oct 10, 2016, at 3:44 PM, Brent Christian  
> wrote:
> 
> Thanks for having a look, and for the suggestions.  I also added an @see 
> #isParallelCapable in registerAsParallelCapable().
> 
> Webrev updated in place:
> http://cr.openjdk.java.net/~bchristi/8165793/webrev.00/
> 

+1

Do you mind fixing @return in the registerAsParallelCapable method with {@code 
true} amd {@code false} since you are in this method.  No need for a new webrev.

thanks
Mandy

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Brent Christian]

On 10/10/16 2:30 PM, Mandy Chung wrote:


The patch looks fine.  It would be good to add @see
#registerAsParallelCapable in this new method.  Also the first
“parallel capable” occurrance in the class spec and the
registerAsParallelCapable method spec to @linkplain
#isParallelCapable.


Thanks for having a look, and for the suggestions.  I also added an @see 
#isParallelCapable in registerAsParallelCapable().


Webrev updated in place:
http://cr.openjdk.java.net/~bchristi/8165793/webrev.00/

-Brent

Re: RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Mandy Chung]

> On Oct 10, 2016, at 1:36 PM, Brent Christian  
> wrote:
> 
> Hi,
> 
> Please review my fix for 8165793.  This follows the discussion here:
> http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-September/009328.html
> 
> The proposal is to add a new public method on ClassLoader:
> 
> /**
> * Returns {@code true} if this class loader is
> * {@linkplain #registerAsParallelCapable parallel capable}, otherwise
> * {@code false}.
> *
> * @return  {@code true} if this class loader is parallel capable,
> *  otherwise {@code false}.
> * @since   9
> */
> public final boolean isParallelCapable();

The patch looks fine.  It would be good to add @see #registerAsParallelCapable 
in this new method.  Also the first “parallel capable” occurrance in the class 
spec and the registerAsParallelCapable method spec to @linkplain 
#isParallelCapable.

Mandy

RFR 8165793 : Provide an API to query if a ClassLoader is parallel capable

[Brent Christian]

Hi,

Please review my fix for 8165793.  This follows the discussion here:
http://mail.openjdk.java.net/pipermail/jigsaw-dev/2016-September/009328.html

The proposal is to add a new public method on ClassLoader:

/**
 * Returns {@code true} if this class loader is
 * {@linkplain #registerAsParallelCapable parallel capable}, otherwise
 * {@code false}.
 *
 * @return  {@code true} if this class loader is parallel capable,
 *  otherwise {@code false}.
 * @since   9
 */
public final boolean isParallelCapable();

Bug:
https://bugs.openjdk.java.net/browse/JDK-8165793
Webrev:
http://cr.openjdk.java.net/~bchristi/8165793/webrev.00/

Thanks!

-Brent