Skip to content

Commit 32ba16f

Browse files
cpovirkGoogle Java Core Libraries
cpovirk
authored and
Google Java Core Libraries
committed
Remove caching of cheap collection views across common.collect.
(like e87d019 and 0a8e1ec but for a wider variety of collections) This includes removing `ViewCachingAbstractMap` entirely in favor of using `AbstractMap` directly. For J2ObjC safety, I removed `@Weak` and `@WeakOuter` annotations now that there is no reference cycle between the outer collection and its view collections. (Really, we should have used `@RetainedWith` instead of `@Weak*`, anyway, so this CL improves fixes existing issues in addition to perhaps preventing new ones. For more on `@RetainedWith`, see cl/781580713.) Parts of the `@Weak*` changes should have been done as part of 0a8e1ec for `Compact*HashMap`. _Not_ covered in this CL: - various other cached views that might benefit, such as `RegularImmutableSet.asList` (cl/922882558) - views that I worry at least a little more about, mainly "invertible operations" (`BiMap.inverse`, `NavigableSet.descendingSet`, etc.) - maybe migrating off `AbstractMap` in some additional cases in which it would make sense to avoid inheriting its fields - GWT/J2CL RELNOTES=n/a PiperOrigin-RevId: 973885682
1 parent f516e75 commit 32ba16f

82 files changed

