hg: jdk8/tl/jdk: 8003881: Prevent lambda implementing inner classes from allowing the creation of new instances

[robert . field]

Changeset: 896d4af2ebfd
Author:rfield
Date:  2012-12-06 21:55 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/896d4af2ebfd

8003881: Prevent lambda implementing inner classes from allowing the creation 
of new instances
Summary: Lambda implementing inner classes now has private constructor (thanks 
Kumar)
Reviewed-by: ksrini

! src/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java
+ test/java/lang/invoke/lambda/LambdaAccessControlDoPrivilegedTest.java
+ test/java/lang/invoke/lambda/LambdaAccessControlTest.java

Re: Review request: 8004042 : Arrrghs.java test failed on windows with access error.

[Stuart Marks]

OK, looks pretty good. But

One little thing is still bothering me. Suppose we get interrupted from the 
sleep() and bail out on that basis. If we got to the point where we're 
sleeping, we must have caught an AccessDeniedException previously. 
Unfortunately this exception is discarded if we were interrupted. This might 
lose valuable diagnostic information. So, what do we do with it? How about adding:


ie.addSuppressed(cause);

in the catch InterruptedException clause, before throwing the RuntimeException.

(There's another issue which is that if there were previous retries, the ADEs 
from them are thrown away. But maybe we should save that one for another day.)


s'marks

On 12/6/12 9:41 AM, David DeHaven wrote:


New webrev posted:
http://cr.openjdk.java.net/~ddehaven/8004042/webrev.2/

Summary:
- Cleaned it up a bit by change to a for loop (eliminating done flag)
- Throws RuntimeException instead of IOException, handles InterruptedException
- Bumped retry count to 10
- Changed @run mode to "main/othervm" as suggested by Kumar

-DrD-

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Remi Forax]

On 12/06/2012 11:05 PM, Jeff Hain wrote:

nitpicking welcome :)

AFAIK (*), removing 'private' for supplier field could prevent
javac to generate accessors for this field, in case it would be
accessed from outside inner class (but since it's not, it would
not yield better perfs, just smaller bytecode).

(*) If I correctly undertood what Rémi once said :)


It's true only if the field is accessed ouside the inner class but 
inside the enclosing class (or the enclosing class of the enclosing class).


BTW, at some point, it will be a good idea to get ride of all these 
method access$000 in java.lang/java.util at least.




-Jeff


Rémi

hg: jdk8/tl: 8004685: add java.util.function to CORE_PKGS.gmk

[mike . duigou]

Changeset: fb1bf5e5bc9e
Author:henryjen
Date:  2012-12-06 15:38 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/rev/fb1bf5e5bc9e

8004685: add java.util.function to CORE_PKGS.gmk
Reviewed-by: mduigou

! common/makefiles/javadoc/CORE_PKGS.gmk

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Jeff Hain]

>nitpicking welcome :)

AFAIK (*), removing 'private' for supplier field could prevent
javac to generate accessors for this field, in case it would be
accessed from outside inner class (but since it's not, it would
not yield better perfs, just smaller bytecode).

(*) If I correctly undertood what Rémi once said :)

-Jeff

Re: Misleading documentation for class java.nio.charset.spi.CharsetProvider

[Christian Schulte]

Am 12/06/12 18:50, schrieb Alan Bateman:
> On 06/12/2012 17:26, Christian Schulte wrote:
>> Hello,
>>
>> I am not sure if this is the correct mailing list to send this mail to.
>> Please apologize any inconvenience caused.
>>
>> The JDK 7 documentation of class java.nio.charset.spi.CharsetProvider
>> states the following:
>>
>> [...]
>> Charset providers are looked up via the current thread's context class
>> loader.
>> [...]
>>
>> Looking at method 'private static Iterator providers()' of class
>> 'java.nio.charset.Charset', the above documentation seems incorrect
>> since that method uses the system class loader.
>>
>> ClassLoader cl = ClassLoader.getSystemClassLoader();
>> ServiceLoader  sl =
>> ServiceLoader.load(CharsetProvider.class, cl);
>>
>> Is this intended ?
>>
> This is a long standing issue (going back to 1.4), we should remove this 
> from the javadoc.

Or update the code to use the thread context class loader if possible.
This would allow using custom charset providers with e.g. Webstart.
Anyway. Thanks for taking a look.

-- 
Christian Schulte

Re: Request for review: JDK-8004337: java/sql tests aren't run in test/Makefile

[Lance Andersen - Oracle]

Looks fine Rob
On Dec 6, 2012, at 4:22 PM, Rob McKenna wrote:

> Hi folks,
> 
> There's a missing folder in the jdk_other test target:
> 
> http://cr.openjdk.java.net/~robm/8004337/webrev.01/ 
> 
> 
>-Rob


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

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Remi Forax]

On 12/06/2012 09:38 PM, Peter Levart wrote:

On 12/06/2012 08:08 PM, Remi Forax wrote:

On 12/06/2012 08:01 PM, Peter Levart wrote:

There's a quick trick that guarantees in-lining of get/set/remove:

public static class FastThreadLocal extends ThreadLocal {
@Override
public final T get() { return super.get(); }

@Override
public final void set(T value) { super.set(value); }

@Override
public final void remove() { super.remove(); }
}

just use static type FastThreadLocal everywhere in code.

I tried it and it works.


No, there is no way to have such guarantee, here, it works either 
because the only class ThreadLocal you load is FastThreadLocal or 
because the VM has profiled the callsite see that you only use 
FastThreadLocal for a specific instruction.


Nothing is certain but death and taxes, I agree.

But think deeper, Remi!


I try, i try, i've trouble to understand what you mean



How do you explain the following test:

public class ThreadLocalTest {

static class Int { int value; }

static class TL0 extends ThreadLocal {}
static class TL1 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL2 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL3 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL4 extends ThreadLocal { public Int get() { 
return super.get(); } }


static long doTest(ThreadLocal tl) {
long t0 = System.nanoTime();
for (int i = 0; i < 1; i++)
tl.get().value++; // 
callsite 1

return System.nanoTime() - t0;
}


here, tl.get() (callsite 1) is called with TL0, TL1, TL2, TL3 and TL4, 
do tl.get() is a polymorphic call




static long doTest(FastThreadLocal tl) {
long t0 = System.nanoTime();
for (int i = 0; i < 1; i++)
tl.get().value++; // callsite 2
return System.nanoTime() - t0;
}


if TL0 is defined like as a FastThreadLocal. doTest is called and here 
tl.get() (callsite 2) is only called with a FastThreadLocal, so the call 
is inlined.




static long test0(ThreadLocal tl) {
if (tl instanceof FastThreadLocal)
return doTest((FastThreadLocal)tl);
else
return doTest(tl);
}

static void test(ThreadLocal tl) {
tl.set(new Int());
System.out.print(tl.getClass().getName() + ":");
for (int i = 0; i < 8; i++)
System.out.print(" " + test0(tl));
System.out.println();
}

public static void main(String[] args) {
TL0 tl0 = new TL0();
test(tl0);
test(new TL1());
test(new TL2());
test(new TL3());
test(new TL4());
test(tl0);
}
}


Which prints the following (demonstrating almost 2x slowdown of TL0 - 
last line compared to first):


test.ThreadLocalTest$TL0: 342716421 326105315 300744544 300654890 
300726346 300752009 300700781 300735651
test.ThreadLocalTest$TL1: 321424139 312128166 312173383 312125203 
312142144 312150949 316760957 313393554
test.ThreadLocalTest$TL2: 525661886 524169413 524184405 524215685 
524162050 524400364 524174966 454370228
test.ThreadLocalTest$TL3: 472042229 471071328 464387909 468047355 
464795171 464466481 464449567 464365974
test.ThreadLocalTest$TL4: 459651686 454142365 454129481 454180718 
454217277 454109611 454119988 456978405
test.ThreadLocalTest$TL0: 582252322 582773455 582612509 582753610 
582626360 582852195 582805654 582598285


