RFR: 8145680: Remove unnecessary explicit initialization of volatile variables in java.base

[Claes Redestad]

Hi all,

On 2015-12-20 20:43, Ulf wrote:
> Hi Claes,
>
> I had a very short look and found in j.l.Thread:
>  211      * Java thread status for tools, 0 indicate thread 'not yet 
> started'
> I guess you meant:
>  211      * Java thread status for tools; 0 indicates: thread 'not yet 
> started'.
>
> -Ulf 

     /*
      * Java thread status for tools, default indicates thread 'not yet started'
      */

OK?

On 2015-12-20 21:02, Jason Mehrens wrote:
> Claes,
>
> For the cases where boolean was being assigned to 'true' (ASSCI and FileLockImpl) does it hurt performance since the accessor methods will now include a branch at the bytecode level?  See: "Speed-kings of inverting booleans" at  http://www.javaspecialists.eu/archive/Issue042.html
>
> Jason

I don't think this is an issue any more:

     public volatile boolean trueValue = true;
     public volatile boolean falseValue = false;

     @Benchmark
     @CompilerControl(CompilerControl.Mode.DONT_INLINE)
     public boolean trueWithFalseValue() throws Exception {
         return !falseValue;
     }

     @Benchmark
     @CompilerControl(CompilerControl.Mode.DONT_INLINE)
     public boolean trueWithTrueValue() throws Exception {
         return trueValue;
     }

DefaultInitBench.trueWithFalseValue  avgt   25  4.538 ± 0.353  ns/op
DefaultInitBench.trueWithTrueValue   avgt   25  4.726 ± 0.466  ns/op

On 2015-12-20 20:33, Joel Borggrén-Franck wrote:
> Hi Claes,
>
> src/java.base/share/classes/java/lang/reflect/Parameter.java
> src/java.base/share/classes/sun/reflect/MethodAccessorGenerator.java
> src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java
>
> looks fine to me.
>
> src/java.base/share/classes/java/lang/Class.java
>
> is fine to but isn't
>
> enumConstants = constants = temporatyConstants;
>
> slightly more idiomatic than
>
> -                enumConstants = temporaryConstants;
> +                constants = temporaryConstants;
> +                enumConstants = constants;

I'm fine either way; updated in-place in this and a few other places.

Thanks!

/Claes

> On Sun, Dec 20, 2015 at 6:29 PM, Claes Redestad
> <claes.redestad at oracle.com> wrote:
>> Hi,
>>
>> the changes to java.net.URI stood out as a bit too intrusive for a cleanup
>> like this, and there's precious little measurable benefit. I decided to
>> break out those to a separate RFE and simplified this patch:
>>
>> Webrev: http://cr.openjdk.java.net/~redestad/8145680/webrev.02
>>
>> /Claes
>>
>>
>> On 2015-12-19 14:07, Claes Redestad wrote:
>>> Hi,
>>>
>>> initializing volatile fields to their default value has a well-known
>>> performance penalty, and removing these should typically be safe. This patch
>>> addresses java.base.
>>>
>>> There are however some corner cases that we need to check for, see
>>> examples and discussion in
>>> http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html
>>>
>>> When meticulously going through and checking each usage for odd pattern
>>> like this I accidentally did a bit of extra cleanup, mostly addressing a
>>> number of cases where the volatile field was being read twice. Sorry!
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8145680
>>>