8145539: (coll) AbstractMap.keySet and .values should not be volatile

Reviewed-by: redestad, plevart, dl, psandoz
This commit is contained in:
Aleksey Shipilev 2015-12-17 21:14:58 +03:00
parent acc5017a2e
commit 4e9bef8f92
7 changed files with 90 additions and 35 deletions

View File

@ -304,9 +304,28 @@ public abstract class AbstractMap<K,V> implements Map<K,V> {
* Each of these fields are initialized to contain an instance of the
* appropriate view the first time this view is requested. The views are
* stateless, so there's no reason to create more than one of each.
*
* <p>Since there is no synchronization performed while accessing these fields,
* it is expected that java.util.Map view classes using these fields have
* no non-final fields (or any fields at all except for outer-this). Adhering
* to this rule would make the races on these fields benign.
*
* <p>It is also imperative that implementations read the field only once,
* as in:
*
* <pre> {@code
* public Set<K> keySet() {
* Set<K> ks = keySet; // single racy read
* if (ks == null) {
* ks = new KeySet();
* keySet = ks;
* }
* return ks;
* }
*}</pre>
*/
transient volatile Set<K> keySet;
transient volatile Collection<V> values;
transient Set<K> keySet;
transient Collection<V> values;
/**
* {@inheritDoc}
@ -325,8 +344,9 @@ public abstract class AbstractMap<K,V> implements Map<K,V> {
* method will not all return the same set.
*/
public Set<K> keySet() {
if (keySet == null) {
keySet = new AbstractSet<K>() {
Set<K> ks = keySet;
if (ks == null) {
ks = new AbstractSet<K>() {
public Iterator<K> iterator() {
return new Iterator<K>() {
private Iterator<Entry<K,V>> i = entrySet().iterator();
@ -361,8 +381,9 @@ public abstract class AbstractMap<K,V> implements Map<K,V> {
return AbstractMap.this.containsKey(k);
}
};
keySet = ks;
}
return keySet;
return ks;
}
/**
@ -382,8 +403,9 @@ public abstract class AbstractMap<K,V> implements Map<K,V> {
* method will not all return the same collection.
*/
public Collection<V> values() {
if (values == null) {
values = new AbstractCollection<V>() {
Collection<V> vals = values;
if (vals == null) {
vals = new AbstractCollection<V>() {
public Iterator<V> iterator() {
return new Iterator<V>() {
private Iterator<Entry<K,V>> i = entrySet().iterator();
@ -418,8 +440,9 @@ public abstract class AbstractMap<K,V> implements Map<K,V> {
return AbstractMap.this.containsValue(v);
}
};
values = vals;
}
return values;
return vals;
}
public abstract Set<Entry<K,V>> entrySet();

View File

@ -380,10 +380,11 @@ public class EnumMap<K extends Enum<K>, V> extends AbstractMap<K, V>
*/
public Set<K> keySet() {
Set<K> ks = keySet;
if (ks != null)
return ks;
else
return keySet = new KeySet();
if (ks == null) {
ks = new KeySet();
keySet = ks;
}
return ks;
}
private class KeySet extends AbstractSet<K> {
@ -418,10 +419,11 @@ public class EnumMap<K extends Enum<K>, V> extends AbstractMap<K, V>
*/
public Collection<V> values() {
Collection<V> vs = values;
if (vs != null)
return vs;
else
return values = new Values();
if (vs == null) {
vs = new Values();
values = vs;
}
return vs;
}
private class Values extends AbstractCollection<V> {

View File

@ -902,8 +902,12 @@ public class HashMap<K,V> extends AbstractMap<K,V>
* @return a set view of the keys contained in this map
*/
public Set<K> keySet() {
Set<K> ks;
return (ks = keySet) == null ? (keySet = new KeySet()) : ks;
Set<K> ks = keySet;
if (ks == null) {
ks = new KeySet();
keySet = ks;
}
return ks;
}
final class KeySet extends AbstractSet<K> {
@ -949,8 +953,12 @@ public class HashMap<K,V> extends AbstractMap<K,V>
* @return a view of the values contained in this map
*/
public Collection<V> values() {
Collection<V> vs;
return (vs = values) == null ? (values = new Values()) : vs;
Collection<V> vs = values;
if (vs == null) {
vs = new Values();
values = vs;
}
return vs;
}
final class Values extends AbstractCollection<V> {

View File

@ -964,10 +964,11 @@ public class IdentityHashMap<K,V>
*/
public Set<K> keySet() {
Set<K> ks = keySet;
if (ks != null)
return ks;
else
return keySet = new KeySet();
if (ks == null) {
ks = new KeySet();
keySet = ks;
}
return ks;
}
private class KeySet extends AbstractSet<K> {
@ -1069,10 +1070,11 @@ public class IdentityHashMap<K,V>
*/
public Collection<V> values() {
Collection<V> vs = values;
if (vs != null)
return vs;
else
return values = new Values();
if (vs == null) {
vs = new Values();
values = vs;
}
return vs;
}
private class Values extends AbstractCollection<V> {

View File

@ -528,8 +528,12 @@ public class LinkedHashMap<K,V>
* @return a set view of the keys contained in this map
*/
public Set<K> keySet() {
Set<K> ks;
return (ks = keySet) == null ? (keySet = new LinkedKeySet()) : ks;
Set<K> ks = keySet;
if (ks == null) {
ks = new LinkedKeySet();
keySet = ks;
}
return ks;
}
final class LinkedKeySet extends AbstractSet<K> {
@ -577,8 +581,12 @@ public class LinkedHashMap<K,V>
* @return a view of the values contained in this map
*/
public Collection<V> values() {
Collection<V> vs;
return (vs = values) == null ? (values = new LinkedValues()) : vs;
Collection<V> vs = values;
if (vs == null) {
vs = new LinkedValues();
values = vs;
}
return vs;
}
final class LinkedValues extends AbstractCollection<V> {

View File

@ -852,7 +852,11 @@ public class TreeMap<K,V>
*/
public Collection<V> values() {
Collection<V> vs = values;
return (vs != null) ? vs : (values = new Values());
if (vs == null) {
vs = new Values();
values = vs;
}
return vs;
}
/**

View File

@ -866,7 +866,11 @@ public class WeakHashMap<K,V>
*/
public Set<K> keySet() {
Set<K> ks = keySet;
return (ks != null ? ks : (keySet = new KeySet()));
if (ks == null) {
ks = new KeySet();
keySet = ks;
}
return ks;
}
private class KeySet extends AbstractSet<K> {
@ -915,7 +919,11 @@ public class WeakHashMap<K,V>
*/
public Collection<V> values() {
Collection<V> vs = values;
return (vs != null) ? vs : (values = new Values());
if (vs == null) {
vs = new Values();
values = vs;
}
return vs;
}
private class Values extends AbstractCollection<V> {