Now with a simple change of:

static class TL0 extends FastThreadLocal {}

...the same test prints:

test.ThreadLocalTest$TL0: 330722181 325823711 301171182 309992192 
321868979 308111417 303806979 300612033
test.ThreadLocalTest$TL1: 330263857 326448062 300607081 300575641 
307442821 300616794 300548457 303462898
test.ThreadLocalTest$TL2: 319627165 311309477 311465815 311279612 
311294427 311315803 311470291 311293823
test.ThreadLocalTest$TL3: 526849874 524209792 524421574 524166747 
524396011 524163313 524395641 524165429
test.ThreadLocalTest$TL4: 464963126 455172216 455466304 455245487 
455368318 455093735 455125038 455317375
test.ThreadLocalTest$TL0: 300472239 300695398 300480230 303459397 
300451419 300679904 300445717 300451166



And that's very repeatable! Try it for yourself (on JDK8 of course).

Regards, Peter


so to summarize, the tl.get() in doTest(ThreadLocal) is polymorphic and 
the one in doTest(FastThreadLocal) is monomorphic thus inlinable.
It has nothing to do with the fact that FastThreadLocal override get, 
set and remove.
You can comment the methods inside FastThreadLocal to see that it change 
nothing.


regards,
Rémi

Request for review: JDK-8004337: java/sql tests aren't run in test/Makefile

[Rob McKenna]

Hi folks,

There's a missing folder in the jdk_other test target:

http://cr.openjdk.java.net/~robm/8004337/webrev.01/ 



-Rob

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Peter Levart]

Ok, I've got an explanation.

It's not because of using different static type of variables in code 
with final methods, but because TL0 was redirected to a separate method 
with separate call sites. The same happens using this variant:


static class TL0 extends ThreadLocal {}
static class TL1 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL2 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL3 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL4 extends ThreadLocal { public Int get() { 
return super.get(); } }


static long doTest(ThreadLocal tl) {
long t0 = System.nanoTime();
for (int i = 0; i < 1; i++)
tl.get().value++;
return System.nanoTime() - t0;
}

static long doTest0(ThreadLocal tl) {
long t0 = System.nanoTime();
for (int i = 0; i < 1; i++)
tl.get().value++;
return System.nanoTime() - t0;
}

static long test0(ThreadLocal tl) {
if (tl instanceof TL0)
return doTest0(tl);
else
return doTest(tl);
}


But I think that deoptimizations that Dough is talking about might be 
prevented by using the following variant of TL:


public class FastThreadLocal extends ThreadLocal {
public final T getFast() { return super.get(); }
public final void setFast(T value) { super.set(value); }
public final void removeFast() { super.remove(); }
}

and invoking the "fast" methods in code.


Right?

Regards, Peter


On 12/06/2012 09:38 PM, Peter Levart wrote:

On 12/06/2012 08:08 PM, Remi Forax wrote:

On 12/06/2012 08:01 PM, Peter Levart wrote:

There's a quick trick that guarantees in-lining of get/set/remove:

public static class FastThreadLocal extends ThreadLocal {
@Override
public final T get() { return super.get(); }

@Override
public final void set(T value) { super.set(value); }

@Override
public final void remove() { super.remove(); }
}

just use static type FastThreadLocal everywhere in code.

I tried it and it works.


No, there is no way to have such guarantee, here, it works either 
because the only class ThreadLocal you load is FastThreadLocal or 
because the VM has profiled the callsite see that you only use 
FastThreadLocal for a specific instruction.


Nothing is certain but death and taxes, I agree.

But think deeper, Remi!

How do you explain the following test:

public class ThreadLocalTest {

static class Int { int value; }

static class TL0 extends ThreadLocal {}
static class TL1 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL2 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL3 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL4 extends ThreadLocal { public Int get() { 
return super.get(); } }


static long doTest(ThreadLocal tl) {
long t0 = System.nanoTime();
for (int i = 0; i < 1; i++)
tl.get().value++;
return System.nanoTime() - t0;
}

static long doTest(FastThreadLocal tl) {
long t0 = System.nanoTime();
for (int i = 0; i < 1; i++)
tl.get().value++;
return System.nanoTime() - t0;
}

static long test0(ThreadLocal tl) {
if (tl instanceof FastThreadLocal)
return doTest((FastThreadLocal)tl);
else
return doTest(tl);
}

static void test(ThreadLocal tl) {
tl.set(new Int());
System.out.print(tl.getClass().getName() + ":");
for (int i = 0; i < 8; i++)
System.out.print(" " + test0(tl));
System.out.println();
}

public static void main(String[] args) {
TL0 tl0 = new TL0();
test(tl0);
test(new TL1());
test(new TL2());
test(new TL3());
test(new TL4());
test(tl0);
}
}


Which prints the following (demonstrating almost 2x slowdown of TL0 - 
last line compared to first):


test.ThreadLocalTest$TL0: 342716421 326105315 300744544 300654890 
300726346 300752009 300700781 300735651
test.ThreadLocalTest$TL1: 321424139 312128166 312173383 312125203 
312142144 312150949 316760957 313393554
test.ThreadLocalTest$TL2: 525661886 524169413 524184405 524215685 
524162050 524400364 524174966 454370228
test.ThreadLocalTest$TL3: 472042229 471071328 464387909 468047355 
464795171 464466481 464449567 464365974
test.ThreadLocalTest$TL4: 459651686 454142365 454129481 454180718 
454217277 454109611 454119988 456978405
test.ThreadLocalTest$TL0: 582252322 582773455 582612509 582753610 
582626360 582852195 582805654 582598285


Now with a simple change of:

static class TL0 extends FastThreadLocal {}

...the same test prints:

test.ThreadLocalTest$TL0: 330722181 325823711 301171182 309992192 
321868979 308111417 303806979 

Request for review: 8000525: Java.net.httpcookie api does not support 2-digit year format

[Rob McKenna]

Hi folks,

According to HttpCookie.java:

"""
There are 3 http cookie specifications:

   Netscape draft
   RFC 2109 -/http://www.ietf.org/rfc/rfc2109.txt/
   RFC 2965 -/http://www.ietf.org/rfc/rfc2965.txt/

HttpCookie class can accept all these 3 forms of syntax.
"""

According to http://www.ietf.org/rfc/rfc2109.txt section 10.1.2:

"""
Netscape's original proposal defined an Expires header that took a date 
value in a fixed-length variant format in place of Max-Age: Wdy, 
DD-Mon-YY HH:MM:SS GMT

"""

Thats in the "Historical" section provided to allow for compatibility 
with Netscape's implementation, which we also support: (as per 
http://docs.oracle.com/javase/6/docs/api/java/net/HttpCookie.html )


While we don't support the rfc explicitly, this change follows the 
format specified in section 5.1.1 of rfc 6265:


"""
3. If the year-value is greater than or equal to 70 and less than or 
equal to 99, increment the year-value by 1900.
4. If the year-value is greater than or equal to 0 and less than or 
equal to 69, increment the year-value by 2000.
1. NOTE: Some existing user agents interpret two-digit years 
differently.

"""

The webrev is at:

http://cr.openjdk.java.net/~robm/8000525/webrev.01/ 



Note: The addition of setLenient(false) has required changes to two 
existing testcases.


-Rob

hg: jdk8/tl/jdk: 8004374: CachedRowSetSwriter.writeData reports wrong number of conflicts in SyncProviderException

[lance . andersen]

Changeset: 41a1b110f34d
Author:lancea
Date:  2012-12-06 15:51 -0500
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/41a1b110f34d

