Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Paul Sandoz]

Hi Peter,

You found that annoying restriction :-) at this point i think this is mostly 
redundant.

This is something i planned to update and limit the restriction to code within 
j.l.invoke and sun.invoke packages.

I'll follow up with a patch soon to unblock, but feel free to beat me to it if 
you wish.

Paul.

> On 14 Apr 2016, at 16:25, Peter Levart  wrote:
> 
> Hi Paul,
> 
> I wanted to try using VarHandles for code internal to JDK but there's a 
> problem with MethodHandles.lookup(). It doesn't allow the caller class loaded 
> by the bootstrap class loader and located in either java.* or sun.* (but not 
> sun.invoke.*) packages:
> 
> 
>private static void checkUnprivilegedlookupClass(Class lookupClass, 
> int allowedModes) {
>String name = lookupClass.getName();
>if (name.startsWith("java.lang.invoke."))
>throw newIllegalArgumentException("illegal lookupClass: 
> "+lookupClass);
> 
>// For caller-sensitive MethodHandles.lookup()
>// disallow lookup more restricted packages
>if (allowedModes == ALL_MODES && lookupClass.getClassLoader() == 
> null) {
>if (name.startsWith("java.") ||
>(name.startsWith("sun.") && 
> !name.startsWith("sun.invoke."))) {
>throw newIllegalArgumentException("illegal lookupClass: " 
> + lookupClass);
>}
>}
>}
> 
> 
> ...strangely, other bootstrap class loaded callers located in jdk.* are 
> allowed. Why such distinction? Is there or will there be an official way to 
> use VarHandles in JDK code and not having to resort to work-arounds like 
> MethodHandles.Lookup.class.getDeclaredField("IMPL_LOOKUP").setAccessible(true)?
> 
> 
> Regards, Peter
> 

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Peter Levart]

Hi Paul,

I wanted to try using VarHandles for code internal to JDK but there's a 
problem with MethodHandles.lookup(). It doesn't allow the caller class 
loaded by the bootstrap class loader and located in either java.* or 
sun.* (but not sun.invoke.*) packages:



private static void checkUnprivilegedlookupClass(Class 
lookupClass, int allowedModes) {

String name = lookupClass.getName();
if (name.startsWith("java.lang.invoke."))
throw newIllegalArgumentException("illegal lookupClass: 
"+lookupClass);


// For caller-sensitive MethodHandles.lookup()
// disallow lookup more restricted packages
if (allowedModes == ALL_MODES && 
lookupClass.getClassLoader() == null) {

if (name.startsWith("java.") ||
(name.startsWith("sun.") && 
!name.startsWith("sun.invoke."))) {
throw newIllegalArgumentException("illegal 
lookupClass: " + lookupClass);

}
}
}


...strangely, other bootstrap class loaded callers located in jdk.* are 
allowed. Why such distinction? Is there or will there be an official way 
to use VarHandles in JDK code and not having to resort to work-arounds 
like 
MethodHandles.Lookup.class.getDeclaredField("IMPL_LOOKUP").setAccessible(true)?



Regards, Peter

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Paul Sandoz]

> On 6 Apr 2016, at 16:45, Claes Redestad  wrote:
> 
> On 04/06/2016 04:09 PM, Paul Sandoz wrote:
>>> small streams become big rivers (i don't know the idiomatic sentence in 
>>> English, so it's a rough translation from a French idiom),
>>> "Death by a thousand cuts" is one of my favorites:).  A "flat profile" is 
>>> another description of a similar thing.
>>> 
>> I still remain unconvinced in this case that such changes warrant an 
>> increase in unsafe usage (temporary or otherwise).
>> 
> 
> I did not intend for this patch to spark any controversy - in my mind it was 
> just a rather straightforward and easy way to save a few Kbs (and some 
> theoretic startup time) on small program startup and I'm happy to withdraw it 
> based on the feedback.
> 

I don’t think there is any controversy. I just think we should place this one 
aside for the moment. We can revisit later on.

Paul.

> I do however think that reducing the dependency graph of things which are 
> loaded in this early has merits on its own, regardless of how much it 
> actually improves things. Using VHs here - or even in CHM - seems more 
> controversial to me than using Unsafe to take shortcuts in low-level class 
> libraries that need to boot fast and with as few dependencies as possible 
> (since that allows them to be used in more places).
> 
> Thanks!
> 
> /Claes

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Paul Sandoz]

> On 6 Apr 2016, at 16:52, Vitaly Davidovich  wrote:
> 
> So are there any bootstrap issues with VH that would preclude using it in 
> BufferedInputStream?

FWIW I did a quick perf experiment a while ago modifying A*FU to use VHs 
underneath and the VM booted just fine.


> Is that even known at this point?

Not precisely.


