Avoid iterating thrice in SetView.immutableCopy()

RELNOTES=Optimize SetView.immutableCopy PiperOrigin-RevId: 695486652
google · Nov 11, 2024 · c309175 · c309175
1 parent 68d28ec
commit c309175
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 18 deletions.
diff --git a/android/guava/src/com/google/common/collect/Sets.java b/android/guava/src/com/google/common/collect/Sets.java
@@ -19,6 +19,7 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.collect.CollectPreconditions.checkNonnegative;
+import static java.lang.Math.min;
 import static java.util.Arrays.asList;
 
 import com.google.common.annotations.GwtCompatible;
@@ -601,9 +602,18 @@ private SetView() {} // no subclasses but our own
      * nonstandard notion of equivalence, for example if it is a {@link TreeSet} using a comparator
      * that is inconsistent with {@link Object#equals(Object)}.
      */
-    @SuppressWarnings("nullness") // Unsafe, but we can't fix it now.
     public ImmutableSet<@NonNull E> immutableCopy() {
-      return ImmutableSet.copyOf((SetView<@NonNull E>) this);
+      // Not using ImmutableSet.copyOf() to avoid iterating thrice (isEmpty, size, iterator).
+      int upperBoundSize = upperBoundSize();
+      if (upperBoundSize == 0) {
+        return ImmutableSet.of();
+      }
+      ImmutableSet.Builder<@NonNull E> builder =
+          ImmutableSet.builderWithExpectedSize(upperBoundSize);
+      for (E element : this) {
+        builder.add(checkNotNull(element));
+      }
+      return builder.build();
     }
 
     /**
@@ -711,6 +721,18 @@ public final void clear() {
      */
     @Override
     public abstract UnmodifiableIterator<E> iterator();
+
+    /**
+     * Returns the upper bound on the size of this set view.
+     *
+     * <p>This method is used to presize the underlying collection when converting to an {@link
+     * ImmutableSet}.
+     */
+    abstract int upperBoundSize();
+
+    static int upperBoundSize(Set<?> set) {
+      return set instanceof SetView ? ((SetView) set).upperBoundSize() : set.size();
+    }
   }
 
   /**
@@ -781,13 +803,8 @@ public <S extends Set<E>> S copyInto(S set) {
       }
 
       @Override
-      @SuppressWarnings({"nullness", "unchecked"}) // see supertype
-      public ImmutableSet<@NonNull E> immutableCopy() {
-        ImmutableSet.Builder<@NonNull E> builder =
-            new ImmutableSet.Builder<@NonNull E>()
-                .addAll((Iterable<@NonNull E>) set1)
-                .addAll((Iterable<@NonNull E>) set2);
-        return (ImmutableSet<@NonNull E>) builder.build();
+      int upperBoundSize() {
+        return upperBoundSize(set1) + upperBoundSize(set2);
       }
     };
   }
@@ -869,6 +886,11 @@ public boolean contains(@CheckForNull Object object) {
       public boolean containsAll(Collection<?> collection) {
         return set1.containsAll(collection) && set2.containsAll(collection);
       }
+
+      @Override
+      int upperBoundSize() {
+        return min(upperBoundSize(set1), upperBoundSize(set2));
+      }
     };
   }
 
@@ -927,6 +949,11 @@ public boolean isEmpty() {
       public boolean contains(@CheckForNull Object element) {
         return set1.contains(element) && !set2.contains(element);
       }
+
+      @Override
+      int upperBoundSize() {
+        return upperBoundSize(set1);
+      }
     };
   }
 
@@ -997,6 +1024,11 @@ public boolean isEmpty() {
       public boolean contains(@CheckForNull Object element) {
         return set1.contains(element) ^ set2.contains(element);
       }
+
+      @Override
+      int upperBoundSize() {
+        return upperBoundSize(set1) + upperBoundSize(set2);
+      }
     };
   }
 

diff --git a/guava/src/com/google/common/collect/Sets.java b/guava/src/com/google/common/collect/Sets.java
@@ -19,6 +19,7 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.collect.CollectPreconditions.checkNonnegative;
+import static java.lang.Math.min;
 import static java.util.Arrays.asList;
 
 import com.google.common.annotations.GwtCompatible;
@@ -601,9 +602,18 @@ private SetView() {} // no subclasses but our own
      * nonstandard notion of equivalence, for example if it is a {@link TreeSet} using a comparator
      * that is inconsistent with {@link Object#equals(Object)}.
      */
-    @SuppressWarnings("nullness") // Unsafe, but we can't fix it now.
     public ImmutableSet<@NonNull E> immutableCopy() {
-      return ImmutableSet.copyOf((SetView<@NonNull E>) this);
+      // Not using ImmutableSet.copyOf() to avoid iterating thrice (isEmpty, size, iterator).
+      int upperBoundSize = upperBoundSize();
+      if (upperBoundSize == 0) {
+        return ImmutableSet.of();
+      }
+      ImmutableSet.Builder<@NonNull E> builder =
+          ImmutableSet.builderWithExpectedSize(upperBoundSize);
+      for (E element : this) {
+        builder.add(checkNotNull(element));
+      }
+      return builder.build();
     }
 
     /**
@@ -725,6 +735,18 @@ public final void clear() {
      */
     @Override
     public abstract UnmodifiableIterator<E> iterator();
+
+    /**
+     * Returns the upper bound on the size of this set view.
+     *
+     * <p>This method is used to presize the underlying collection when converting to an {@link
+     * ImmutableSet}.
+     */
+    abstract int upperBoundSize();
+
+    static int upperBoundSize(Set<?> set) {
+      return set instanceof SetView ? ((SetView) set).upperBoundSize() : set.size();
+    }
   }
 
   /**
@@ -805,13 +827,8 @@ public <S extends Set<E>> S copyInto(S set) {
       }
 
       @Override
-      @SuppressWarnings({"nullness", "unchecked"}) // see supertype
-      public ImmutableSet<@NonNull E> immutableCopy() {
-        ImmutableSet.Builder<@NonNull E> builder =
-            new ImmutableSet.Builder<@NonNull E>()
-                .addAll((Iterable<@NonNull E>) set1)
-                .addAll((Iterable<@NonNull E>) set2);
-        return (ImmutableSet<@NonNull E>) builder.build();
+      int upperBoundSize() {
+        return upperBoundSize(set1) + upperBoundSize(set2);
       }
     };
   }
@@ -903,6 +920,11 @@ public boolean contains(@CheckForNull Object object) {
       public boolean containsAll(Collection<?> collection) {
         return set1.containsAll(collection) && set2.containsAll(collection);
       }
+
+      @Override
+      int upperBoundSize() {
+        return min(upperBoundSize(set1), upperBoundSize(set2));
+      }
     };
   }
 
@@ -971,6 +993,11 @@ public boolean isEmpty() {
       public boolean contains(@CheckForNull Object element) {
         return set1.contains(element) && !set2.contains(element);
       }
+
+      @Override
+      int upperBoundSize() {
+        return upperBoundSize(set1);
+      }
     };
   }
 
@@ -1041,6 +1068,11 @@ public boolean isEmpty() {
       public boolean contains(@CheckForNull Object element) {
         return set1.contains(element) ^ set2.contains(element);
       }
+
+      @Override
+      int upperBoundSize() {
+        return upperBoundSize(set1) + upperBoundSize(set2);
+      }
     };
   }