8004374: CachedRowSetSwriter.writeData reports wrong number of conflicts in 
SyncProviderException
Reviewed-by: naoto

! src/share/classes/com/sun/rowset/internal/CachedRowSetWriter.java

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Peter Levart]

On 12/06/2012 08:08 PM, Remi Forax wrote:

On 12/06/2012 08:01 PM, Peter Levart wrote:

There's a quick trick that guarantees in-lining of get/set/remove:

public static class FastThreadLocal extends ThreadLocal {
@Override
public final T get() { return super.get(); }

@Override
public final void set(T value) { super.set(value); }

@Override
public final void remove() { super.remove(); }
}

just use static type FastThreadLocal everywhere in code.

I tried it and it works.


No, there is no way to have such guarantee, here, it works either 
because the only class ThreadLocal you load is FastThreadLocal or 
because the VM has profiled the callsite see that you only use 
FastThreadLocal for a specific instruction.


Nothing is certain but death and taxes, I agree.

But think deeper, Remi!

How do you explain the following test:

public class ThreadLocalTest {

static class Int { int value; }

static class TL0 extends ThreadLocal {}
static class TL1 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL2 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL3 extends ThreadLocal { public Int get() { 
return super.get(); } }
static class TL4 extends ThreadLocal { public Int get() { 
return super.get(); } }


static long doTest(ThreadLocal tl) {
long t0 = System.nanoTime();
for (int i = 0; i < 1; i++)
tl.get().value++;
return System.nanoTime() - t0;
}

static long doTest(FastThreadLocal tl) {
long t0 = System.nanoTime();
for (int i = 0; i < 1; i++)
tl.get().value++;
return System.nanoTime() - t0;
}

static long test0(ThreadLocal tl) {
if (tl instanceof FastThreadLocal)
return doTest((FastThreadLocal)tl);
else
return doTest(tl);
}

static void test(ThreadLocal tl) {
tl.set(new Int());
System.out.print(tl.getClass().getName() + ":");
for (int i = 0; i < 8; i++)
System.out.print(" " + test0(tl));
System.out.println();
}

public static void main(String[] args) {
TL0 tl0 = new TL0();
test(tl0);
test(new TL1());
test(new TL2());
test(new TL3());
test(new TL4());
test(tl0);
}
}


Which prints the following (demonstrating almost 2x slowdown of TL0 - 
last line compared to first):


test.ThreadLocalTest$TL0: 342716421 326105315 300744544 300654890 
300726346 300752009 300700781 300735651
test.ThreadLocalTest$TL1: 321424139 312128166 312173383 312125203 
312142144 312150949 316760957 313393554
test.ThreadLocalTest$TL2: 525661886 524169413 524184405 524215685 
524162050 524400364 524174966 454370228
test.ThreadLocalTest$TL3: 472042229 471071328 464387909 468047355 
464795171 464466481 464449567 464365974
test.ThreadLocalTest$TL4: 459651686 454142365 454129481 454180718 
454217277 454109611 454119988 456978405
test.ThreadLocalTest$TL0: 582252322 582773455 582612509 582753610 
582626360 582852195 582805654 582598285


Now with a simple change of:

static class TL0 extends FastThreadLocal {}

...the same test prints:

test.ThreadLocalTest$TL0: 330722181 325823711 301171182 309992192 
321868979 308111417 303806979 300612033
test.ThreadLocalTest$TL1: 330263857 326448062 300607081 300575641 
307442821 300616794 300548457 303462898
test.ThreadLocalTest$TL2: 319627165 311309477 311465815 311279612 
311294427 311315803 311470291 311293823
test.ThreadLocalTest$TL3: 526849874 524209792 524421574 524166747 
524396011 524163313 524395641 524165429
test.ThreadLocalTest$TL4: 464963126 455172216 455466304 455245487 
455368318 455093735 455125038 455317375
test.ThreadLocalTest$TL0: 300472239 300695398 300480230 303459397 
300451419 300679904 300445717 300451166



And that's very repeatable! Try it for yourself (on JDK8 of course).

Regards, Peter







Regards, Peter


cheers,
Rémi



On 12/06/2012 01:03 PM, Doug Lea wrote:

On 12/06/12 06:56, Vitaly Davidovich wrote:

Doug,

When you see the fast to slow ThreadLocal transition due to class 
loading
invalidating inlined get(), do you not then see it get restored 
back to fast
mode since the receiver type in your call sites is still the 
monomorphic
ThreadLocal (and not the unrelated subclasses)? Just trying to 
understand what

Rémi and you are saying.



The possible outcomes are fairly non-deterministic, depending
on hotspot's mood about recompiles, tiered-compile interactions,
method size, Amddahl's law interactions, phase of moon, etc.

(In j.u.c, we have learned that our users appreciate things
being predictably fast enough rather than being
unpredictably sometimes even faster but often slower.
So when we see such cases, as with ThreadLocal, they get added
to todo list.)

-Doug

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Alan Bateman]

On 06/12/2012 19:43, Akhil Arora wrote:


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

also incorporates a nitpick from Mike -
  Objects.requireNonNull returns its arg

Thanks - nitpicking welcome :)
Would it be worth including a note in the withInitial's javadoc to make 
it clear that the given Supplier needs to be thread-safe?


-Alan.

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Akhil Arora]

On 12/06/2012 11:02 AM, Remi Forax wrote:


just nitpicking,
return new SuppliedThreadLocal(supplier);
should be
return new SuppliedThreadLocal<>(supplier);

Also, can you add a @see in the constructor of ThreadLocal that
references ThreadLocal.withInitialValue(Supplier).


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

also incorporates a nitpick from Mike -
  Objects.requireNonNull returns its arg

Thanks - nitpicking welcome :)

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

[Alan Bateman]

On 06/12/2012 17:38, Daniel Fuchs wrote:

Updated the javadoc for newInstance():

 



-- daniel

Thumbs up from me!

-Alan

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Remi Forax]

On 12/06/2012 01:12 PM, Vitaly Davidovich wrote:

Understood - I'm just trying to make sure I don't have the wrong mental
model of this in my mind.  Taking pathology aside (e.g. code cache is full
when time to recompile, poor tiering interaction, etc), I'd expect the fast
inlined path to be restored assuming call site type profile (of the ones we
care about) doesn't change after other subclasses are loaded.  Would be
good if someone can correct that if it's inaccurate.  Apologies if this is
slightly hijacking the thread, but it's Remi's fault :).


:)

Usually, it's inline just fine because the code just stores the 
ThreadLocal in a static field, the VM profiles the code and you get the 
fast path.
But some codes use a ThreadLocal in a non static field or use a map of 
ThreadLocal so the profile becomes bloated, and there is no inlining 
anymore.


If all loaded codes always use the same ThreadLocal (using 
ThreadLocal.withSupplier()) then, the call to the supplier is not 
inlined but you don't care because it's not part of the fast path of get.


cheers,
Rémi



Sent from my phone
On Dec 6, 2012 7:03 AM, "Doug Lea"  wrote:


On 12/06/12 06:56, Vitaly Davidovich wrote:


Doug,

When you see the fast to slow ThreadLocal transition due to class loading
invalidating inlined get(), do you not then see it get restored back to
fast
mode since the receiver type in your call sites is still the monomorphic
ThreadLocal (and not the unrelated subclasses)? Just trying to understand
what
Rémi and you are saying.



The possible outcomes are fairly non-deterministic, depending
on hotspot's mood about recompiles, tiered-compile interactions,
method size, Amddahl's law interactions, phase of moon, etc.

(In j.u.c, we have learned that our users appreciate things
being predictably fast enough rather than being
unpredictably sometimes even faster but often slower.
So when we see such cases, as with ThreadLocal, they get added
to todo list.)