> You mentioned that VH was designed to minimize bootstrapping issues, but 
> where's the "minimized" line drawn?
> 

I want to draw the line where it’s possible to modify CHM to use VHs.

Paul.

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Vitaly Davidovich]

So are there any bootstrap issues with VH that would preclude using it in
BufferedInputStream? Is that even known at this point? You mentioned that
VH was designed to minimize bootstrapping issues, but where's the
"minimized" line drawn?

On Wed, Apr 6, 2016 at 10:09 AM, Paul Sandoz  wrote:

>
> > On 6 Apr 2016, at 15:28, fo...@univ-mlv.fr wrote:
> >
> > > > While i fully agree in the general case, in disagree for this
> specific
> > > > case,
> > > > there are few uses of ARFU inside the JDK (or outside) but currently
> > > > because BufferedInputStream uses an ARFU, each time someone starts a
> Java
> > > > application, the VM loads ARFU and its implementation with a high
> > > > probability to no need it at all after
> > > >
> > >
> > > Does that really contribute sufficiently to slow down in start time? It
> > > really seems like a micro-optimisation.
> >
> > small streams become big rivers (i don't know the idiomatic sentence in
> English, so it's a rough translation from a French idiom),
> > "Death by a thousand cuts" is one of my favorites :).  A "flat profile"
> is another description of a similar thing.
> >
> I still remain unconvinced in this case that such changes warrant an
> increase in unsafe usage (temporary or otherwise).
>
> > In general, I agree that easy/cheap/maintainable/etc wins should be done
> even if individually they don't matter much (if that were the requirement,
> there would be no progress whatsoever).
> >
> > In this case, using Unsafe for now seems trivial; when VH is ready,
> someone is going to sweep the code anyway and this will be just one more
> place to mechanically update.  Is VH definitely going to be part of Java 9?
> If so, then perhaps no point in making this change unless it's going to be
> backported to Java 8.
> >
> > Also don't forget that releasing VarHandle API as a public API for 9 is
> vastly different from replacing all usages of Unsafe by some VarHandle
> methods inside the JDK, see Paul's answser about the bootstrap issues.
> >
>
> VHs are now in jdk9/dev. I dunno when it will rise up to the master and
> the next EA build.
>
> Paul.
>

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Claes Redestad]

On 04/06/2016 04:09 PM, Paul Sandoz wrote:

small streams become big rivers (i don't know the idiomatic sentence in 
English, so it's a rough translation from a French idiom),
"Death by a thousand cuts" is one of my favorites:).  A "flat profile" is 
another description of a similar thing.


I still remain unconvinced in this case that such changes warrant an increase 
in unsafe usage (temporary or otherwise).



I did not intend for this patch to spark any controversy - in my mind it 
was just a rather straightforward and easy way to save a few Kbs (and 
some theoretic startup time) on small program startup and I'm happy to 
withdraw it based on the feedback.


I do however think that reducing the dependency graph of things which 
are loaded in this early has merits on its own, regardless of how much 
it actually improves things. Using VHs here - or even in CHM - seems 
more controversial to me than using Unsafe to take shortcuts in 
low-level class libraries that need to boot fast and with as few 
dependencies as possible (since that allows them to be used in more places).


Thanks!

/Claes

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Remi Forax]

- Mail original -
> De: "Paul Sandoz" <paul.san...@oracle.com>
> Cc: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net>
> Envoyé: Mercredi 6 Avril 2016 16:09:14
> Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of  
> AtomicReferenceFieldUpdater with Unsafe
> 
> 
> > On 6 Apr 2016, at 15:28, fo...@univ-mlv.fr wrote:
> > 
> > > > While i fully agree in the general case, in disagree for this specific
> > > > case,
> > > > there are few uses of ARFU inside the JDK (or outside) but currently
> > > > because BufferedInputStream uses an ARFU, each time someone starts a
> > > > Java
> > > > application, the VM loads ARFU and its implementation with a high
> > > > probability to no need it at all after
> > > >
> > >
> > > Does that really contribute sufficiently to slow down in start time? It
> > > really seems like a micro-optimisation.
> > 
> > small streams become big rivers (i don't know the idiomatic sentence in
> > English, so it's a rough translation from a French idiom),
> > "Death by a thousand cuts" is one of my favorites :).  A "flat profile" is
> > another description of a similar thing.
> > 
> I still remain unconvinced in this case that such changes warrant an increase
> in unsafe usage (temporary or otherwise).

Ok, you win :)

