Re: [PATCH] remove redundant initialization of volatile fields with default values
Cool, thanks! Do you know anyone who could sponsor this and create a web-review against the patch? Regards, Sergeyt Tsypanov 13.08.2020, 19:22, "Sean Mullan" : > On 8/13/20 9:04 AM, Сергей Цыпанов wrote: >> Hi, >> >> I don't have account in JBS, so I cannot file an issue. >> >> Previously when I submitted patches via core-libs-dev mailing list >> previleged users >> filed the issues and created web-reviews. >> >> I think this should be a subtask of >> https://bugs.openjdk.java.net/browse/JDK-6736490, there's >> already one I've mentioned in previous mail: >> https://bugs.openjdk.java.net/browse/JDK-8145680 > > Done: see https://bugs.openjdk.java.net/browse/JDK-8251548 > > --Sean > >> Regards, >> Sergey Tsypanov >> >> 13.08.2020, 14:05, "Sean Mullan" : >>> On 8/13/20 7:04 AM, Сергей Цыпанов wrote: Hello, previously I've sent an email regarding removal of redundant assignments if default values to volatile fields, see https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html There was a concern whether it's completely safe to remove those assignments from JMM point of view, see https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html Recently I've found a thread in concurrency-interest mailing list where Aleksey Shiplive tried to find a constraint agians such removal: https://urldefense.com/v3/__http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html__;!!GqivPVa7Brio!I4TMi9HPzckS0_w9Qmgw0-kGArRRuctFvBSnpthDRPaGGqgvl9yyrjVHboPdHMd6$ It appears that there are no constraitns and Doug Lea mentions in https://urldefense.com/v3/__http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html__;!!GqivPVa7Brio!I4TMi9HPzckS0_w9Qmgw0-kGArRRuctFvBSnpthDRPaGGqgvl9yyrjVHbvX4nrL2$ that "there is never any reason to explicitly initialize fields to 0/0.0/false/null" Also there we similar code changes in java.base before: - https://bugs.openjdk.java.net/browse/JDK-6736490 - https://bugs.openjdk.java.net/browse/JDK-8035284 - https://bugs.openjdk.java.net/browse/JDK-8145680 So I think now we can accept the patch as the changes appear to be safe. >>> >>> Ok, it seems like a good change. Are you able to file a JBS issue for >>> this? After that you can request a formal code review. >>> >>> Thanks, >>> Sean
Re: [PATCH] remove redundant initialization of volatile fields with default values
On 8/13/20 9:04 AM, Сергей Цыпанов wrote: Hi, I don't have account in JBS, so I cannot file an issue. Previously when I submitted patches via core-libs-dev mailing list previleged users filed the issues and created web-reviews. I think this should be a subtask of https://bugs.openjdk.java.net/browse/JDK-6736490, there's already one I've mentioned in previous mail: https://bugs.openjdk.java.net/browse/JDK-8145680 Done: see https://bugs.openjdk.java.net/browse/JDK-8251548 --Sean Regards, Sergey Tsypanov 13.08.2020, 14:05, "Sean Mullan" : On 8/13/20 7:04 AM, Сергей Цыпанов wrote: Hello, previously I've sent an email regarding removal of redundant assignments if default values to volatile fields, see https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html There was a concern whether it's completely safe to remove those assignments from JMM point of view, see https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html Recently I've found a thread in concurrency-interest mailing list where Aleksey Shiplive tried to find a constraint agians such removal: https://urldefense.com/v3/__http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html__;!!GqivPVa7Brio!I4TMi9HPzckS0_w9Qmgw0-kGArRRuctFvBSnpthDRPaGGqgvl9yyrjVHboPdHMd6$ It appears that there are no constraitns and Doug Lea mentions in https://urldefense.com/v3/__http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html__;!!GqivPVa7Brio!I4TMi9HPzckS0_w9Qmgw0-kGArRRuctFvBSnpthDRPaGGqgvl9yyrjVHbvX4nrL2$ that "there is never any reason to explicitly initialize fields to 0/0.0/false/null" Also there we similar code changes in java.base before: - https://bugs.openjdk.java.net/browse/JDK-6736490 - https://bugs.openjdk.java.net/browse/JDK-8035284 - https://bugs.openjdk.java.net/browse/JDK-8145680 So I think now we can accept the patch as the changes appear to be safe. Ok, it seems like a good change. Are you able to file a JBS issue for this? After that you can request a formal code review. Thanks, Sean
Re: [PATCH] remove redundant initialization of volatile fields with default values
Hi, I don't have account in JBS, so I cannot file an issue. Previously when I submitted patches via core-libs-dev mailing list previleged users filed the issues and created web-reviews. I think this should be a subtask of https://bugs.openjdk.java.net/browse/JDK-6736490, there's already one I've mentioned in previous mail: https://bugs.openjdk.java.net/browse/JDK-8145680 Regards, Sergey Tsypanov 13.08.2020, 14:05, "Sean Mullan" : > On 8/13/20 7:04 AM, Сергей Цыпанов wrote: >> Hello, >> >> previously I've sent an email regarding removal of redundant assignments if >> default values to volatile fields, see >> https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html >> >> There was a concern whether it's completely safe to remove those >> assignments from JMM point of view, see >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html >> >> Recently I've found a thread in concurrency-interest mailing list where >> Aleksey Shiplive tried to find a constraint >> agians such removal: >> http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html >> >> It appears that there are no constraitns and Doug Lea mentions in >> >> http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html >> that "there is never any reason to explicitly initialize fields to >> 0/0.0/false/null" >> >> Also there we similar code changes in java.base before: >> >> - https://bugs.openjdk.java.net/browse/JDK-6736490 >> - https://bugs.openjdk.java.net/browse/JDK-8035284 >> - https://bugs.openjdk.java.net/browse/JDK-8145680 >> >> So I think now we can accept the patch as the changes appear to be safe. > > Ok, it seems like a good change. Are you able to file a JBS issue for > this? After that you can request a formal code review. > > Thanks, > Sean
Re: [PATCH] remove redundant initialization of volatile fields with default values
On 8/13/20 7:04 AM, Сергей Цыпанов wrote: Hello, previously I've sent an email regarding removal of redundant assignments if default values to volatile fields, see https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html There was a concern whether it's completely safe to remove those assignments from JMM point of view, see https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html Recently I've found a thread in concurrency-interest mailing list where Aleksey Shiplive tried to find a constraint agians such removal: http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html It appears that there are no constraitns and Doug Lea mentions in http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html that "there is never any reason to explicitly initialize fields to 0/0.0/false/null" Also there we similar code changes in java.base before: - https://bugs.openjdk.java.net/browse/JDK-6736490 - https://bugs.openjdk.java.net/browse/JDK-8035284 - https://bugs.openjdk.java.net/browse/JDK-8145680 So I think now we can accept the patch as the changes appear to be safe. Ok, it seems like a good change. Are you able to file a JBS issue for this? After that you can request a formal code review. Thanks, Sean
[PATCH] remove redundant initialization of volatile fields with default values
Hello,
previously I've sent an email regarding removal of redundant assignments if
default values to volatile fields, see
https://mail.openjdk.java.net/pipermail/security-dev/2020-June/022137.html
There was a concern whether it's completely safe to remove those assignments
from JMM point of view, see
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-June/067341.html
Recently I've found a thread in concurrency-interest mailing list where Aleksey
Shiplive tried to find a constraint
agians such removal:
http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html
It appears that there are no constraitns and Doug Lea mentions in
http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014770.html
that "there is never any reason to explicitly initialize fields to
0/0.0/false/null"
Also there we similar code changes in java.base before:
- https://bugs.openjdk.java.net/browse/JDK-6736490
- https://bugs.openjdk.java.net/browse/JDK-8035284
- https://bugs.openjdk.java.net/browse/JDK-8145680
So I think now we can accept the patch as the changes appear to be safe.
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
--- a/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java
+++ b/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java
@@ -102,11 +102,11 @@
boolean isResumption;
SSLSessionImpl resumingSession;
// Session
[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;
}
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
--- a/src/java.base/share/classes/sun/security/ssl/HandshakeContext.java
+++
