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

[Claes Redestad]

Thanks for all the reviews! Pushed.

/Claes

On 2015-12-21 11:06, Chris Hegarty wrote:
> Thanks for doing this Claes. The changes look good to me.
>
> -Chris.
>
> On 20/12/15 23:06, Claes Redestad wrote:
>> 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
>>>>>
>>