Re: [PATCH] remove redundant initialization of volatile fields with default values
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" :
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: "Сергей Цыпанов"
À: "core-libs-dev"
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
Re: [PATCH] remove redundant initialization of volatile fields with default values
On 19/06/2020 11:25 pm, David Holmes wrote:
Hi Remi,
On 19/06/2020 6:03 pm, Remi Forax wrote:
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);
}
Surely you intended to read field to get the volatile read to pair with
the volatile write of the initializer? Otherwise there is no
happens-before ordering.
And as Peter points out that may not suffice as you can't distinguish
the default initialization of field with the actual volatile write in
the initializer.
David
David
regards,
Rémi
- Mail original -
De: "Сергей Цыпанов"
À: "core-libs-dev"
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
Re: [PATCH] remove redundant initialization of volatile fields with default values
Hi Remi,
On 19/06/2020 6:03 pm, Remi Forax wrote:
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);
}
Surely you intended to read field to get the volatile read to pair with
the volatile write of the initializer? Otherwise there is no
happens-before ordering.
David
regards,
Rémi
- Mail original -
De: "Сергей Цыпанов"
À: "core-libs-dev"
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:
BenchmarkMode Cnt Score Error Units
VolatileFieldBenchmark.explicitInit avgt 40 11.087 ± 0.140 ns/op
VolatileFieldBenchmark.noInitavgt 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
Re: [PATCH] remove redundant initialization of volatile fields with default values
Hi Remi,
On 6/19/20 10:03 AM, Remi Forax wrote:
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);
}
I don't think this is guaranteed by the JMM. Unless you also read the
'field' in thread 2 and observe the effects of write to 'field' before
reading 'i' :
thread 2:
var a = global;
if (a != null && !a.field) {
System.out.println(a.i);
}
And even that might not work as you have to "observe" the effects of
write to 'field' before reading 'i' and if the value of 'field' didn't
change by the write, nothing guarantees that you have observed the
effects of write to 'field' if you read false from it. So we could argue
that any program that relies on similar unexistent guarantees is wrong,
but works because the implementation usually exhibits stronger semantics
than required. So removing such volatile write might break some programs
in practice that are already broken in theory.
Anyway, assigning default value to a field in constructor is always sign
of bed smell and it is almost always possible to restructure code so
that it is not needed. You have to be careful and see the "whole
program" since other places might have to be modified too to preserve
the semantics.
Regards, Peter
regards,
Rémi
- Mail original -
De: "Сергей Цыпанов"
À: "core-libs-dev"
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:
BenchmarkMode Cnt Score Error Units
VolatileFieldBenchmark.explicitInit avgt 40 11.087 ± 0.140 ns/op
VolatileFieldBenchmark.noInitavgt 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
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" :
> 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: "Сергей Цыпанов"
>> À: "core-libs-dev"
>> 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 Tsypanovdiff --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 lookup = null;
+private volatile Map lookup;
}
Re: [PATCH] remove redundant initialization of volatile fields with default values
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: "Сергей Цыпанов"
> À: "core-libs-dev"
> 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:
>
> BenchmarkMode Cnt Score Error Units
> VolatileFieldBenchmark.explicitInit avgt 40 11.087 ± 0.140 ns/op
> VolatileFieldBenchmark.noInitavgt 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
Re: [PATCH] remove redundant initialization of volatile fields with default values
On 19/06/2020 05:57, Сергей Цыпанов wrote: Hello, while investigating an issue I've found out that assignment of default value to volatile fields slows down object instantiation. Yes, there's been several efforts over the years to eliminate the initializing volatile fields to their default values. It's good to keep these in check. It's probably best to get a sponsor on the security-libs list as all by one of these in the security code. -Alan
[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:
BenchmarkMode Cnt Score Error Units
VolatileFieldBenchmark.explicitInit avgt 40 11.087 ± 0.140 ns/op
VolatileFieldBenchmark.noInitavgt 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 Tsypanovdiff --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 lookup = null;
+private volatile Map lookup;
}
diff --git a/src/java.base/share/classes/javax/security/auth/Subject.java b/src/java.base/share/classes/javax/security/auth/Subject.java
--- a/src/java.base/share/classes/javax/security/auth/Subject.java
+++ b/src/java.base/share/classes/javax/security/auth/Subject.java
@@ -126,7 +126,7 @@
*
* @serial
*/
-private volatile boolean readOnly = false;
+private volatile boolean readOnly;
private static final int PRINCIPAL_SET = 1;
private static final int PUB_CREDENTIAL_SET = 2;
diff --git a/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java b/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java
--- a/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java
+++ b/src/java.base/share/classes/sun/security/provider/AbstractDrbg.java
@@ -71,7 +71,7 @@
// Common working status
-private boolean instantiated = false;
+private boolean instantiated;
/**
* Reseed counter of a DRBG instance. A mechanism should increment it
@@ -80,7 +80,7 @@
*
* Volatile, will be used in a double checked locking.
*/
-protected volatile int reseedCounter = 0;
+protected volatile int reseedCounter;
// Mech features. If not same as below, must be redefined in constructor.
diff --git a/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java b/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java
--- a/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java
+++ b/src/java.base/share/classes/sun/security/ssl/DTLSOutputRecord.java
@@ -36,7 +36,7 @@
*/
final class DTLSOutputRecord extends OutputRecord implements DTLSRecord {
-private DTLSFragmenter fragmenter = null;
+private DTLSFragmenter fragmenter;
int writeEpoch;
@@ -44,7 +44,7 @@
Authenticator prevWriteAuthenticator;
SSLWriteCipher prevWriteCipher;
-private volatile boolean isCloseWaiting = false;
+private volatile boolean isCloseWaiting;
DTLSOutputRecord(HandshakeHash handshakeHash) {
super(handshakeHash, SSLWriteCipher.nullDTlsWriteCipher());
diff --git a/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java b/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java
---
