[PATCH] remove redundant initialization of volatile fields with default values

[Peter Levart]

Hi, Sergey,


On 6/19/20 11:33 AM, Сергей Цыпанов wrote:
> Hello Remi,
>
> thanks for pointing this out, I didn't take this into account.
>
> As I understand, the volatile semantics here covers all the fields of an object,
> no matter whether they are declared before or after volatile field (probably
> actual object layout might be different from declaration in source code).

The layout of the fields doesn't matter. The order of assignment and the 
order of reading and volatileness/finallness of fields matter.


>
> If that's true I think we still can apply this optimization at least for
>
> 1) classes with single field and without super-classes


I think that even removing assignment of default value to a field in a 
singular-field class (either volatile or not) in constructor can be 
observed. But I don't believe any program is relying on the observed 
behavior of such assignment - quite the contrary this might be a hidden bug.


> 2) classes with all non-volatile fields declared final


Final fields have to be explicitly assigned in the constructor even if 
they need to contain default value(s). But they have special JMM 
semantics and such assignment is not problematic.


> 3) classes with not-initialized non-volatile fields


This is similar to point 1). The additional non-explicitly assigned 
non-volatile fields don't matter here.


> 4) classes with super-classes where non-volatile fields are not initialized (or inialized with default values)


Where fields are declared (in super class or in sub class) does not 
matter. At runtime we are dealing with a single object. So if both 
non-volatile fields and volatile fields are assigned it depends on the 
order of assignments and on the order of reading those fields in the 
"whole program" whether removing volatile assignment of default value 
can have an effect on the program semantics. But as said, relying on the 
effects of the default value volatile write in constructor is fragile as 
it is not guaranteed by JMM. So such writes need to be studied and 
eventually removed.


>
> Looking at source code I see that we could keep java.security.KeyStore and


KeyStore does not explicitly assign non-volatile non-final fields in 
constructor so the assignment of false to a volatile 'initialized' field 
in constructor can safely be removed.


> its nested class PasswordProtection along with java.util.ListResourceBundle.


PasswordProtection has just final fields and one volatile field 
'destroyed' which is explicitly assigned to 'false' in constructor. This 
assignment can safely be removed.


ListResourceBundle is more complicated and would need to be studied more 
carefully.


Regards, Peter


>
> Please correct me if I miss anything in my speculation.
>
> Regards,
> Sergey Tsypanov
>
> 19.06.2020, 10:04, "Remi Forax" <forax at univ-mlv.fr>:
>> Hi Sergei,
>> the problem is that you are changing the semantics if there are several fields.
>>
>> By example with the code below, you have the guarantee that the code will print 4 (if it prints something),
>> if you remove the assignment field = false, the code can print 0 or 4.
>>
>>    class A {
>>      int i = 4;
>>      volatile boolean field = false;
>>    }
>>
>> thread 1:
>>    global = new A()
>>
>> thread 2:
>>    var a = global;
>>    if (a != null) {
>>      System.out.println(a.i);
>>    }
>>
>> regards,
>> Rémi
>>
>> ----- Mail original -----
>>>   De: "Сергей Цыпанов" <sergei.tsypanov at yandex.ru>
>>>   À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>>>   Envoyé: Vendredi 19 Juin 2020 06:57:25
>>>   Objet: [PATCH] remove redundant initialization of volatile fields with default values
>>>   Hello,
>>>
>>>   while investigating an issue I've found out that assignment of default value to
>>>   volatile fields slows down object instantiation.
>>>
>>>   Consider the benchmark:
>>>
>>>   @State(Scope.Thread)
>>>   @OutputTimeUnit(TimeUnit.NANOSECONDS)
>>>   @BenchmarkMode(value = Mode.AverageTime)
>>>   @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>>>   public class VolatileFieldBenchmark {
>>>    @Benchmark
>>>    public Object explicitInit() {
>>>      return new ExplicitInit();
>>>    }
>>>
>>>    @Benchmark
>>>    public Object noInit() {
>>>      return new NoInit();
>>>    }
>>>
>>>    private static class ExplicitInit {
>>>      private volatile boolean field = false;
>>>    }
>>>    private static class NoInit {
>>>      private volatile boolean field;
>>>    }
>>>   }
>>>
>>>   This gives the following results as of my machine:
>>>
>>>   Benchmark Mode Cnt Score Error Units
>>>   VolatileFieldBenchmark.explicitInit avgt 40 11.087 ± 0.140 ns/op
>>>   VolatileFieldBenchmark.noInit avgt 40 3.367 ± 0.131 ns/op
>>>
>>>   I've looked into source code of java.base and found out several cases where the
>>>   default value is assigned to volatile field.
>>>
>>>   Getting rid of such assignements demonstates improvement as of object
>>>   instantiation, e.g. javax.security.auth.Subject:
>>>
>>>             Mode Cnt Score Error Units
>>>   before avgt 40 35.933 ± 2.647 ns/op
>>>   after avgt 40 30.817 ± 2.384 ns/op
>>>
>>>   As of testing tier1 and tier2 are both ok after the changes.
>>>
>>>   Best regards,
>>>   Sergey Tsypanov