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

[Сергей Цыпанов]

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).

If that's true I think we still can apply this optimization at least for

1) classes with single field and without super-classes
2) classes with all non-volatile fields declared final
3) classes with not-initialized non-volatile fields
4) classes with super-classes where non-volatile fields are not initialized (or 
inialized with default values)

Looking at source code I see that we could keep java.security.KeyStore and 
its nested class PasswordProtection along with java.util.ListResourceBundle.

Please correct me if I miss anything in my speculation.

Regards,
Sergey Tsypanov

19.06.2020, 10:04, "Remi Forax" <fo...@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.tsypa...@yandex.ru>
>>  À: "core-libs-dev" <core-libs-dev@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
diff --git a/src/java.base/share/classes/java/security/KeyStore.java b/src/java.base/share/classes/java/security/KeyStore.java
--- a/src/java.base/share/classes/java/security/KeyStore.java
+++ b/src/java.base/share/classes/java/security/KeyStore.java
@@ -219,7 +219,7 @@
     private KeyStoreSpi keyStoreSpi;
 
     // Has this keystore been initialized (loaded)?
-    private boolean initialized = false;
+    private boolean initialized;
 
     /**
      * A marker interface for {@code KeyStore}
@@ -264,7 +264,7 @@
         private final char[] password;
         private final String protectionAlgorithm;
         private final AlgorithmParameterSpec protectionParameters;
-        private volatile boolean destroyed = false;
+        private volatile boolean destroyed;
 
         /**
          * Creates a password parameter.
diff --git a/src/java.base/share/classes/java/util/ListResourceBundle.java b/src/java.base/share/classes/java/util/ListResourceBundle.java
--- a/src/java.base/share/classes/java/util/ListResourceBundle.java
+++ b/src/java.base/share/classes/java/util/ListResourceBundle.java
@@ -206,5 +206,5 @@
         lookup = temp;
     }
 
-    private volatile Map<String,Object> lookup = null;
+    private volatile Map<String,Object> lookup;
 }