> 
> > In general, I agree that easy/cheap/maintainable/etc wins should be done
> > even if individually they don't matter much (if that were the requirement,
> > there would be no progress whatsoever).
> > 
> > In this case, using Unsafe for now seems trivial; when VH is ready, someone
> > is going to sweep the code anyway and this will be just one more place to
> > mechanically update.  Is VH definitely going to be part of Java 9? If so,
> > then perhaps no point in making this change unless it's going to be
> > backported to Java 8.
> > 
> > Also don't forget that releasing VarHandle API as a public API for 9 is
> > vastly different from replacing all usages of Unsafe by some VarHandle
> > methods inside the JDK, see Paul's answser about the bootstrap issues.
> > 
> 
> VHs are now in jdk9/dev. I dunno when it will rise up to the master and the
> next EA build.

Oh, i didn't notice.
congrat !

> 
> Paul.
> 

Rémi

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Paul Sandoz]

> On 6 Apr 2016, at 15:28, fo...@univ-mlv.fr wrote:
> 
> > > While i fully agree in the general case, in disagree for this specific
> > > case,
> > > there are few uses of ARFU inside the JDK (or outside) but currently
> > > because BufferedInputStream uses an ARFU, each time someone starts a Java
> > > application, the VM loads ARFU and its implementation with a high
> > > probability to no need it at all after
> > >
> >
> > Does that really contribute sufficiently to slow down in start time? It
> > really seems like a micro-optimisation.
> 
> small streams become big rivers (i don't know the idiomatic sentence in 
> English, so it's a rough translation from a French idiom),
> "Death by a thousand cuts" is one of my favorites :).  A "flat profile" is 
> another description of a similar thing.
> 
I still remain unconvinced in this case that such changes warrant an increase 
in unsafe usage (temporary or otherwise).

> In general, I agree that easy/cheap/maintainable/etc wins should be done even 
> if individually they don't matter much (if that were the requirement, there 
> would be no progress whatsoever).
> 
> In this case, using Unsafe for now seems trivial; when VH is ready, someone 
> is going to sweep the code anyway and this will be just one more place to 
> mechanically update.  Is VH definitely going to be part of Java 9? If so, 
> then perhaps no point in making this change unless it's going to be 
> backported to Java 8.
> 
> Also don't forget that releasing VarHandle API as a public API for 9 is 
> vastly different from replacing all usages of Unsafe by some VarHandle 
> methods inside the JDK, see Paul's answser about the bootstrap issues.
> 

VHs are now in jdk9/dev. I dunno when it will rise up to the master and the 
next EA build.

Paul.

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[forax]

- Mail original -

> De: "Vitaly Davidovich" <vita...@gmail.com>
> À: "Remi Forax" <fo...@univ-mlv.fr>
> Cc: "Paul Sandoz" <paul.san...@oracle.com>, "core-libs-dev Libs"
> <core-libs-dev@openjdk.java.net>
> Envoyé: Mercredi 6 Avril 2016 15:04:39
> Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of
> AtomicReferenceFieldUpdater with Unsafe

> On Wednesday, April 6, 2016, Remi Forax < fo...@univ-mlv.fr > wrote:

> > - Mail original -
> 
> > > De: "Paul Sandoz" < paul.san...@oracle.com >
> 
> > > Cc: "core-libs-dev Libs" < core-libs-dev@openjdk.java.net >
> 
> > > Envoyé: Mercredi 6 Avril 2016 12:37:50
> 
> > > Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of
> > > AtomicReferenceFieldUpdater with Unsafe
> 
> > >
> 
> > >
> 
> > > > On 6 Apr 2016, at 12:10, Remi Forax < fo...@univ-mlv.fr > wrote:
> 
[...] 

> > > > While i fully agree in the general case, in disagree for this specific
> 
> > > > case,
> 
> > > > there are few uses of ARFU inside the JDK (or outside) but currently
> 
> > > > because BufferedInputStream uses an ARFU, each time someone starts a
> > > > Java
> 
> > > > application, the VM loads ARFU and its implementation with a high
> 
> > > > probability to no need it at all after
> 
> > > >
> 
> > >
> 
> > > Does that really contribute sufficiently to slow down in start time? It
> 
> > > really seems like a micro-optimisation.
> 

> > small streams become big rivers (i don't know the idiomatic sentence in
> > English, so it's a rough translation from a French idiom),
> 
> "Death by a thousand cuts" is one of my favorites :). A "flat profile" is
> another description of a similar thing.

> In general, I agree that easy/cheap/maintainable/etc wins should be done even
> if individually they don't matter much (if that were the requirement, there
> would be no progress whatsoever).

> In this case, using Unsafe for now seems trivial; when VH is ready, someone
> is going to sweep the code anyway and this will be just one more place to
> mechanically update. Is VH definitely going to be part of Java 9? If so,
> then perhaps no point in making this change unless it's going to be
> backported to Java 8.
Also don't forget that releasing VarHandle API as a public API for 9 is vastly 
different from replacing all usages of Unsafe by some VarHandle methods inside 
the JDK, see Paul's answser about the bootstrap issues. 

Rémi 

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Vitaly Davidovich]