-Doug

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Remi Forax]

On 12/06/2012 08:01 PM, Peter Levart wrote:

There's a quick trick that guarantees in-lining of get/set/remove:

public static class FastThreadLocal extends ThreadLocal {
@Override
public final T get() { return super.get(); }

@Override
public final void set(T value) { super.set(value); }

@Override
public final void remove() { super.remove(); }
}

just use static type FastThreadLocal everywhere in code.

I tried it and it works.


No, there is no way to have such guarantee, here, it works either 
because the only class ThreadLocal you load is FastThreadLocal or 
because the VM has profiled the callsite see that you only use 
FastThreadLocal for a specific instruction.





Regards, Peter


cheers,
Rémi



On 12/06/2012 01:03 PM, Doug Lea wrote:

On 12/06/12 06:56, Vitaly Davidovich wrote:

Doug,

When you see the fast to slow ThreadLocal transition due to class 
loading
invalidating inlined get(), do you not then see it get restored back 
to fast
mode since the receiver type in your call sites is still the 
monomorphic
ThreadLocal (and not the unrelated subclasses)? Just trying to 
understand what

Rémi and you are saying.



The possible outcomes are fairly non-deterministic, depending
on hotspot's mood about recompiles, tiered-compile interactions,
method size, Amddahl's law interactions, phase of moon, etc.

(In j.u.c, we have learned that our users appreciate things
being predictably fast enough rather than being
unpredictably sometimes even faster but often slower.
So when we see such cases, as with ThreadLocal, they get added
to todo list.)

-Doug

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

[Mandy Chung]

On 12/6/12 9:38 AM, Daniel Fuchs wrote:

Updated the javadoc for newInstance():

 





Looks good to me.

Nit: you may want to use @linkplain instead of @link for this:
   {@link java.util.ServiceConfigurationError service configuration error},

Thanks
Mandy

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Remi Forax]

On 12/06/2012 07:29 PM, Doug Lea wrote:

On 12/06/12 13:00, Akhil Arora wrote:


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

- added SuppliedThreadLocal inner class
- reverted javadoc changes to no-args constructor
- added null check for Supplier



Great! Thanks for the quick response.

-Doug




Yes, great !

just nitpicking,
   return new SuppliedThreadLocal(supplier);
should be
   return new SuppliedThreadLocal<>(supplier);

Also, can you add a @see in the constructor of ThreadLocal that 
references ThreadLocal.withInitialValue(Supplier).


cheers,
Rémi

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Peter Levart]

There's a quick trick that guarantees in-lining of get/set/remove:

public static class FastThreadLocal extends ThreadLocal {
@Override
public final T get() { return super.get(); }

@Override
public final void set(T value) { super.set(value); }

@Override
public final void remove() { super.remove(); }
}

just use static type FastThreadLocal everywhere in code.

I tried it and it works.


Regards, Peter

On 12/06/2012 01:03 PM, Doug Lea wrote:

On 12/06/12 06:56, Vitaly Davidovich wrote:

Doug,

When you see the fast to slow ThreadLocal transition due to class 
loading
invalidating inlined get(), do you not then see it get restored back 
to fast

mode since the receiver type in your call sites is still the monomorphic
ThreadLocal (and not the unrelated subclasses)? Just trying to 
understand what

Rémi and you are saying.



The possible outcomes are fairly non-deterministic, depending
on hotspot's mood about recompiles, tiered-compile interactions,
method size, Amddahl's law interactions, phase of moon, etc.

(In j.u.c, we have learned that our users appreciate things
being predictably fast enough rather than being
unpredictably sometimes even faster but often slower.
So when we see such cases, as with ThreadLocal, they get added
to todo list.)

-Doug

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

[Remi Forax]

On 12/06/2012 06:35 PM, Brian Goetz wrote:
This suggestion makes me nervous, because we're already asking a great 
deal of type inference.  If you say


  reduce(Operators::plus)

and there are eight versions of Operators to choose from (and add 
boxing into the mix), I think we'll see a lot more inference errors.  
If the user says Integer::plus, the user has already told us what we 
want, and we're done.


I think that rule for method references are strong enough (unlike lambda 
you don't have to guess the type of the formal parameter) to work here,

but we should not play with the devil.

Rémi



On 12/5/2012 6: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

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Doug Lea]

On 12/06/12 13:00, Akhil Arora wrote:


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

- added SuppliedThreadLocal inner class
- reverted javadoc changes to no-args constructor
- added null check for Supplier



Great! Thanks for the quick response.

-Doug

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Akhil Arora]

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

Was there a CCC request for this?


Yes, a CCC was submitted on Nov 12, but it was deferred pending a 
discussion on this alias. I just updated and resubmitted the CCC based 
on Doug and Remi's suggestions.

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Akhil Arora]

On 12/06/2012 09:38 AM, Doug Lea wrote:

Much better; thanks!
As a minor matter, it probably makes sense to give that class a
name. Even though it is a non-public static class, the name
will be seen here and there by debuggers, stacktraces, etc,
and the default "$" name will be confusing. As in:


static final class SuppliedThreadLocal extends ThreadLocal ..

public static  ThreadLocal withInitial(Supplier
supplier) {
return new SuppliedThreadLocal(supplier);
}


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

- added SuppliedThreadLocal inner class
- reverted javadoc changes to no-args constructor
- added null check for Supplier

Thanks

Re: Misleading documentation for class java.nio.charset.spi.CharsetProvider

[Alan Bateman]

On 06/12/2012 17:26, Christian Schulte wrote:

Hello,

I am not sure if this is the correct mailing list to send this mail to.
Please apologize any inconvenience caused.

The JDK 7 documentation of class java.nio.charset.spi.CharsetProvider
states the following:

[...]
Charset providers are looked up via the current thread's context class
loader.
[...]

Looking at method 'private static Iterator providers()' of class
'java.nio.charset.Charset', the above documentation seems incorrect
since that method uses the system class loader.

ClassLoader cl = ClassLoader.getSystemClassLoader();
ServiceLoader  sl =
ServiceLoader.load(CharsetProvider.class, cl);

Is this intended ?

This is a long standing issue (going back to 1.4), we should remove this 
from the javadoc.


-Alan.

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Doug Lea]

On 12/06/12 12:01, Akhil Arora wrote:



redone - http://cr.openjdk.java.net/~akhil/8003246.1/webrev/



Much better; thanks!
As a minor matter, it probably makes sense to give that class a
name. Even though it is a non-public static class, the name
will be seen here and there by debuggers, stacktraces, etc,
and the default "$" name will be confusing. As in:


static final class SuppliedThreadLocal extends ThreadLocal ..

public static  ThreadLocal withInitial(Supplier supplier) {
   return new SuppliedThreadLocal(supplier);
}

Re: Review request: 8004042 : Arrrghs.java test failed on windows with access error.

[David DeHaven]

New webrev posted:
http://cr.openjdk.java.net/~ddehaven/8004042/webrev.2/

Summary: 
- Cleaned it up a bit by change to a for loop (eliminating done flag)
- Throws RuntimeException instead of IOException, handles InterruptedException
- Bumped retry count to 10
- Changed @run mode to "main/othervm" as suggested by Kumar

-DrD-

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

[Daniel Fuchs]

Updated the javadoc for newInstance():



-- daniel

On 12/6/12 11:04 AM, Alan Bateman wrote:

On 05/12/2012 16:17, Daniel Fuchs wrote:

Hi,

Please find below a revised version of the changes for
the javax.xml.parsers package.

It hopefully incorporates all comments I have received so far.




* better wording/formating for Javadoc changes
* using for( : ) syntax in findServiceProvider
* improved // comments explaining why the additional layer of
  RuntimeException is needed when wrapping ServiceConfigurationError.

