Improve NaiveMultisetSemaphore#estimateCurrentNumUniqueValues by having it use Semaphore#availablePermits. I have no idea why I didn't do this initially.

RELNOTES: None
PiperOrigin-RevId: 207883650

2 files changed, 28 insertions, 14 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java b/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java
index 5996408d54..8a0bb93bde 100644
--- a/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java
+++ b/src/main/java/com/google/devtools/build/lib/concurrent/MultisetSemaphore.java
@@ -133,14 +133,15 @@ public abstract class MultisetSemaphore<T> {
   }
 
   private static class NaiveMultisetSemaphore<T> extends MultisetSemaphore<T> {
+    private final int maxNumUniqueValues;
     private final Semaphore semaphore;
     private final Object lock = new Object();
     // Protected by 'lock'.
-    private final HashMultiset<T> actualValues;
+    private final HashMultiset<T> actualValues = HashMultiset.create();
 
     private NaiveMultisetSemaphore(int maxNumUniqueValues) {
+      this.maxNumUniqueValues = maxNumUniqueValues;
       this.semaphore = new Semaphore(maxNumUniqueValues);
-      actualValues = HashMultiset.create();
     }
 
     @Override
@@ -173,11 +174,7 @@ public abstract class MultisetSemaphore<T> {
 
     @Override
     public int estimateCurrentNumUniqueValues() {
-      // Notes:
-      //   (1) The race here is completely benign; we're just supposed to return an estimate.
-      //   (2) See the javadoc for Multiset#size, which explains to use entrySet().size() to get the
-      //       number of unique values.
-      return actualValues.entrySet().size();
+      return maxNumUniqueValues - semaphore.availablePermits();
     }
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/concurrent/MultisetSemaphoreTest.java b/src/test/java/com/google/devtools/build/lib/concurrent/MultisetSemaphoreTest.java
index a33aab18f3..3cd0ffc490 100644
--- a/src/test/java/com/google/devtools/build/lib/concurrent/MultisetSemaphoreTest.java
+++ b/src/test/java/com/google/devtools/build/lib/concurrent/MultisetSemaphoreTest.java
@@ -45,27 +45,44 @@ public class MultisetSemaphoreTest {
         .maxNumUniqueValues(3)
         .build();
 
-    // And we serially acquire permits for 3 unique values,
+    // Then it initially has 0 unique values.
+    assertThat(multisetSemaphore.estimateCurrentNumUniqueValues()).isEqualTo(0);
+
+    // And then when we serially acquire permits for 3 unique values,
     multisetSemaphore.acquireAll(ImmutableSet.of("a", "b", "c"));
     // Then the MultisetSemaphore thinks it currently has 3 unique values.
     assertThat(multisetSemaphore.estimateCurrentNumUniqueValues()).isEqualTo(3);
-    // And then attempt to acquire permits for 2 of those same unique values,
-    // Then we don't deadlock,
+
+    // And then when we attempt to acquire permits for 2 of those same unique values, we don't block
+    // forever,
     multisetSemaphore.acquireAll(ImmutableSet.of("b", "c"));
     // And the MultisetSemaphore still thinks it currently has 3 unique values.
     assertThat(multisetSemaphore.estimateCurrentNumUniqueValues()).isEqualTo(3);
-    // And then we release one of the permit for one of those unique values,
+
+    // And then when we release one of the permit for one of those unique values,
     multisetSemaphore.releaseAll(ImmutableSet.of("c"));
-    // Then the MultisetSemaphore still thinks it currently has 3 unique values.
+    // The MultisetSemaphore still thinks it currently has 3 unique values.
     assertThat(multisetSemaphore.estimateCurrentNumUniqueValues()).isEqualTo(3);
+
     // And then we release the final permit for that unique value,
     multisetSemaphore.releaseAll(ImmutableSet.of("c"));
-    // Then the MultisetSemaphore thinks it currently has 2 unique values.
+    // The MultisetSemaphore thinks it currently has 2 unique values.
     assertThat(multisetSemaphore.estimateCurrentNumUniqueValues()).isEqualTo(2);
-    // And we are able to acquire a permit for a 4th unique value,
+
+    // And then we attempt to acquire a permit for a 4th unique value, we don't block forever,
     multisetSemaphore.acquireAll(ImmutableSet.of("d"));
     // And the MultisetSemaphore thinks it currently has 3 unique values.
     assertThat(multisetSemaphore.estimateCurrentNumUniqueValues()).isEqualTo(3);
+
+    // And then we release one permit each for the remaining 3 that unique values,
+    multisetSemaphore.releaseAll(ImmutableSet.of("a", "b", "d"));
+    // The MultisetSemaphore thinks it currently has 1 unique values.
+    assertThat(multisetSemaphore.estimateCurrentNumUniqueValues()).isEqualTo(1);
+
+    // And then we release the final permit for the remaining unique value,
+    multisetSemaphore.releaseAll(ImmutableSet.of("b"));
+    // The MultisetSemaphore thinks it currently has 0 unique values.
+    assertThat(multisetSemaphore.estimateCurrentNumUniqueValues()).isEqualTo(0);
   }
 
   @Test