On Wednesday, April 6, 2016, Remi Forax <fo...@univ-mlv.fr> wrote:

> - Mail original -
> > De: "Paul Sandoz" <paul.san...@oracle.com <javascript:;>>
> > Cc: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net <javascript:;>>
> > Envoyé: Mercredi 6 Avril 2016 12:37:50
> > Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of
> AtomicReferenceFieldUpdater with Unsafe
> >
> >
> > > On 6 Apr 2016, at 12:10, Remi Forax <fo...@univ-mlv.fr <javascript:;>>
> wrote:
> > >
> > > - Mail original -
> > >> De: "Paul Sandoz" <paul.san...@oracle.com <javascript:;>>
> > >> Cc: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net
> <javascript:;>>
> > >> Envoyé: Mercredi 6 Avril 2016 09:37:00
> > >> Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of
> > >>AtomicReferenceFieldUpdater with Unsafe
> > >>
> > >>
> > >>> On 5 Apr 2016, at 17:15, Claes Redestad <claes.redes...@oracle.com
> <javascript:;>>
> > >>> wrote:
> > >>>
> > >>> On 04/04/2016 05:45 PM, Martin Buchholz wrote:
> > >>>>> I think this one is going too far.
> > >>>>>
> > >>>>> A*FU/VarHandles should are supposed to act like a go-to
> replacement for
> > >>>>> Unsafe throughout the class library, and we want to shrink the
> Unsafe
> > >>>>> exposure. Also, I don't think removing A*FU in favor of Unsafe here
> > >>>>> wins
> > >>>>> us anything: there should be no throughput hit, and we *will* load
> A*FU
> > >>>>> down the road anyway, negating the startup wins.
> > >>
> > >> +1 i would leave this one as is.
> > >>
> > >> In general same goes for the @Stable/ForceInline annotations etc. We
> > >> should
> > >> use this stuff carefully within java.base and also sparingly to
> qualifying
> > >> JDK modules.
> > >
> > > While i fully agree in the general case, in disagree for this specific
> > > case,
> > > there are few uses of ARFU inside the JDK (or outside) but currently
> > > because BufferedInputStream uses an ARFU, each time someone starts a
> Java
> > > application, the VM loads ARFU and its implementation with a high
> > > probability to no need it at all after
> > >
> >
> > Does that really contribute sufficiently to slow down in start time? It
> > really seems like a micro-optimisation.
>
> small streams become big rivers (i don't know the idiomatic sentence in
> English, so it's a rough translation from a French idiom),

"Death by a thousand cuts" is one of my favorites :).  A "flat profile" is
another description of a similar thing.

In general, I agree that easy/cheap/maintainable/etc wins should be done
even if individually they don't matter much (if that were the requirement,
there would be no progress whatsoever).

In this case, using Unsafe for now seems trivial; when VH is ready, someone
is going to sweep the code anyway and this will be just one more place to
mechanically update.  Is VH definitely going to be part of Java 9? If so,
then perhaps no point in making this change unless it's going to be
backported to Java 8.

currently the main problem is that the loading of modules adds so much
> overhead to the startup time that you see nothing :(
>
> Also, startup time is one aspect, size of the VM data structures at
> runtime, size of the CDS archive* are also impacted.
>
> >
> >
> > > ARFU are not a replacement of Unsafe, VarHandles are. So adding a new
> usage
> > > of Unsafe for a good reason goes in the right direction here,
> > > when VarHandle will replace usages of Unsafe, the code of
> > > BufferedInputStream will be refactored again.
> > >
> >
> > My preference is not to change it if it’s gonna change later on.
>
> but there is also a good chance to miss that change later because you will
> look for usages of Unsafe and not usages of ARFU.
> Replacing ARFU by Unsafe in BufferedInputStream makes the code more
> similar to the other usages of Unsafe in the JDK.
>
> [...]
>
> >
> > Paul.
> >
>
> Rémi
>
> * removing classes from a CDS archive is important for 32 bits windows VM
>
> Rémi
>


-- 
Sent from my phone

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Remi Forax]