best regards,

-- daniel

You've addressed all my comments and I think this looks very good.

One other comment. Now that the wording is "Otherwise the default
implementation, if present, is returned" it raises the question as to
how what happens if the default implementation is not present. A
suggestion is to just handle it in the next statement, something like
"In the case of a SCE or the default provider is not present, then FCE
will be thrown".

I see Mandy's comment about the bullet item "Platform default
DocumentBuilderFactory instance". I hadn't noticed this but
I assume this should just be removed now.

-Alan.

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Chris Hegarty]

Thank you Akhil, this looks much better.

I'm not completely comfortable with, "The initial value of the variable 
is provided by calling the {@code intialValue} method." Similarly, " The 
initial value of the variable is determined by invoking the {@code get} 
method on the {@code Supplier."


Should these statements defer to the text in initialValue? Or maybe just 
leave out the additional text in the no-args constructor, and have 
withInitial() just describe its implementation? My concern is with the 
omission of the behavior of set(), and the term 'initial value' could be 
misinterpreted.


Anyway, I'm relatively happy with this, let's see what others think.

-Chris.

On 12/06/2012 05:01 PM, Akhil Arora wrote:

On 12/06/2012 03:32 AM, Doug Lea wrote:


These seem like really good reasons to implement as you
suggested in last post, with a static factory that uses a non-public
*final* subclass that wires initialValue to supplier call,
and not otherwise modifying the ThreadLocal class.
It has the added benefit of, by not touching ThreadLocal,
guaranteeing that it is time/space-neutral for other existing
uses. Which also means that any future time/space improvements
in ThreadLocal will not need to be constrained by this change.

Jim/Akhil, could you please redo it this way?


redone - http://cr.openjdk.java.net/~akhil/8003246.1/webrev/

Thanks

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

[Brian Goetz]

This suggestion makes me nervous, because we're already asking a great 
deal of type inference.  If you say


  reduce(Operators::plus)

and there are eight versions of Operators to choose from (and add boxing 
into the mix), I think we'll see a lot more inference errors.  If the 
user says Integer::plus, the user has already told us what we want, and 
we're done.


On 12/5/2012 6: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

Misleading documentation for class java.nio.charset.spi.CharsetProvider

[Christian Schulte]

Hello,

I am not sure if this is the correct mailing list to send this mail to.
Please apologize any inconvenience caused.

The JDK 7 documentation of class java.nio.charset.spi.CharsetProvider
states the following:

[...]
Charset providers are looked up via the current thread's context class
loader.
[...]

Looking at method 'private static Iterator providers()' of class
'java.nio.charset.Charset', the above documentation seems incorrect
since that method uses the system class loader.

ClassLoader cl = ClassLoader.getSystemClassLoader();
ServiceLoader sl =
ServiceLoader.load(CharsetProvider.class, cl);

Is this intended ?


Regards,

-- 
Christian Schulte

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Akhil Arora]

On 12/06/2012 03:32 AM, Doug Lea wrote:


These seem like really good reasons to implement as you
suggested in last post, with a static factory that uses a non-public
*final* subclass that wires initialValue to supplier call,
and not otherwise modifying the ThreadLocal class.
It has the added benefit of, by not touching ThreadLocal,
guaranteeing that it is time/space-neutral for other existing
uses. Which also means that any future time/space improvements
in ThreadLocal will not need to be constrained by this change.

Jim/Akhil, could you please redo it this way?


redone - http://cr.openjdk.java.net/~akhil/8003246.1/webrev/

Thanks

signatures that are recorded for default methods

[Lance Andersen - Oracle]

Folks,

Will the signatures for interfaces that are recorded by the TCKs for interfaces 
record the fact that a method includes a default method? or will it just record 
the method definition?

I am assuming it will, but I know there has been discussion that a implementor 
could choose a different default implementation on one of the recent threads 
that was up for review.

I am still trying to understand what our guidelines are, if any for documenting 
the behavior of the supplied default methods given the javadocs are part of the 
specification for many APIs (and in some case the only spec).

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: Review request: 8004042 : Arrrghs.java test failed on windows with access error.

[David DeHaven]

