Collapse inheritance hierarchy of `PersistentStringIndexer`.
`AbstractIndexer` and `CanonicalStringIndexer` only have a single subclass. Merge them both into `PersistentStringIndexer`.
PiperOrigin-RevId: 625452496
Change-Id: I12eaab56ffb36b5e5f3cb1c822ee737249d16eb8
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java
index 5cefc15..ba21a14 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java
@@ -210,7 +210,7 @@
PersistentStringIndexer indexer;
try {
- indexer = PersistentStringIndexer.newPersistentStringIndexer(indexFile, clock);
+ indexer = PersistentStringIndexer.create(indexFile, clock);
} catch (IOException e) {
return logAndThrowOrRecurse(
cacheRoot,
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexer.java b/src/main/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexer.java
index e12826c..18a401e 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexer.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexer.java
@@ -13,10 +13,13 @@
// limitations under the License.
package com.google.devtools.build.lib.actions.cache;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
import com.google.devtools.build.lib.clock.Clock;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ConditionallyThreadSafe;
-import com.google.devtools.build.lib.util.CanonicalStringIndexer;
import com.google.devtools.build.lib.util.PersistentMap;
+import com.google.devtools.build.lib.util.StringCanonicalizer;
+import com.google.devtools.build.lib.util.StringIndexer;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
import java.io.DataInputStream;
@@ -24,21 +27,114 @@
import java.io.IOException;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
import javax.annotation.Nullable;
/**
- * Persistent version of the CanonicalStringIndexer.
+ * Persistent implementation of {@link StringIndexer}.
*
- * <p>This class is backed by a PersistentMap that holds one direction of the canonicalization
- * mapping. The other direction is handled purely in memory and reconstituted at load-time.
+ * <p>This class is backed by a {@link PersistentMap} that holds one direction of the
+ * canonicalization mapping. The other direction is handled purely in memory and reconstituted at
+ * load-time.
*
* <p>Thread-safety is ensured by locking on all mutating operations from the superclass. Read-only
* operations are not locked, but rather backed by ConcurrentMaps.
*/
-@ConditionallyThreadSafe // condition: each instance must instantiated with
-// different dataFile.
-final class PersistentStringIndexer extends CanonicalStringIndexer {
+@ConditionallyThreadSafe // Each instance must be instantiated with a different dataPath.
+final class PersistentStringIndexer implements StringIndexer {
+
+ private static final int NOT_FOUND = -1;
+ private static final int INITIAL_ENTRIES = 10000;
+
+ /** Instantiates and loads instance of the persistent string indexer. */
+ static PersistentStringIndexer create(Path dataPath, Clock clock) throws IOException {
+ PersistentIndexMap persistentIndexMap =
+ new PersistentIndexMap(
+ dataPath, FileSystemUtils.replaceExtension(dataPath, ".journal"), clock);
+ Map<Integer, String> reverseMapping = new ConcurrentHashMap<>(INITIAL_ENTRIES);
+ for (Map.Entry<String, Integer> entry : persistentIndexMap.entrySet()) {
+ if (reverseMapping.put(entry.getValue(), entry.getKey()) != null) {
+ throw new IOException("Corrupted filename index has duplicate entry: " + entry.getKey());
+ }
+ }
+ return new PersistentStringIndexer(persistentIndexMap, reverseMapping);
+ }
+
+ // This is similar to (Synchronized) BiMap.
+ // These maps *must* be weakly threadsafe to ensure thread safety for string indexer as a whole.
+ // Specifically, mutating operations are serialized, but read-only operations may be executed
+ // concurrently with mutators.
+ private final PersistentIndexMap stringToInt;
+ private final Map<Integer, String> intToString;
+
+ private PersistentStringIndexer(
+ PersistentIndexMap stringToInt, Map<Integer, String> intToString) {
+ this.stringToInt = stringToInt;
+ this.intToString = intToString;
+ }
+
+ @Override
+ public synchronized void clear() {
+ stringToInt.clear();
+ intToString.clear();
+ }
+
+ @Override
+ public int size() {
+ return intToString.size();
+ }
+
+ @Override
+ public int getOrCreateIndex(String s) {
+ Integer i = stringToInt.get(s);
+ if (i == null) {
+ s = StringCanonicalizer.intern(s);
+ synchronized (this) {
+ // First, make sure another thread hasn't just added the entry:
+ i = stringToInt.get(s);
+ if (i != null) {
+ return i;
+ }
+
+ int ind = intToString.size();
+ stringToInt.put(s, ind);
+ intToString.put(ind, s);
+ return ind;
+ }
+ } else {
+ return i;
+ }
+ }
+
+ @Override
+ public int getIndex(String s) {
+ return stringToInt.getOrDefault(s, NOT_FOUND);
+ }
+
+ @Override
+ @Nullable
+ public String getStringForIndex(int i) {
+ return intToString.get(i);
+ }
+
+ /** Saves index data to the file. */
+ synchronized long save() throws IOException {
+ return stringToInt.save();
+ }
+
+ /** Flushes the journal. */
+ synchronized void flush() {
+ stringToInt.flush();
+ }
+
+ @Override
+ public synchronized String toString() {
+ StringBuilder builder = new StringBuilder();
+ builder.append("size = ").append(size()).append("\n");
+ for (Map.Entry<String, Integer> entry : stringToInt.entrySet()) {
+ builder.append(entry.getKey()).append(" <==> ").append(entry.getValue()).append("\n");
+ }
+ return builder.toString();
+ }
/**
* Persistent metadata map. Used as a backing map to provide a persistent implementation of the
@@ -51,12 +147,8 @@
private final Clock clock;
private long nextUpdate;
- public PersistentIndexMap(Path mapFile, Path journalFile, Clock clock) throws IOException {
- super(
- VERSION,
- PersistentStringIndexer.<String, Integer>newConcurrentMap(INITIAL_ENTRIES),
- mapFile,
- journalFile);
+ PersistentIndexMap(Path mapFile, Path journalFile, Clock clock) throws IOException {
+ super(VERSION, new ConcurrentHashMap<>(INITIAL_ENTRIES), mapFile, journalFile);
this.clock = clock;
nextUpdate = clock.nanoTime();
load(/* failFast= */ true);
@@ -73,13 +165,12 @@
}
@Override
- @Nullable
public Integer remove(Object object) {
throw new UnsupportedOperationException();
}
- public void flush() {
- super.forceFlush();
+ void flush() {
+ forceFlush();
}
@Override
@@ -90,7 +181,7 @@
}
byte[] content = new byte[length];
in.readFully(content);
- return bytes2string(content);
+ return new String(content, UTF_8);
}
@Override
@@ -100,7 +191,7 @@
@Override
protected void writeKey(String key, DataOutputStream out) throws IOException {
- byte[] content = string2bytes(key);
+ byte[] content = key.getBytes(UTF_8);
out.writeInt(content.length);
out.write(content);
}
@@ -110,48 +201,4 @@
out.writeInt(value);
}
}
-
- private final PersistentIndexMap persistentIndexMap;
- private static final int INITIAL_ENTRIES = 10000;
-
- /**
- * Instantiates and loads instance of the persistent string indexer.
- */
- static PersistentStringIndexer newPersistentStringIndexer(Path dataPath,
- Clock clock) throws IOException {
- PersistentIndexMap persistentIndexMap = new PersistentIndexMap(dataPath,
- FileSystemUtils.replaceExtension(dataPath, ".journal"), clock);
- Map<Integer, String> reverseMapping = newConcurrentMap(INITIAL_ENTRIES);
- for (Map.Entry<String, Integer> entry : persistentIndexMap.entrySet()) {
- if (reverseMapping.put(entry.getValue(), entry.getKey()) != null) {
- throw new IOException("Corrupted filename index has duplicate entry: " + entry.getKey());
- }
- }
- return new PersistentStringIndexer(persistentIndexMap, reverseMapping);
- }
-
- private PersistentStringIndexer(PersistentIndexMap stringToInt,
- Map<Integer, String> intToString) {
- super(stringToInt, intToString);
- this.persistentIndexMap = stringToInt;
- }
-
- /**
- * Saves index data to the file.
- */
- synchronized long save() throws IOException {
- return persistentIndexMap.save();
- }
-
- /**
- * Flushes the journal.
- */
- synchronized void flush() {
- persistentIndexMap.flush();
- }
-
- private static <K, V> ConcurrentMap<K, V> newConcurrentMap(int expectedCapacity) {
- return new ConcurrentHashMap<>(expectedCapacity);
- }
-
}
diff --git a/src/main/java/com/google/devtools/build/lib/util/AbstractIndexer.java b/src/main/java/com/google/devtools/build/lib/util/AbstractIndexer.java
deleted file mode 100644
index 110b2e1..0000000
--- a/src/main/java/com/google/devtools/build/lib/util/AbstractIndexer.java
+++ /dev/null
@@ -1,37 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-
-package com.google.devtools.build.lib.util;
-
-import static java.nio.charset.StandardCharsets.UTF_8;
-
-/**
- * Abstract class for string indexers.
- */
-abstract class AbstractIndexer implements StringIndexer {
-
- /**
- * Conversion from String to byte[].
- */
- protected static byte[] string2bytes(String string) {
- return string.getBytes(UTF_8);
- }
-
- /**
- * Conversion from byte[] to String.
- */
- protected static String bytes2string(byte[] bytes) {
- return new String(bytes, UTF_8);
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/util/BUILD b/src/main/java/com/google/devtools/build/lib/util/BUILD
index f21f547..e39f8a7 100644
--- a/src/main/java/com/google/devtools/build/lib/util/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/util/BUILD
@@ -206,10 +206,8 @@
java_library(
name = "util",
srcs = [
- "AbstractIndexer.java",
"AnsiStrippingOutputStream.java",
"CPU.java",
- "CanonicalStringIndexer.java",
"ClassName.java",
"DependencySet.java",
"Either.java",
@@ -243,7 +241,6 @@
deps = [
":os",
":shell_escaper",
- ":string",
"//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/vfs",
diff --git a/src/main/java/com/google/devtools/build/lib/util/CanonicalStringIndexer.java b/src/main/java/com/google/devtools/build/lib/util/CanonicalStringIndexer.java
deleted file mode 100644
index 43e0ed9..0000000
--- a/src/main/java/com/google/devtools/build/lib/util/CanonicalStringIndexer.java
+++ /dev/null
@@ -1,104 +0,0 @@
-// Copyright 2014 The Bazel Authors. All rights reserved.
-//
-// Licensed under the Apache License, Version 2.0 (the "License");
-// you may not use this file except in compliance with the License.
-// You may obtain a copy of the License at
-//
-// http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
-package com.google.devtools.build.lib.util;
-
-import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import java.util.Map;
-
-/**
- * A string indexer backed by a map and reverse index lookup.
- * Every unique string is stored in memory exactly once.
- */
-@ThreadSafe
-public class CanonicalStringIndexer extends AbstractIndexer {
-
- private static final int NOT_FOUND = -1;
-
- // This is similar to (Synchronized) BiMap.
- // These maps *must* be weakly threadsafe to ensure thread safety for string
- // indexer as a whole. Specifically, mutating operations are serialized, but
- // read-only operations may be executed concurrently with mutators.
- private final Map<String, Integer> stringToInt;
- private final Map<Integer, String> intToString;
-
- /*
- * Creates an indexer instance from two backing maps. These maps may be
- * pre-initialized with data, but *must*:
- * a. Support read-only operations done concurrently with mutations.
- * Note that mutations will be serialized.
- * b. Be reverse mappings of each other, if pre-initialized.
- */
- public CanonicalStringIndexer(Map<String, Integer> stringToInt,
- Map<Integer, String> intToString) {
- Preconditions.checkArgument(stringToInt.size() == intToString.size());
- this.stringToInt = stringToInt;
- this.intToString = intToString;
- }
-
-
- @Override
- public synchronized void clear() {
- stringToInt.clear();
- intToString.clear();
- }
-
- @Override
- public int size() {
- return intToString.size();
- }
-
- @Override
- public int getOrCreateIndex(String s) {
- Integer i = stringToInt.get(s);
- if (i == null) {
- s = StringCanonicalizer.intern(s);
- synchronized (this) {
- // First, make sure another thread hasn't just added the entry:
- i = stringToInt.get(s);
- if (i != null) {
- return i;
- }
-
- int ind = intToString.size();
- stringToInt.put(s, ind);
- intToString.put(ind, s);
- return ind;
- }
- } else {
- return i;
- }
- }
-
- @Override
- public int getIndex(String s) {
- Integer i = stringToInt.get(s);
- return (i == null) ? NOT_FOUND : i;
- }
-
- @Override
- public String getStringForIndex(int i) {
- return intToString.get(i);
- }
-
- @Override
- public synchronized String toString() {
- StringBuilder builder = new StringBuilder();
- builder.append("size = ").append(size()).append("\n");
- for (Map.Entry<String, Integer> entry : stringToInt.entrySet()) {
- builder.append(entry.getKey()).append(" <==> ").append(entry.getValue()).append("\n");
- }
- return builder.toString();
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/util/StringIndexer.java b/src/main/java/com/google/devtools/build/lib/util/StringIndexer.java
index 9bbd8d6..28a4f39 100644
--- a/src/main/java/com/google/devtools/build/lib/util/StringIndexer.java
+++ b/src/main/java/com/google/devtools/build/lib/util/StringIndexer.java
@@ -13,40 +13,32 @@
// limitations under the License.
package com.google.devtools.build.lib.util;
-/**
- * An object that provides bidirectional String <-> unique integer mapping.
- */
+import javax.annotation.Nullable;
+
+/** Provides bidirectional string ⇔ unique integer mapping. */
public interface StringIndexer {
- /**
- * Removes all mappings.
- */
- public void clear();
+ /** Removes all mappings. */
+ void clear();
+
+ /** Returns the number of strings in the index. */
+ int size();
/**
- * @return some measure of the size of the index.
+ * Creates new mapping for the given string if necessary and returns string index. Also, as a side
+ * effect, additional mappings may be created for various prefixes of the given string.
*/
- public int size();
+ int getOrCreateIndex(String s);
/**
- * Creates new mapping for the given string if necessary and returns
- * string index. Also, as a side effect, zero or more additional mappings
- * may be created for various prefixes of the given string.
- *
- * @return a unique index.
+ * Returns the unique index for the given string if one was created via {@link #getOrCreateIndex},
+ * or else {@code -1}.
*/
- public int getOrCreateIndex(String s);
+ int getIndex(String s);
/**
- * @return a unique index for the given string or -1 if string
- * was not added.
+ * Returns the string associated with the given index or {@code null} if it is not in the index.
*/
- public int getIndex(String s);
-
- /**
- * @return string associated with the given index or null if
- * mapping does not exist.
- */
- public String getStringForIndex(int i);
-
+ @Nullable
+ String getStringForIndex(int i);
}
diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexerTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexerTest.java
index f307b5c..6876590 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/cache/PersistentStringIndexerTest.java
@@ -71,7 +71,7 @@
public final void createIndexer() throws Exception {
dataPath = scratch.resolve("/cache/test.dat");
journalPath = scratch.resolve("/cache/test.journal");
- psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock);
+ psi = PersistentStringIndexer.create(dataPath, clock);
}
private void assertSize(int expected) {
@@ -160,7 +160,7 @@
assertThat(journalPath.exists()).isFalse();
// Now restore data from file and verify it.
- psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock);
+ psi = PersistentStringIndexer.create(dataPath, clock);
assertThat(journalPath.exists()).isFalse();
clock.advance(4);
assertSize(10);
@@ -182,7 +182,7 @@
assertThat(journalPath.exists()).isTrue();
// Now restore data from file and verify it. All data should be restored from journal;
- psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock);
+ psi = PersistentStringIndexer.create(dataPath, clock);
assertThat(dataPath.exists()).isTrue();
assertThat(journalPath.exists()).isFalse();
clock.advance(4);
@@ -208,7 +208,7 @@
assertThat(journalPath.exists()).isTrue();
// Now restore data from file and verify it. All data should be restored from journal;
- psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock);
+ psi = PersistentStringIndexer.create(dataPath, clock);
assertThat(dataPath.exists()).isTrue();
assertThat(journalPath.exists()).isFalse();
assertThat(dataPath.getFileSize())
@@ -240,7 +240,7 @@
assertThat(journalPath.exists()).isTrue();
// Now restore data from file and verify it. All data should be restored from journal;
- psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock);
+ psi = PersistentStringIndexer.create(dataPath, clock);
assertThat(dataPath.exists()).isTrue();
assertThat(journalPath.exists()).isFalse();
assertThat(dataPath.getFileSize())
@@ -257,8 +257,7 @@
FileSystemUtils.writeContentAsLatin1(journalPath, "bogus content");
IOException e =
assertThrows(
- IOException.class,
- () -> psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock));
+ IOException.class, () -> psi = PersistentStringIndexer.create(dataPath, clock));
assertThat(e).hasMessageThat().contains("too short: Only 13 bytes");
journalPath.delete();
@@ -274,7 +273,7 @@
byte[] journalContent = FileSystemUtils.readContent(journalPath);
// Now restore data from file and verify it. All data should be restored from journal;
- psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock);
+ psi = PersistentStringIndexer.create(dataPath, clock);
assertThat(dataPath.exists()).isTrue();
assertThat(journalPath.exists()).isFalse();
@@ -282,9 +281,7 @@
assertThat(dataPath.delete()).isTrue();
FileSystemUtils.writeContent(journalPath,
Arrays.copyOf(journalContent, journalContent.length - 1));
- assertThrows(
- EOFException.class,
- () -> psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock));
+ assertThrows(EOFException.class, () -> psi = PersistentStringIndexer.create(dataPath, clock));
// Corrupt the journal with a negative size value.
byte[] journalCopy = journalContent.clone();
@@ -293,16 +290,13 @@
FileSystemUtils.writeContent(journalPath, journalCopy);
e =
assertThrows(
- IOException.class,
- () -> psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock));
+ IOException.class, () -> psi = PersistentStringIndexer.create(dataPath, clock));
assertThat(e).hasMessageThat().contains("corrupt key length");
// Now put back corrupted journal. We should get an error.
journalContent[journalContent.length - 13] = 100;
FileSystemUtils.writeContent(journalPath, journalContent);
- assertThrows(
- IOException.class,
- () -> psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock));
+ assertThrows(IOException.class, () -> psi = PersistentStringIndexer.create(dataPath, clock));
}
@Test
@@ -337,8 +331,7 @@
IOException e =
assertThrows(
- IOException.class,
- () -> psi = PersistentStringIndexer.newPersistentStringIndexer(dataPath, clock));
+ IOException.class, () -> psi = PersistentStringIndexer.create(dataPath, clock));
assertThat(e).hasMessageThat().contains("Corrupted filename index has duplicate entry");
}