- Mail original -
> De: "Paul Sandoz" <paul.san...@oracle.com>
> Cc: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net>
> Envoyé: Mercredi 6 Avril 2016 12:37:50
> Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of  
> AtomicReferenceFieldUpdater with Unsafe
> 
> 
> > On 6 Apr 2016, at 12:10, Remi Forax <fo...@univ-mlv.fr> wrote:
> > 
> > - Mail original -
> >> De: "Paul Sandoz" <paul.san...@oracle.com>
> >> Cc: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net>
> >> Envoyé: Mercredi 6 Avril 2016 09:37:00
> >> Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of
> >>AtomicReferenceFieldUpdater with Unsafe
> >> 
> >> 
> >>> On 5 Apr 2016, at 17:15, Claes Redestad <claes.redes...@oracle.com>
> >>> wrote:
> >>> 
> >>> On 04/04/2016 05:45 PM, Martin Buchholz wrote:
> >>>>> I think this one is going too far.
> >>>>> 
> >>>>> A*FU/VarHandles should are supposed to act like a go-to replacement for
> >>>>> Unsafe throughout the class library, and we want to shrink the Unsafe
> >>>>> exposure. Also, I don't think removing A*FU in favor of Unsafe here
> >>>>> wins
> >>>>> us anything: there should be no throughput hit, and we *will* load A*FU
> >>>>> down the road anyway, negating the startup wins.
> >> 
> >> +1 i would leave this one as is.
> >> 
> >> In general same goes for the @Stable/ForceInline annotations etc. We
> >> should
> >> use this stuff carefully within java.base and also sparingly to qualifying
> >> JDK modules.
> > 
> > While i fully agree in the general case, in disagree for this specific
> > case,
> > there are few uses of ARFU inside the JDK (or outside) but currently
> > because BufferedInputStream uses an ARFU, each time someone starts a Java
> > application, the VM loads ARFU and its implementation with a high
> > probability to no need it at all after
> > 
> 
> Does that really contribute sufficiently to slow down in start time? It
> really seems like a micro-optimisation.

small streams become big rivers (i don't know the idiomatic sentence in 
English, so it's a rough translation from a French idiom),
currently the main problem is that the loading of modules adds so much overhead 
to the startup time that you see nothing :(

Also, startup time is one aspect, size of the VM data structures at runtime, 
size of the CDS archive* are also impacted.

> 
> 
> > ARFU are not a replacement of Unsafe, VarHandles are. So adding a new usage
> > of Unsafe for a good reason goes in the right direction here,
> > when VarHandle will replace usages of Unsafe, the code of
> > BufferedInputStream will be refactored again.
> > 
> 
> My preference is not to change it if it’s gonna change later on.

but there is also a good chance to miss that change later because you will look 
for usages of Unsafe and not usages of ARFU.
Replacing ARFU by Unsafe in BufferedInputStream makes the code more similar to 
the other usages of Unsafe in the JDK.

[...]

> 
> Paul.
> 

Rémi

* removing classes from a CDS archive is important for 32 bits windows VM

Rémi

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Paul Sandoz]

> On 6 Apr 2016, at 12:10, Remi Forax <fo...@univ-mlv.fr> wrote:
> 
> - Mail original -
>> De: "Paul Sandoz" <paul.san...@oracle.com>
>> Cc: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net>
>> Envoyé: Mercredi 6 Avril 2016 09:37:00
>> Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of 
>> AtomicReferenceFieldUpdater with Unsafe
>> 
>> 
>>> On 5 Apr 2016, at 17:15, Claes Redestad <claes.redes...@oracle.com> wrote:
>>> 
>>> On 04/04/2016 05:45 PM, Martin Buchholz wrote:
>>>>> I think this one is going too far.
>>>>> 
>>>>> A*FU/VarHandles should are supposed to act like a go-to replacement for
>>>>> Unsafe throughout the class library, and we want to shrink the Unsafe
>>>>> exposure. Also, I don't think removing A*FU in favor of Unsafe here wins
>>>>> us anything: there should be no throughput hit, and we *will* load A*FU
>>>>> down the road anyway, negating the startup wins.
>> 
>> +1 i would leave this one as is.
>> 
>> In general same goes for the @Stable/ForceInline annotations etc. We should
>> use this stuff carefully within java.base and also sparingly to qualifying
>> JDK modules.
> 
> While i fully agree in the general case, in disagree for this specific case,
> there are few uses of ARFU inside the JDK (or outside) but currently because 
> BufferedInputStream uses an ARFU, each time someone starts a Java 
> application, the VM loads ARFU and its implementation with a high probability 
> to no need it at all after
> 

Does that really contribute sufficiently to slow down in start time? It really 
seems like a micro-optimisation.


> ARFU are not a replacement of Unsafe, VarHandles are. So adding a new usage 
> of Unsafe for a good reason goes in the right direction here,
> when VarHandle will replace usages of Unsafe, the code of BufferedInputStream 
> will be refactored again.
> 

My preference is not to change it if it’s gonna change later on.


>> 
>>>>> 
>>>>> -Aleksey
>>>> It is surprising to see new uses of Unsafe when we have an ongoing
>>>> initiative within openjdk (especially from Paul Sandoz) to remove most
>>>> uses.  Varhandles are coming and are expected to replace uses of
>>>> Unsafe in the JDK.
>>> 
>>> This is just a very minor win on hello world/-version style tests, so I'm
>>> happy to withdraw this if other early usages, such as CHM, is moving to
>>> VarHandles anyhow.
>>> 
>>> OTOH using dangerous, internal APIs like this rather than nice, public APIs
>>> early in the VM bootstrap has other merits, such as not unintentionally
>>> causing bootstrap issues. Say, I don't know if VarHandles have any
>>> dependencies on java.lang.invoke currently…
>> 
>> It does, but was designed to be minimize bootstrap issues and class spinning
>> so there are less dependencies than MHs.
>> 
>> CHM is a tricky class because MethodType uses CHM and VHs uses MethodType.
>> There is probably a way of switching from a less concurrent concurrent map
>> to CHM at “safe-point” when the VM transitions to booted. To be
>> investigated...
> 
> another solution is perhaps to not use a concurrent hashmap in MethodType, 
> method types need to be interned only the first time a method handle is 
> invoked with an invokeExact/invoke (but not by invokedynamic) so instead of 
> interning all method types which artificially put pressure on the interning 
> map to be lock free, interning only the method type when need it may allow to 
> not use a CHM in MethodType.
> 

I need to re-familiarise myself with the code to see what is possible here. 
MethodType also comes with a shared erased form which also caches LambdaForms 
for various kinds. It’s not clear to me if this can be easily be teased apart.

Paul.

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Remi Forax]