>> A fix for intermittent test failures causing grief on Windows, particularly 
>> Windows 7 and 2008 systems:
>> http://cr.openjdk.java.net/~ddehaven/8004042/webrev.1/
>> 
>> The underlying cause is as of yet unknown, but I was able to create an 
>> environment that caused similar failures by running "watch -n 0.2 cat 
>> JTwork/scratch/atest.bat" in another shell then running jtreg. Without the 
>> patch it would fail very regularly (especially when isolated and looped 
>> 1,000 times), with the patch it always passes.
> 
> We had been discussing/speculating that the problem is the virus scanner 
> checking the old incarnation of the file at the same time we want to create a 
> new one. If so, and if it were to hold the file open with exclusive access, 
> that would explain the AccessDeniedException instead of 
> FileAlreadyExistsException (in the CREATE_NEW case).
> 
> (I still don't have an explanation for why the file deletion doesn't throw an 
> exception. That's what seems to cause jtreg some grief in its cleanup phase, 
> and for that reason we've added retry logic to jtreg.)

I see a lot of quirky filesystem behavior just using Windows 7, especially 
compared to Unixish systems … but maybe that's just me and my environment :)


> In any case I think backing off and retrying is probably what's getting us 
> the benefit, not the adjustment of the file creation flags.

I agree… I also think this is deeper than just the virus scanner, but I've not 
the expertise to validate that claim.


> In any case I'd increase the number of retry attempts. The test environment 
> is surprisingly hostile. In the RMI tests we try to open an unused network 
> port and retry 10 times if that fails, and sometimes that's not enough. (See 
> Jim Gish's recent changeset.)

For 8004317 (I think you pushed it yesterday)? I'll take a look at it.

That was my initial value. I backed it off thinking it was overly aggressive, 
but if the environment is that hostile perhaps it's not.


> InterruptedException shouldn't be ignored. Jtreg will interrupt the test if 
> it times out, so this interruption should be handled gracefully. Perhaps, 
> wrap the InterruptedException in an IOException and rethrow it? (Since the 
> caller is clearly prepared to handle IOException.) Terminating the loop and 
> returning normally doesn't seem right, since the file wasn't created 
> successfully.

Good point! I'll add that. I think I should through RuntimeException in both 
cases, to be consistent.


> This is only style, but perhaps it would be good to get rid of the 'done' 
> boolean and replace the 'done = true' statement with a 'return'. This 
> simplifies things a bit, I think.

I rewrote it to use a for loop instead and it's much cleaner. I'll post a new 
webrev later today.

-DrD-

Re: Review request: 8003562: Provide a command-line tool to find static dependencies

[Mandy Chung]

Thanks Erik for the help. I missed the files for images build.   I'll 
incorporate your patch.


Mandy

On 12/6/2012 6:37 AM, Erik Joelsson wrote:

Hello,

I've looked at the build changes and noted that the old and new build 
did not produce the same results. Here is a webrev with adjustments (I 
chose to do a full forest webrev because I needed to change a file in 
the root too):


http://cr.openjdk.java.net/~erikj/8003562/webrev.03/

The changes are:

* Add the jdeps package to tools.jar in new build
* Exclude jdeps from jre in new build
* Add "-DEXPAND_CLASSPATH_WILDCARDS 
-DNEVER_ACT_AS_SERVER_CLASS_MACHINE" to compile in old build. I wasn't 
sure if these were intended for this launcher, but I'm guessing that 
it makes sense to match the settings of javap and javah. The other 
alternative is to remove these from both old and new build.
* Add ./bin/jdeps to some exception lists in compare script. (jdeps, 
like most executables, contains the version string.)


With these changes, I don't see any new regressions in the compare 
script on my machine.


/Erik

On 2012-12-06 02:36, Mandy Chung wrote:

This review request (add a new jdeps tool in the JDK) include changes in
langtools and the jdk build.  I would need reviewers from the langtools
and the build-infra team.

Webrev for review:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/langtools-webrev.02/
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/jdeps/jdk-webrev.02/

The jdeps source is in the langtools repo.  The change in the jdk 
repo is

to add the new jdeps executable.  I made change in the old and new build
for the addition of this new jdeps tool.  I discussed with Jon whether I
should modify langtools/make/build.xml to add a new jdeps target. Since
the old build will be replaced by the new build soon, I simply add
com/sun/tools/jdeps as part of javap.includes.

Alan,

The -d option is kept as a hidden option and use as implementation. We
can remove it when it's determined not useful in the future.  I also
rename -v:summary to -summary.

Thanks
Mandy



$ jdep -h
Usage: jdeps  
where possible options include:
  -version Version information
  -classpath Specify where to find class files
  -summary Print dependency summary only
  -v:class Print class-level dependencies
  -v:package   Print package-level dependencies
  -p Restrict analysis to classes in this package
   (may be given multiple times)
  -eRestrict analysis to packages matching 
pattern

   (-p and -e are exclusive)
  -P  --profileShow profile or the file containing a package
  -R  --recursive  Traverse all dependencies recursively
  -all Process all classes specified in -classpath

$ jdep Notepad.jar Ensemble.jar
Notepad.jar -> D:\tools\devtools\jdk8\windows-i586\jre\lib\rt.jar
 (Notepad.jar)
  -> java.awt
  -> java.awt.event
  -> java.beans
  -> java.io
  -> java.lang
  -> java.net
  -> java.util
  -> java.util.logging
  -> javax.swing
  -> javax.swing.border
  -> javax.swing.event
  -> javax.swing.text
  -> javax.swing.tree
  -> javax.swing.undo

Ensemble.jar -> D:\tools\devtools\jdk8\windows-i586\jre\lib\jfxrt.jar
Ensemble.jar -> D:\tools\devtools\jdk8\windows-i586\jre\lib\rt.jar
   com.javafx.main (Ensemble.jar)
  -> java.applet
  -> java.awt
  -> java.awt.event
  -> java.io
  -> java.lang
  -> java.lang.reflect
  -> java.net
  -> java.security
  -> java.util
  -> java.util.jar
  -> javax.swing
  -> sun.misc JDK internal API 
(rt.jar)

Re: Proposal: Fully Concurrent ClassLoading

[David M. Lloyd]

On 12/06/2012 05:35 AM, Alan Bateman wrote:

On 05/12/2012 11:59, David Holmes wrote:

Java 7 introduced support for parallel classloading by adding to each
class loader a ConcurrentHashMap, referenced through a new field,
parallelLockMap. This contains a mapping from class names to Objects
to use as a classloading lock for that class name. This scheme has a
number of inefficiencies. To address this we propose for Java 8 the
notion of a fully concurrent classloader ...

This is a fairly simple proposal that I've written up as a blog entry:

https://blogs.oracle.com/dholmes/entry/parallel_classloading_revisited_fully_concurrent


The jdk7 implementation is very unfortunate, it's a pity this wasn't
caught before 7 shipped.

I think the proposal is good, it preserves compatibility, and if there
are loaders calling registerAsParallelCapable now (probably very few)
then it they may be able to change to using registerAsFullyConcurrent
without too much work.

I am tempted to suggest that registerAsParallelCapable should be
deprecated too.


I'm sorry I missed the original post, and I just want to add my 
wholehearted support for this idea.  Our application server's class 
loader implementation can be configured (on certain JVMs) to run in a 
lock-free mode and we have seen good performance and memory footprint as 
a result on these particular JVMs.


As far as the defineClass concurrent redefine issue goes - our current 
implementation simply does a try/catch for redefinition exceptions.  If 
such an exception occurs, we load the concurrently defined class and 
return that.  In practice, even with many threads, we would see 
relatively few such collisions.  But, a tryDefineClass or 
defineClassIfNotPresent would be a welcome addition for certain.


I'm very glad to see that Mr. Holmes has taken this initiative and found 
solutions to the various stumbling blocks.


--
- DML

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

[David Holmes]

On 6/12/2012 7:27 AM, 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:

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.


I don't find it desirable either.


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?


Why do you consider Math to be representative of "local style"? Math 
isn't even internally consistent with its style - see Math.max versus 
Math.addExact etc. And Math doesn't include the arguments type in its 
description. (Consistency is not a strong point in the Java APIs :( )


Given these are being added to support the new functional type I suggest 
using the same style as for the functional types.



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?


I withdraw my objection.

David
-


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

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Vitaly Davidovich]

Understood - I'm just trying to make sure I don't have the wrong mental
model of this in my mind.  Taking pathology aside (e.g. code cache is full
when time to recompile, poor tiering interaction, etc), I'd expect the fast
inlined path to be restored assuming call site type profile (of the ones we
care about) doesn't change after other subclasses are loaded.  Would be
good if someone can correct that if it's inaccurate.  Apologies if this is
slightly hijacking the thread, but it's Remi's fault :).

Sent from my phone
On Dec 6, 2012 7:03 AM, "Doug Lea"  wrote:

> On 12/06/12 06:56, Vitaly Davidovich wrote:
>
>> Doug,
>>
>> When you see the fast to slow ThreadLocal transition due to class loading
>> invalidating inlined get(), do you not then see it get restored back to
>> fast
>> mode since the receiver type in your call sites is still the monomorphic
>> ThreadLocal (and not the unrelated subclasses)? Just trying to understand
>> what
>> Rémi and you are saying.
>>
>>
> The possible outcomes are fairly non-deterministic, depending
> on hotspot's mood about recompiles, tiered-compile interactions,
> method size, Amddahl's law interactions, phase of moon, etc.
>
> (In j.u.c, we have learned that our users appreciate things
> being predictably fast enough rather than being
> unpredictably sometimes even faster but often slower.
> So when we see such cases, as with ThreadLocal, they get added
> to todo list.)
>
> -Doug
>
>
>
>
>

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Doug Lea]

On 12/06/12 06:56, Vitaly Davidovich wrote:

Doug,

When you see the fast to slow ThreadLocal transition due to class loading
invalidating inlined get(), do you not then see it get restored back to fast
mode since the receiver type in your call sites is still the monomorphic
ThreadLocal (and not the unrelated subclasses)? Just trying to understand what
Rémi and you are saying.



The possible outcomes are fairly non-deterministic, depending
on hotspot's mood about recompiles, tiered-compile interactions,
method size, Amddahl's law interactions, phase of moon, etc.

(In j.u.c, we have learned that our users appreciate things
being predictably fast enough rather than being
unpredictably sometimes even faster but often slower.
So when we see such cases, as with ThreadLocal, they get added
to todo list.)

-Doug

Re: Request for Review : CR#8004015 : [final (?) pass] Add interface extends and defaults for basic functional interfaces

[Chris Hegarty]

Mike,

Some small comments.

1) IntUnaryOperator.java

Typo in:
  + 30  * This is the primitive type specialization of {@link 
IntUnaryOperator} for
  +  31  * {@code int} and also may be used as a {@code 
IntUnaryOperator}. When
  +  32  * used as a {@code IntUnaryOperator} the default {@code 
operate} implementation
  +  33  * provided by this interface neither accepts null parameters 
nor does it return

  +  34  * null results.

   IntUnaryOperator -> UnaryOperator


2) Double/Int/Long Function

   "When used as a Function the default apply implementation provided
by this interface neither accepts null parameters nor does it
return null results."