Lines changed: 311 additions & 1414 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎android/guava-tests/benchmark/com/google/common/collect/ConcurrentHashMultisetBenchmark.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ public boolean setCount(E element, int oldCount, int newCount) {
421421
// Views
422422

423423
@Override
424-
Set<E> createElementSet() {
424+
public Set<E> elementSet() {
425425
Set<E> delegate = countMap.keySet();
426426
return new ForwardingSet<E>() {
427427
@Override

‎android/guava-tests/test/com/google/common/collect/MultimapsTest.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -374,25 +374,25 @@ public void testAsMap_multimap() {
374374
Multimap<String, Integer> multimap =
375375
Multimaps.newMultimap(new HashMap<String, Collection<Integer>>(), new QueueSupplier());
376376
Map<String, Collection<Integer>> map = Multimaps.asMap(multimap);
377-
assertThat(map).isSameInstanceAs(multimap.asMap());
377+
assertThat(map).isEqualTo(multimap.asMap());
378378
}
379379

380380
public void testAsMap_listMultimap() {
381381
ListMultimap<String, Integer> listMultimap = ArrayListMultimap.create();
382382
Map<String, List<Integer>> map = Multimaps.asMap(listMultimap);
383-
assertThat(map).isSameInstanceAs(listMultimap.asMap());
383+
assertThat(map).isEqualTo(listMultimap.asMap());
384384
}
385385

386386
public void testAsMap_setMultimap() {
387387
SetMultimap<String, Integer> setMultimap = LinkedHashMultimap.create();
388388
Map<String, Set<Integer>> map = Multimaps.asMap(setMultimap);
389-
assertThat(map).isSameInstanceAs(setMultimap.asMap());
389+
assertThat(map).isEqualTo(setMultimap.asMap());
390390
}
391391

392392
public void testAsMap_sortedSetMultimap() {
393393
SortedSetMultimap<String, Integer> sortedSetMultimap = TreeMultimap.create();
394394
Map<String, SortedSet<Integer>> map = Multimaps.asMap(sortedSetMultimap);
395-
assertThat(map).isSameInstanceAs(sortedSetMultimap.asMap());
395+
assertThat(map).isEqualTo(sortedSetMultimap.asMap());
396396
}
397397

398398
public void testForMap() {

‎android/guava-tests/test/com/google/common/collect/TransposedTableTest.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,11 @@ public void testTransposedViews() {
6464
Table<Integer, String, Character> original = HashBasedTable.create();
6565
Table<String, Integer, Character> transpose = transpose(original);
6666
original.put(1, "foo", 'a');
67-
assertThat(transpose.rowKeySet()).isSameInstanceAs(original.columnKeySet());
68-
assertThat(transpose.columnKeySet()).isSameInstanceAs(original.rowKeySet());
69-
assertThat(transpose.rowMap()).isSameInstanceAs(original.columnMap());
70-
assertThat(transpose.columnMap()).isSameInstanceAs(original.rowMap());
71-
assertThat(transpose.values()).isSameInstanceAs(original.values());
67+
assertThat(transpose.rowKeySet()).isEqualTo(original.columnKeySet());
68+
assertThat(transpose.columnKeySet()).isEqualTo(original.rowKeySet());
69+
assertThat(transpose.rowMap()).isEqualTo(original.columnMap());
70+
assertThat(transpose.columnMap()).isEqualTo(original.rowMap());
71+
assertThat(transpose.values()).containsExactlyElementsIn(original.values());
7272
assertEquals(original.row(1), transpose.column(1));
7373
assertEquals(original.row(2), transpose.column(2));
7474
assertEquals(original.column("foo"), transpose.row("foo"));

‎android/guava/src/com/google/common/collect/AbstractBiMap.java‎

Lines changed: 3 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,7 @@
2828
import com.google.common.annotations.GwtIncompatible;
2929
import com.google.common.annotations.J2ktIncompatible;
3030
import com.google.errorprone.annotations.CanIgnoreReturnValue;
31-
import com.google.errorprone.annotations.concurrent.LazyInit;
3231
import com.google.j2objc.annotations.RetainedWith;
33-
import com.google.j2objc.annotations.WeakOuter;
3432
import java.io.IOException;
3533
import java.io.ObjectInputStream;
3634
import java.io.ObjectOutputStream;
@@ -208,18 +206,11 @@ public BiMap<V, K> inverse() {
208206
return inverse;
209207
}
210208

211-
@LazyInit private transient @Nullable Set<K> keySet;
212-
213209
@Override
214210
public Set<K> keySet() {
215-
Set<K> result = keySet;
216-
if (result == null) {
217-
result = keySet = new KeySet();
218-
}
219-
return result;
211+
return new KeySet();
220212
}
221213

222-
@WeakOuter
223214
private final class KeySet extends ForwardingSet<K> {
224215
@Override
225216
protected Set<K> delegate() {
@@ -256,22 +247,15 @@ public Iterator<K> iterator() {
256247
}
257248
}
258249

259-
@LazyInit private transient @Nullable Set<V> valueSet;
260-
261250
@Override
262251
public Set<V> values() {
263252
/*
264253
* We can almost reuse the inverse's keySet, except we have to fix the
265254
* iteration order so that it is consistent with the forward map.
266255
*/
267-
Set<V> result = valueSet;
268-
if (result == null) {
269-
result = valueSet = new ValueSet();
270-
}
271-
return result;
256+
return new ValueSet();
272257
}
273258

274-
@WeakOuter
275259
private final class ValueSet extends ForwardingSet<V> {
276260
final Set<V> valuesDelegate = inverse.keySet();
277261

@@ -302,15 +286,9 @@ public String toString() {
302286
}
303287
}
304288

305-
@LazyInit private transient @Nullable Set<Entry<K, V>> entrySet;
306-
307289
@Override
308290
public Set<Entry<K, V>> entrySet() {
309-
Set<Entry<K, V>> result = entrySet;
310-
if (result == null) {
311-
result = entrySet = new EntrySet();
312-
}
313-
return result;
291+
return new EntrySet();
314292
}
315293

316294
private final class BiMapEntry extends ForwardingMapEntry<K, V> {
@@ -371,7 +349,6 @@ public void remove() {
371349
};
372350
}
373351

374-
@WeakOuter
375352
private final class EntrySet extends ForwardingSet<Entry<K, V>> {
376353
final Set<Entry<K, V>> esDelegate = delegate.entrySet();
377354

‎android/guava/src/com/google/common/collect/AbstractListMultimap.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ public final boolean put(@ParametricNullness K key, @ParametricNullness V value)
128128
* values.
129129
*/
130130
@Override
131-
public final Map<K, Collection<V>> asMap() {
131+
public Map<K, Collection<V>> asMap() {
132132
return super.asMap();
133133
}
134134

‎android/guava/src/com/google/common/collect/AbstractMapBasedMultimap.java‎

Lines changed: 10 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,9 @@
3333
import com.google.common.annotations.GwtCompatible;
3434
import com.google.common.annotations.GwtIncompatible;
3535
import com.google.common.annotations.J2ktIncompatible;
36-
import com.google.common.collect.Maps.ViewCachingAbstractMap;
37-
import com.google.j2objc.annotations.WeakOuter;
3836
import java.io.Serializable;
3937
import java.util.AbstractCollection;
38+
import java.util.AbstractMap;
4039
import java.util.Collection;
4140
import java.util.Comparator;
4241
import java.util.ConcurrentModificationException;
@@ -327,7 +326,6 @@ final List<V> wrapList(
327326
* subcollection {@code refreshIfEmpty}, {@code removeIfEmpty}, and {@code addToMap} methods call
328327
* the corresponding methods of the full wrapped collection.
329328
*/
330-
@WeakOuter
331329
class WrappedCollection extends AbstractCollection<V> {
332330
@ParametricNullness final K key;
333331
Collection<V> delegate;
@@ -605,7 +603,6 @@ public final boolean retainAll(Collection<?> c) {
605603
}
606604

607605
/** Set decorator that stays in sync with the multimap values for a key. */
608-
@WeakOuter
609606
final class WrappedSet extends WrappedCollection implements Set<V> {
610607
WrappedSet(@ParametricNullness K key, Set<V> delegate) {
611608
super(key, delegate, null);
@@ -632,7 +629,6 @@ public boolean removeAll(Collection<?> c) {
632629
}
633630

634631
/** SortedSet decorator that stays in sync with the multimap values for a key. */
635-
@WeakOuter
636632
class WrappedSortedSet extends WrappedCollection implements SortedSet<V> {
637633
WrappedSortedSet(
638634
@ParametricNullness K key, SortedSet<V> delegate, @Nullable WrappedCollection ancestor) {
@@ -691,7 +687,6 @@ public final SortedSet<V> tailSet(@ParametricNullness V fromElement) {
691687
}
692688
}
693689

694-
@WeakOuter
695690
final class WrappedNavigableSet extends WrappedSortedSet implements NavigableSet<V> {
696691
WrappedNavigableSet(
697692
@ParametricNullness K key, NavigableSet<V> delegate, @Nullable WrappedCollection ancestor) {
@@ -769,7 +764,6 @@ public NavigableSet<V> tailSet(@ParametricNullness V fromElement, boolean inclus
769764
}
770765

771766
/** List decorator that stays in sync with the multimap values for a key. */
772-
@WeakOuter
773767
private class WrappedList extends WrappedCollection implements List<V> {
774768
WrappedList(@ParametricNullness K key, List<V> delegate, @Nullable WrappedCollection ancestor) {
775769
super(key, delegate, ancestor);
@@ -926,7 +920,7 @@ private final class RandomAccessWrappedList extends WrappedList implements Rando
926920
}
927921

928922
@Override
929-
Set<K> createKeySet() {
923+
public Set<K> keySet() {
930924
return new KeySet(map);
931925
}
932926

@@ -940,7 +934,6 @@ final Set<K> createMaybeNavigableKeySet() {
940934
}
941935
}
942936

943-
@WeakOuter
944937
private class KeySet extends Maps.KeySet<K, Collection<V>> {
945938
KeySet(Map<K, Collection<V>> subMap) {
946939
super(subMap);
@@ -1011,7 +1004,6 @@ public final int hashCode() {
10111004
}
10121005
}
10131006

1014-
@WeakOuter
10151007
private class SortedKeySet extends KeySet implements SortedSet<K> {
10161008
SortedKeySet(SortedMap<K, Collection<V>> subMap) {
10171009
super(subMap);
@@ -1054,7 +1046,6 @@ public SortedSet<K> tailSet(@ParametricNullness K fromElement) {
10541046
}
10551047
}
10561048

1057-
@WeakOuter
10581049
private final class NavigableKeySet extends SortedKeySet implements NavigableSet<K> {
10591050
NavigableKeySet(NavigableMap<K, Collection<V>> subMap) {
10601051
super(subMap);
@@ -1211,11 +1202,6 @@ public final void remove() {
12111202
*/
12121203
@Override
12131204
public Collection<V> values() {
1214-
return super.values();
1215-
}
1216-
1217-
@Override
1218-
final Collection<V> createValues() {
12191205
return new Values();
12201206
}
12211207

@@ -1230,17 +1216,17 @@ V output(@ParametricNullness K key, @ParametricNullness V value) {
12301216
};
12311217
}
12321218

1219+
@Override
1220+
public Multiset<K> keys() {
1221+
return new Multimaps.Keys<K, V>(this);
1222+
}
1223+
12331224
/*
12341225
* TODO(kevinb): should we copy this javadoc to each concrete class, so that
12351226
* classes like LinkedHashMultimap that need to say something different are
12361227
* still able to {@inheritDoc} all the way from Multimap?
12371228
*/
12381229

1239-
@Override
1240-
Multiset<K> createKeys() {
1241-
return new Multimaps.Keys<K, V>(this);
1242-
}
1243-
12441230
/**
12451231
* {@inheritDoc}
12461232
*
@@ -1252,11 +1238,6 @@ Multiset<K> createKeys() {
12521238
*/
12531239
@Override
12541240
public Collection<Entry<K, V>> entries() {
1255-
return super.entries();
1256-
}
1257-
1258-
@Override
1259-
final Collection<Entry<K, V>> createEntries() {
12601241
if (this instanceof SetMultimap) {
12611242
return new EntrySet();
12621243
} else {
@@ -1283,7 +1264,7 @@ Entry<K, V> output(@ParametricNullness K key, @ParametricNullness V value) {
12831264
}
12841265

12851266
@Override
1286-
Map<K, Collection<V>> createAsMap() {
1267+
public Map<K, Collection<V>> asMap() {
12871268
return new AsMap(map);
12881269
}
12891270

@@ -1297,8 +1278,7 @@ final Map<K, Collection<V>> createMaybeNavigableAsMap() {
12971278
}
12981279
}
12991280

1300-
@WeakOuter
1301-
private class AsMap extends ViewCachingAbstractMap<K, Collection<V>> {
1281+
private class AsMap extends AbstractMap<K, Collection<V>> {
13021282
/**
13031283
* Usually the same as map, but smaller for the headMap(), tailMap(), or subMap() of a
13041284
* SortedAsMap.
@@ -1310,7 +1290,7 @@ private class AsMap extends ViewCachingAbstractMap<K, Collection<V>> {
13101290
}
13111291

13121292
@Override
1313-
final Set<Entry<K, Collection<V>>> createEntrySet() {
1293+
public final Set<Entry<K, Collection<V>>> entrySet() {
13141294
return new AsMapEntries();
13151295
}
13161296

@@ -1390,7 +1370,6 @@ final Entry<K, Collection<V>> wrapEntry(Entry<K, Collection<V>> entry) {
13901370
return immutableEntry(key, wrapCollection(key, entry.getValue()));
13911371
}
13921372

1393-
@WeakOuter
13941373
final class AsMapEntries extends Maps.EntrySet<K, Collection<V>> {
13951374
@Override
13961375
Map<K, Collection<V>> map() {
@@ -1449,7 +1428,6 @@ public void remove() {
14491428
}
14501429
}
14511430

1452-
@WeakOuter
14531431
private class SortedAsMap extends AsMap implements SortedMap<K, Collection<V>> {
14541432
SortedAsMap(SortedMap<K, Collection<V>> submap) {
14551433
super(submap);
@@ -1492,21 +1470,10 @@ public SortedMap<K, Collection<V>> tailMap(@ParametricNullness K fromKey) {
14921470
return new SortedAsMap(sortedMap().tailMap(fromKey));
14931471
}
14941472

1495-
@Nullable SortedSet<K> sortedKeySet;
1496-
14971473
// returns a SortedSet, even though returning a Set would be sufficient to
14981474
// satisfy the SortedMap.keySet() interface
14991475
@Override
15001476
public SortedSet<K> keySet() {
1501-
SortedSet<K> result = sortedKeySet;
1502-
if (result == null) {
1503-
result = sortedKeySet = createKeySet();
1504-
}
1505-
return result;
1506-
}
1507-
1508-
@Override
1509-
SortedSet<K> createKeySet() {
15101477
return new SortedKeySet(sortedMap());
15111478
}
15121479
}
@@ -1607,11 +1574,6 @@ public NavigableMap<K, Collection<V>> descendingMap() {
16071574

16081575
@Override
16091576
public NavigableSet<K> keySet() {
1610-
return (NavigableSet<K>) super.keySet();
1611-
}
1612-
1613-
@Override
1614-
NavigableSet<K> createKeySet() {
16151577
return new NavigableKeySet(sortedMap());
16161578
}
16171579

0 commit comments

Comments
 (0)