- Mail original -
> De: "Paul Sandoz" <paul.san...@oracle.com>
> Cc: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net>
> Envoyé: Mercredi 6 Avril 2016 09:37:00
> Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of  
> AtomicReferenceFieldUpdater with Unsafe
> 
> 
> > On 5 Apr 2016, at 17:15, Claes Redestad <claes.redes...@oracle.com> wrote:
> > 
> > On 04/04/2016 05:45 PM, Martin Buchholz wrote:
> >>> I think this one is going too far.
> >>> 
> >>> A*FU/VarHandles should are supposed to act like a go-to replacement for
> >>> Unsafe throughout the class library, and we want to shrink the Unsafe
> >>> exposure. Also, I don't think removing A*FU in favor of Unsafe here wins
> >>> us anything: there should be no throughput hit, and we *will* load A*FU
> >>> down the road anyway, negating the startup wins.
> 
> +1 i would leave this one as is.
> 
> In general same goes for the @Stable/ForceInline annotations etc. We should
> use this stuff carefully within java.base and also sparingly to qualifying
> JDK modules.

While i fully agree in the general case, in disagree for this specific case,
there are few uses of ARFU inside the JDK (or outside) but currently because 
BufferedInputStream uses an ARFU, each time someone starts a Java application, 
the VM loads ARFU and its implementation with a high probability to no need it 
at all after

ARFU are not a replacement of Unsafe, VarHandles are. So adding a new usage of 
Unsafe for a good reason goes in the right direction here,
when VarHandle will replace usages of Unsafe, the code of BufferedInputStream 
will be refactored again.

> 
> >>> 
> >>> -Aleksey
> >> It is surprising to see new uses of Unsafe when we have an ongoing
> >> initiative within openjdk (especially from Paul Sandoz) to remove most
> >> uses.  Varhandles are coming and are expected to replace uses of
> >> Unsafe in the JDK.
> > 
> > This is just a very minor win on hello world/-version style tests, so I'm
> > happy to withdraw this if other early usages, such as CHM, is moving to
> > VarHandles anyhow.
> > 
> > OTOH using dangerous, internal APIs like this rather than nice, public APIs
> > early in the VM bootstrap has other merits, such as not unintentionally
> > causing bootstrap issues. Say, I don't know if VarHandles have any
> > dependencies on java.lang.invoke currently…
> 
> It does, but was designed to be minimize bootstrap issues and class spinning
> so there are less dependencies than MHs.
> 
> CHM is a tricky class because MethodType uses CHM and VHs uses MethodType.
> There is probably a way of switching from a less concurrent concurrent map
> to CHM at “safe-point” when the VM transitions to booted. To be
> investigated...

another solution is perhaps to not use a concurrent hashmap in MethodType, 
method types need to be interned only the first time a method handle is invoked 
with an invokeExact/invoke (but not by invokedynamic) so instead of interning 
all method types which artificially put pressure on the interning map to be 
lock free, interning only the method type when need it may allow to not use a 
CHM in MethodType.

> 
> Paul.
> 

Rémi

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Paul Sandoz]

> On 5 Apr 2016, at 17:15, Claes Redestad  wrote:
> 
> On 04/04/2016 05:45 PM, Martin Buchholz wrote:
>>> I think this one is going too far.
>>> 
>>> A*FU/VarHandles should are supposed to act like a go-to replacement for
>>> Unsafe throughout the class library, and we want to shrink the Unsafe
>>> exposure. Also, I don't think removing A*FU in favor of Unsafe here wins
>>> us anything: there should be no throughput hit, and we *will* load A*FU
>>> down the road anyway, negating the startup wins.