"When used as a Function", is this really necessary, or should the
behavior of the default apply method just be described?

Why the restriction on null parameters, it seems overly restrictive
since applyAs accepts nulls, right?

3) package description

   "null values are accepted and returned by these functional
interfaces according to the constraints of the specification in
which the functional interfaces are used. The functional interfaces
themselves do not constrain or mandate use of null values. Most
usages of the functional interfaces will define the role, if any,
of null for that context."

Given these changes, doesn't this need to be reworked ( to indicate
that some methods specify null value behavior)?

4) Trivially, IntSupplier is missing a '', before "This is the
   primitive type..."

5) I agree with David, NPE should be defined where applicable.


-Chris.


On 12/06/2012 06:06 AM, David Holmes wrote:

On 6/12/2012 4:20 AM, Mike Duigou wrote:

I have updated webrev again to fix some reported javadoc technical
issues and added null handling specification to the
{Int|Double|Long}Supplier.

http://cr.openjdk.java.net/~mduigou/8004015/2/webrev/
http://cr.openjdk.java.net/~mduigou/8004015/2/specdiff/java/util/function/package-summary.html


I believe that this iteration is complete (or very nearly so).


Sorry to be a pain but this:

 left - the left operand, must be non-null

doesn't tell you what happens if it is null. Is it not better to simply
have:

@param left the left operand
@param right the right operand
@throws NullPointerException if either left or right are null

?

David
-



Mike

On Dec 4 2012, at 21:47 , Mike Duigou wrote:


Hello all;

I have updated the proposed patch. The changes primarily add class
and method documentation regarding handling of null for the primitive
specializations.

http://cr.openjdk.java.net/~mduigou/8004015/1/webrev/
http://cr.openjdk.java.net/~mduigou/8004015/1/specdiff/java/util/function/package-summary.html


I've also reformatted the source for the default methods.

Mike


On Nov 26 2012, at 18:12 , Mike Duigou wrote:


Hello all;

In the original patch which added the basic lambda functional
interfaces, CR#8001634 [1], none of the interfaces extended other
interfaces. The reason was primarily that the javac compiler did
not, at the time that 8001634 was proposed, support extension
methods. The compiler now supports adding of method defaults so this
patch improves the functional interfaces by filing in the
inheritance hierarchy.

Adding the parent interfaces and default methods allows each
functional interface to be used in more places. It is especially
important for the functional interfaces which support primitive
types, IntSupplier, IntFunction, IntUnaryOperator,
IntBinaryOperator, etc. We expect that eventually standard
implementations of these interfaces will be provided for functions
like max, min, sum, etc. By extending the reference oriented
functional interfaces such as Function, the primitive
implementations can be used with the boxed primitive types along
with the primitive types for which they are defined.

The patch to add parent interfaces and default methods can be found
here:

http://cr.openjdk.java.net/~mduigou/8004015/0/webrev/
http://cr.openjdk.java.net/~mduigou/8004015/0/specdiff/java/util/function/package-summary.html


Mike

[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c2e80176a697

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Vitaly Davidovich]

Doug,

When you see the fast to slow ThreadLocal transition due to class loading
invalidating inlined get(), do you not then see it get restored back to
fast mode since the receiver type in your call sites is still the
monomorphic ThreadLocal (and not the unrelated subclasses)? Just trying to
understand what Rémi and you are saying.

Thanks

Sent from my phone
On Dec 6, 2012 6:33 AM, "Doug Lea"  wrote:

> On 12/06/12 03:23, Remi Forax wrote:
>
>> On 12/06/2012 06:10 AM, David Holmes wrote:
>>
>>> On 6/12/2012 5:39 AM, Akhil Arora wrote:
>>>
 This patch adds a constructor to ThreadLocal to supply initial values
 using the new Supplier functional interface. Please review. This work
 was
 done by Jim Gish.

 http://cr.openjdk.java.net/~**akhil/8003246.0/webrev/
 http://bugs.sun.com/**bugdatabase/view_bug.do?bug_**id=8003246

>>> ...
>>>
>>
>> There are two good reasons to provide a new ThreadLocal<>(() ->
>> initialValue), if all goes as planned, every Supplier will share the same
>> class so instead of having one class by thread local that want to specify
>> an
>> initialValue, we will have only 2 classes (or 3 if the ThreadLocal that
>> takes
>> a Supplier is a subclass) in memory.
>>
>
>
> These seem like really good reasons to implement as you
> suggested in last post, with a static factory that uses a non-public
> *final* subclass that wires initialValue to supplier call,
> and not otherwise modifying the ThreadLocal class.
> It has the added benefit of, by not touching ThreadLocal,
> guaranteeing that it is time/space-neutral for other existing
> uses. Which also means that any future time/space improvements
> in ThreadLocal will not need to be constrained by this change.
>
> Jim/Akhil, could you please redo it this way?
>
>
>  Also, letting people to subclass ThreadLocal is not a good idea because
>> if in
>> one location someone decide to override get() to by example add a logging
>> code (I've seen a similar problem) then suddenly all the codes that use
>> ThreadLocal.get() will not be able to inline it.
>>
>
> (Yes. We see this when our (very heavy) uses of ThreadLocal
> inside j.u.c. go from fast to slow mode due to overrides in other
> unrelated ThreadLocal classes in application code. I've many
> times considered introducing a "JDK-internal" variant of
> ThreadLocal to protect against such things. Possibly even
> one with only a finite fixed capacity, that would allow
> further speed ups. Or maybe just introducing say a dozen
> extra dedicated fields in class Thread, making it optimally fast
> at the expense of non-extensibility.)
>
> -Doug
>
>
>

Re: Proposal: Fully Concurrent ClassLoading

[Alan Bateman]

On 05/12/2012 11:59, David Holmes wrote:
Java 7 introduced support for parallel classloading by adding to each 
class loader a ConcurrentHashMap, referenced through a new field, 
parallelLockMap. This contains a mapping from class names to Objects 
to use as a classloading lock for that class name. This scheme has a 
number of inefficiencies. To address this we propose for Java 8 the 
notion of a fully concurrent classloader ...


This is a fairly simple proposal that I've written up as a blog entry:

https://blogs.oracle.com/dholmes/entry/parallel_classloading_revisited_fully_concurrent 

The jdk7 implementation is very unfortunate, it's a pity this wasn't 
caught before 7 shipped.


I think the proposal is good, it preserves compatibility, and if there 
are loaders calling registerAsParallelCapable now (probably very few) 
then it they may be able to change to using registerAsFullyConcurrent 
without too much work.


I am tempted to suggest that registerAsParallelCapable should be 
deprecated too.


-Alan.

Re: Proposal: Fully Concurrent ClassLoading

[Peter Levart]

Hi David,

What about a middle-ground mode where findLoadedClass() and delegation 
to parent would be called without locks and only findClass() would be 
guarded by a per-class-name-per-classloader lock? In this mode 
concurrent attempts to define the same class would still be possible, 
but the possibility would be much lower. Usually findClass() involves 
non-ignorable level of resource-intensive work (ZIP, IO, NET) before it 
calls defineClass()...
Also, in this mode, the Map of locks could be implemented so that lock 
objects would be Weak(ly)Reference(d), since requests for already loaded 
classes would be satisfied by findLoadedClass() already. We'd just need 
a ConcurrentWeakValuesHashMap.


There must be something I'm missing here, isn't it?

Regards, Peter

On 12/05/2012 12:59 PM, David Holmes wrote:
Java 7 introduced support for parallel classloading by adding to each 
class loader a ConcurrentHashMap, referenced through a new field, 
parallelLockMap. This contains a mapping from class names to Objects 
to use as a classloading lock for that class name. This scheme has a 
number of inefficiencies. To address this we propose for Java 8 the 
notion of a fully concurrent classloader ...


This is a fairly simple proposal that I've written up as a blog entry:

https://blogs.oracle.com/dholmes/entry/parallel_classloading_revisited_fully_concurrent 



Please discuss this proposal here.

Thanks,
David Holmes

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Doug Lea]