+1 i would leave this one as is.

In general same goes for the @Stable/ForceInline annotations etc. We should use 
this stuff carefully within java.base and also sparingly to qualifying JDK 
modules.

>>> 
>>> -Aleksey
>> It is surprising to see new uses of Unsafe when we have an ongoing
>> initiative within openjdk (especially from Paul Sandoz) to remove most
>> uses.  Varhandles are coming and are expected to replace uses of
>> Unsafe in the JDK.
> 
> This is just a very minor win on hello world/-version style tests, so I'm 
> happy to withdraw this if other early usages, such as CHM, is moving to 
> VarHandles anyhow.
> 
> OTOH using dangerous, internal APIs like this rather than nice, public APIs 
> early in the VM bootstrap has other merits, such as not unintentionally 
> causing bootstrap issues. Say, I don't know if VarHandles have any 
> dependencies on java.lang.invoke currently…

It does, but was designed to be minimize bootstrap issues and class spinning so 
there are less dependencies than MHs.

CHM is a tricky class because MethodType uses CHM and VHs uses MethodType. 
There is probably a way of switching from a less concurrent concurrent map to 
CHM at “safe-point” when the VM transitions to booted. To be investigated...

Paul.

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Claes Redestad]

On 04/04/2016 05:45 PM, Martin Buchholz wrote:

I think this one is going too far.

A*FU/VarHandles should are supposed to act like a go-to replacement for
Unsafe throughout the class library, and we want to shrink the Unsafe
exposure. Also, I don't think removing A*FU in favor of Unsafe here wins
us anything: there should be no throughput hit, and we*will*  load A*FU
down the road anyway, negating the startup wins.

-Aleksey

It is surprising to see new uses of Unsafe when we have an ongoing
initiative within openjdk (especially from Paul Sandoz) to remove most
uses.  Varhandles are coming and are expected to replace uses of
Unsafe in the JDK.


This is just a very minor win on hello world/-version style tests, so 
I'm happy to withdraw this if other early usages, such as CHM, is moving 
to VarHandles anyhow.


OTOH using dangerous, internal APIs like this rather than nice, public 
APIs early in the VM bootstrap has other merits, such as not 
unintentionally causing bootstrap issues. Say, I don't know if 
VarHandles have any dependencies on java.lang.invoke currently...


/Claes

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Aleksey Shipilev]

On 04/03/2016 03:51 AM, Claes Redestad wrote:
> BufferedInputStream is loaded early, and uses 
> AtomicReferenceFieldUpdater to provide CAS functionality to allow for
> closing streams asynchronously. Using Unsafe directly instead does
> the exact same thing in the end, but avoids loading a few (4) classes
> and thus brings us a small startup improvement.

> Bug: https://bugs.openjdk.java.net/browse/JDK-8153334
> Webrev: http://cr.openjdk.java.net/~redestad/8153334/webrev.01/

I think this one is going too far.

A*FU/VarHandles should are supposed to act like a go-to replacement for
Unsafe throughout the class library, and we want to shrink the Unsafe
exposure. Also, I don't think removing A*FU in favor of Unsafe here wins
us anything: there should be no throughput hit, and we *will* load A*FU
down the road anyway, negating the startup wins.

-Aleksey

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Chris Hegarty]

On 03/04/16 21:39, Claes Redestad wrote:






Thanks for looking at this, Alan!

Cleaned things up here:
http://cr.openjdk.java.net/~redestad/8153334/webrev.02/


Looks good Claes.

-Chris.

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Claes Redestad]

On 04/03/2016 04:55 PM, Alan Bateman wrote:



On 03/04/2016 13:19, Claes Redestad wrote:

Hi Remi,

On 2016-04-03 13:57, Remi Forax wrote:

Hi Claes,
the patch is fine for me with the minor nitpick that the static 
final containing Unsafe should be called UNSAFE and not just U.


sure, I copied the setup/naming convention from ConcurrentHashMap, 
but UNSAFE does make it stand out better.




do you know why BufferedInputStream is loaded in first place during 
the startup of the VM ?


given the output of -Xlog:classload I think it's first used by 
java.lang.System for what becomes System.in.


Yes, initPhase1 (or what used to be initializeSystemClass) will be the 
first usage.


I skimmed the webrev, looks okay to me. I think I would import 
jdk.internal.misc.Unsafe rather than using the fully qualified class 
name twice. That will also avoid the line break.


-Alan



Thanks for looking at this, Alan!

Cleaned things up here: 
http://cr.openjdk.java.net/~redestad/8153334/webrev.02/


/Claes

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Alan Bateman]

On 03/04/2016 13:19, Claes Redestad wrote:

Hi Remi,

On 2016-04-03 13:57, Remi Forax wrote:

Hi Claes,
the patch is fine for me with the minor nitpick that the static final 
containing Unsafe should be called UNSAFE and not just U.


sure, I copied the setup/naming convention from ConcurrentHashMap, but 
UNSAFE does make it stand out better.




do you know why BufferedInputStream is loaded in first place during 
the startup of the VM ?


given the output of -Xlog:classload I think it's first used by 
java.lang.System for what becomes System.in.


Yes, initPhase1 (or what used to be initializeSystemClass) will be the 
first usage.


I skimmed the webrev, looks okay to me. I think I would import 
jdk.internal.misc.Unsafe rather than using the fully qualified class 
name twice. That will also avoid the line break.


-Alan

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[forax]

- Mail original -
> De: "Claes Redestad" <claes.redes...@oracle.com>
> À: "Remi Forax" <fo...@univ-mlv.fr>
> Cc: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net>
> Envoyé: Dimanche 3 Avril 2016 14:19:52
> Objet: Re: RFR: 8153334: Replace BufferedInputStreams use of 
> AtomicReferenceFieldUpdater with Unsafe
> 
> Hi Remi,
> 
> On 2016-04-03 13:57, Remi Forax wrote:
> > Hi Claes,
> > the patch is fine for me with the minor nitpick that the static final
> > containing Unsafe should be called UNSAFE and not just U.
> 
> sure, I copied the setup/naming convention from ConcurrentHashMap, but
> UNSAFE does make it stand out better.
> 
> >
> > do you know why BufferedInputStream is loaded in first place during the
> > startup of the VM ?
> 
> given the output of -Xlog:classload I think it's first used by
> java.lang.System for what becomes System.in.

oh, i see :)

> 
> Thanks!
> 
> /Claes

cheers,
Rémi

> 
> >
> > regards,
> > Rémi
> >
> > - Mail original -----
> >> De: "Claes Redestad" <claes.redes...@oracle.com>
> >> À: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net>
> >> Envoyé: Dimanche 3 Avril 2016 02:51:35
> >> Objet: RFR: 8153334: Replace BufferedInputStreams use of
> >>AtomicReferenceFieldUpdater with Unsafe
> >>
> >> Hi,
> >>
> >> BufferedInputStream is loaded early, and uses
> >> AtomicReferenceFieldUpdater to provide
> >> CAS functionality to allow for closing streams asynchronously. Using
> >> Unsafe directly instead
> >> does the exact same thing in the end, but avoids loading a few (4)
> >> classes and thus brings
> >> us a small startup improvement.
> >>
> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8153334
> >> Webrev: http://cr.openjdk.java.net/~redestad/8153334/webrev.01/
> >>
> >> Thanks!
> >>
> >> /Claes
> >>
> 
> 

Re: RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Claes Redestad]

Hi Remi,

On 2016-04-03 13:57, Remi Forax wrote:

Hi Claes,
the patch is fine for me with the minor nitpick that the static final 
containing Unsafe should be called UNSAFE and not just U.


sure, I copied the setup/naming convention from ConcurrentHashMap, but 
UNSAFE does make it stand out better.




do you know why BufferedInputStream is loaded in first place during the startup 
of the VM ?


given the output of -Xlog:classload I think it's first used by 
java.lang.System for what becomes System.in.


Thanks!

/Claes



regards,
Rémi

- Mail original -

De: "Claes Redestad" <claes.redes...@oracle.com>
À: "core-libs-dev Libs" <core-libs-dev@openjdk.java.net>
Envoyé: Dimanche 3 Avril 2016 02:51:35
Objet: RFR: 8153334: Replace BufferedInputStreams use of
AtomicReferenceFieldUpdater with Unsafe

Hi,

BufferedInputStream is loaded early, and uses
AtomicReferenceFieldUpdater to provide
CAS functionality to allow for closing streams asynchronously. Using
Unsafe directly instead
does the exact same thing in the end, but avoids loading a few (4)
classes and thus brings
us a small startup improvement.

Bug: https://bugs.openjdk.java.net/browse/JDK-8153334
Webrev: http://cr.openjdk.java.net/~redestad/8153334/webrev.01/

Thanks!

/Claes

RFR: 8153334: Replace BufferedInputStreams use of AtomicReferenceFieldUpdater with Unsafe

[Claes Redestad]

Hi,

BufferedInputStream is loaded early, and uses 
AtomicReferenceFieldUpdater to provide
CAS functionality to allow for closing streams asynchronously. Using 
Unsafe directly instead
does the exact same thing in the end, but avoids loading a few (4) 
classes and thus brings

us a small startup improvement.

Bug: https://bugs.openjdk.java.net/browse/JDK-8153334
Webrev: http://cr.openjdk.java.net/~redestad/8153334/webrev.01/

Thanks!

/Claes