On 12/06/12 03:23, Remi Forax wrote:

On 12/06/2012 06:10 AM, David Holmes wrote:

On 6/12/2012 5:39 AM, Akhil Arora wrote:

This patch adds a constructor to ThreadLocal to supply initial values
using the new Supplier functional interface. Please review. This work was
done by Jim Gish.

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

...


There are two good reasons to provide a new ThreadLocal<>(() ->
initialValue), if all goes as planned, every Supplier will share the same
class so instead of having one class by thread local that want to specify an
initialValue, we will have only 2 classes (or 3 if the ThreadLocal that takes
a Supplier is a subclass) in memory.



These seem like really good reasons to implement as you
suggested in last post, with a static factory that uses a non-public
*final* subclass that wires initialValue to supplier call,
and not otherwise modifying the ThreadLocal class.
It has the added benefit of, by not touching ThreadLocal,
guaranteeing that it is time/space-neutral for other existing
uses. Which also means that any future time/space improvements
in ThreadLocal will not need to be constrained by this change.

Jim/Akhil, could you please redo it this way?



Also, letting people to subclass ThreadLocal is not a good idea because if in
one location someone decide to override get() to by example add a logging
code (I've seen a similar problem) then suddenly all the codes that use
ThreadLocal.get() will not be able to inline it.


(Yes. We see this when our (very heavy) uses of ThreadLocal
inside j.u.c. go from fast to slow mode due to overrides in other
unrelated ThreadLocal classes in application code. I've many
times considered introducing a "JDK-internal" variant of
ThreadLocal to protect against such things. Possibly even
one with only a finite fixed capacity, that would allow
further speed ups. Or maybe just introducing say a dozen
extra dedicated fields in class Thread, making it optimally fast
at the expense of non-extensibility.)

-Doug

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

[Daniel Fuchs]

Hi Mandy,

On 12/5/12 10:55 PM, Mandy Chung wrote:

Hi Daniel,

Looks good to me.  One question - the last bullet in the search order
covers the default implementation case:
   "Platform default DocumentBuilderFactory instance."

Would it make sense to merge the statement "Otherwise, the default
implementation, if present, is returned" with the above statement?  e.g.
"Default implementation of DocumentBuilderFactory  if
present".


Good point - the description matches the internal implementation, but
maybe too literally:

ServiceLoader.load() may return the default implementation - or
may return null - hence the text 'the default implementation, if 
present, is returned.'


If ServiceLoader.load() returns null, then the algorithm will again
try to instantiate the default implementation - usually using
Class.forName with the TCCL - hence the last bullet:
'Platform default DocumentBuilderFactory instance.'

This last step is necessary because the default implementation
may be present without being accessible through a service
provider (that's the default configuration: in JDK 8 - with no
user configuration, ServiceLoader.load() will never instantiate
the default implementation)


However - from a user point of view - I don't think it makes sense
to differentiate these two cases: As a user of the JAXP parser factories
I won't really care how the default implementation is instantiated,
will I? So indeed - I think we should merge the two!

Thanks,

-- daniel




Mandy

On 12/5/2012 8:17 AM, Daniel Fuchs wrote:

Hi,

Please find below a revised version of the changes for
the javax.xml.parsers package.

It hopefully incorporates all comments I have received so far.




* better wording/formating for Javadoc changes
* using for( : ) syntax in findServiceProvider
* improved // comments explaining why the additional layer of
  RuntimeException is needed when wrapping ServiceConfigurationError.

best regards,

-- daniel

On 12/4/12 2:54 PM, Alan Bateman wrote:

On 03/12/2012 19:04, Daniel Fuchs wrote:

Hi,

This is a first webrev in a series that addresses a change intended
for JDK 8:

7169894: JAXP Plugability Layer: using service loader

I've been building up on Joe's work and Paul & Alan comments
from an earlier discussion thread [1]

This here addresses changes in the javax.xml.parsers
package for the SAXParserFactory and DocumentBuilderFactory - and
more specifically their no-argument newInstance() method.

This change replaces the custom code that was reading the
META-INF/services/ resources by a simple call to ServiceLoader.





Thank you very much for taking this one on. I think your approach to
take javax.xml.parsers on its own is good as the previous review rounds
got really stuck in the mire due to the number of factories, code
duplication, and subtle specification and implementation differences.

In the class descriptions it suggests that the default implementation
may be "installed as a module" but we aren't there yet so I'm not sure
about using the term "module". I think it should be okay to say
"installed as an extension" as ServiceLoader uses this term.

The class description also suggests that a SCE will be wrapped as a
FactoryConfigurationException but in FactoryFinder I see that it actual
wraps a RuntimeException. I think you can just use the no-arg
constructor then then use initCause to set the cause to the SCE.

Also the statement "If a misconfigured provider is encountered and SCE
is thrown" can probably be changed to "If SCE is thrown" as it can be
thrown for other reasons too.

Minor nit is that the updates to DocumentBuilderFactory's class
description requires a really wide screen compared to the rest of the
javadoc.

Minor implementation suggestion for findServiceProvider is that you can
use for-each to iterate through the providers.

Otherwise I think you've captured all the points of feedback from the
original review.

Finally one suggestion is to make keep notes on the "before & after"
behavior, this is something that will be important for release and
compatibility notes.

-Alan.

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

[Alan Bateman]

On 05/12/2012 16:17, Daniel Fuchs wrote:

Hi,

Please find below a revised version of the changes for
the javax.xml.parsers package.

It hopefully incorporates all comments I have received so far.

 



* better wording/formating for Javadoc changes
* using for( : ) syntax in findServiceProvider
* improved // comments explaining why the additional layer of
  RuntimeException is needed when wrapping ServiceConfigurationError.

best regards,

-- daniel

You've addressed all my comments and I think this looks very good.

One other comment. Now that the wording is "Otherwise the default 
implementation, if present, is returned" it raises the question as to 
how what happens if the default implementation is not present. A 
suggestion is to just handle it in the next statement, something like 
"In the case of a SCE or the default provider is not present, then FCE 
will be thrown".


I see Mandy's comment about the bullet item "Platform default 
DocumentBuilderFactory instance". I hadn't noticed this but 
I assume this should just be removed now.


-Alan.

Re: RFR: 8003246: Add Supplier to ThreadLocal

[Remi Forax]

On 12/06/2012 06:10 AM, David Holmes wrote:

On 6/12/2012 5:39 AM, Akhil Arora wrote:

This patch adds a constructor to ThreadLocal to supply initial values
using the new Supplier functional interface. Please review. This work
was done by Jim Gish.

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


Has anyone actually established that this is a useful addition to 
make? What is the acceptance criteria for adding these new mechanisms 
to existing classes? Was there a CCC request for this?


We need to be very wary of unnecessary bloat.


It's a necessary bloat :)

There are two good reasons to provide a new ThreadLocal<>(() -> 
initialValue),
if all goes as planned, every Supplier will share the same class so 
instead of having one class by thread local that want to specify an 
initialValue, we will have only 2 classes (or 3 if the ThreadLocal that 
takes a Supplier is a subclass) in memory. Also, letting people to 
subclass ThreadLocal is not a good idea because if in one location 
someone decide to override get() to by example add a logging code (I've 
seen a similar problem) then suddenly all the codes that use 
ThreadLocal.get() will not be able to inline it. Given that too many 
things are done using ThreadLocal in JEE container, having a way to 
provide an initial value without subclassing ThreadLocal is a good idea.




David


Rémi