From 745068430a57a66890ac0718af66e71df8bef0d4 Mon Sep 17 00:00:00 2001 From: Michael Pollmeier Date: Mon, 30 Jan 2023 10:11:02 +0100 Subject: [PATCH] use regular jdk String.intern() I'm not sure why we didn't use `String.intern()` from the start. This PR doesn't only simplify our codebase, but saves us the `internedStrings` ConcurrentHashMap. The heap size for a large CPG (linux 4 kernel) goes down from 35830m to 35260m, i.e. 570m, or 1.6%. --- .../java/overflowdb/storage/OdbStorageTest.java | 14 ++++++-------- core/src/main/java/overflowdb/Graph.java | 10 ++-------- .../overflowdb/storage/NodeDeserializer.java | 5 +---- .../main/java/overflowdb/storage/OdbStorage.java | 15 ++++++--------- .../java/overflowdb/util/StringInterner.java | 16 ---------------- 5 files changed, 15 insertions(+), 45 deletions(-) delete mode 100644 core/src/main/java/overflowdb/util/StringInterner.java diff --git a/core-tests/src/test/java/overflowdb/storage/OdbStorageTest.java b/core-tests/src/test/java/overflowdb/storage/OdbStorageTest.java index 650a4c77..413d07d4 100644 --- a/core-tests/src/test/java/overflowdb/storage/OdbStorageTest.java +++ b/core-tests/src/test/java/overflowdb/storage/OdbStorageTest.java @@ -8,7 +8,6 @@ import overflowdb.Graph; import overflowdb.testdomains.gratefuldead.GratefulDead; import overflowdb.testdomains.gratefuldead.Song; -import overflowdb.util.StringInterner; import java.io.File; import java.io.IOException; @@ -18,7 +17,6 @@ import static org.junit.Assert.assertFalse; public class OdbStorageTest { - private StringInterner stringInterner = new StringInterner(); @Test public void persistToFileIfStorageConfigured() throws IOException { @@ -68,7 +66,7 @@ public void shouldDeleteTmpStorageIfNoStorageLocationConfigured() { public void shouldErrorWhenTryingToOpenWithoutStorageFormatVersion() throws IOException { File storageFile = Files.createTempFile("overflowdb", "bin").toFile(); storageFile.deleteOnExit(); - OdbStorage storage = OdbStorage.createWithSpecificLocation(storageFile, stringInterner); + OdbStorage storage = OdbStorage.createWithSpecificLocation(storageFile); storage.close(); // modify storage: drop storage version @@ -78,14 +76,14 @@ public void shouldErrorWhenTryingToOpenWithoutStorageFormatVersion() throws IOEx store.close(); // should throw a BackwardsCompatibilityError - OdbStorage.createWithSpecificLocation(storageFile, stringInterner); + OdbStorage.createWithSpecificLocation(storageFile); } @Test(expected = BackwardsCompatibilityError.class) public void shouldErrorWhenTryingToOpenDifferentStorageFormatVersion() throws IOException { File storageFile = Files.createTempFile("overflowdb", "bin").toFile(); storageFile.deleteOnExit(); - OdbStorage storage = OdbStorage.createWithSpecificLocation(storageFile, stringInterner); + OdbStorage storage = OdbStorage.createWithSpecificLocation(storageFile); storage.close(); // modify storage: change storage version @@ -95,14 +93,14 @@ public void shouldErrorWhenTryingToOpenDifferentStorageFormatVersion() throws IO store.close(); // should throw a BackwardsCompatibilityError - OdbStorage.createWithSpecificLocation(storageFile, stringInterner); + OdbStorage.createWithSpecificLocation(storageFile); } @Test public void shouldProvideStringToIntGlossary() throws IOException { File storageFile = Files.createTempFile("overflowdb", "bin").toFile(); storageFile.deleteOnExit(); - OdbStorage storage = OdbStorage.createWithSpecificLocation(storageFile, stringInterner); + OdbStorage storage = OdbStorage.createWithSpecificLocation(storageFile); String a = "a"; String b = "b"; @@ -119,7 +117,7 @@ public void shouldProvideStringToIntGlossary() throws IOException { // should survive restarts storage.close(); - storage = OdbStorage.createWithSpecificLocation(storageFile, stringInterner); + storage = OdbStorage.createWithSpecificLocation(storageFile); assertEquals(stringIdA, storage.lookupOrCreateStringToIntMapping(a)); assertEquals(stringIdB, storage.lookupOrCreateStringToIntMapping(b)); diff --git a/core/src/main/java/overflowdb/Graph.java b/core/src/main/java/overflowdb/Graph.java index 2293ec2a..d9a70035 100644 --- a/core/src/main/java/overflowdb/Graph.java +++ b/core/src/main/java/overflowdb/Graph.java @@ -31,7 +31,6 @@ public final class Graph implements AutoCloseable { protected final OdbStorage storage; public final NodeSerializer nodeSerializer; protected final NodeDeserializer nodeDeserializer; - protected final StringInterner stringInterner; protected final Optional heapUsageMonitor; protected final boolean overflowEnabled; protected final ReferenceManager referenceManager; @@ -66,11 +65,10 @@ private Graph(Config config, this.config = config; this.nodeFactoryByLabel = nodeFactoryByLabel; this.edgeFactoryByLabel = edgeFactoryByLabel; - this.stringInterner = new StringInterner(); this.storage = config.getStorageLocation().isPresent() - ? OdbStorage.createWithSpecificLocation(config.getStorageLocation().get().toFile(), stringInterner) - : OdbStorage.createWithTempFile(stringInterner); + ? OdbStorage.createWithSpecificLocation(config.getStorageLocation().get().toFile()) + : OdbStorage.createWithTempFile(); this.nodeDeserializer = new NodeDeserializer(this, nodeFactoryByLabel, config.isSerializationStatsEnabled(), storage); this.nodeSerializer = new NodeSerializer(config.isSerializationStatsEnabled(), storage, convertPropertyForPersistence); this.nodesWriter = new NodesWriter(nodeSerializer, storage); @@ -216,7 +214,6 @@ public synchronized void close() { } else { this.closed = true; shutdownNow(); - stringInterner.clear(); } } @@ -432,7 +429,4 @@ public ArrayList> getAllLibraryVersions() { return storage.getAllLibraryVersions(); } - public StringInterner getStringInterner() { - return this.stringInterner; - } } diff --git a/core/src/main/java/overflowdb/storage/NodeDeserializer.java b/core/src/main/java/overflowdb/storage/NodeDeserializer.java index 9ac1cc63..28e9b8e8 100644 --- a/core/src/main/java/overflowdb/storage/NodeDeserializer.java +++ b/core/src/main/java/overflowdb/storage/NodeDeserializer.java @@ -11,7 +11,6 @@ import overflowdb.NodeFactory; import overflowdb.NodeRef; import overflowdb.util.PropertyHelper; -import overflowdb.util.StringInterner; import java.io.IOException; import java.util.ArrayList; @@ -23,12 +22,10 @@ public class NodeDeserializer extends BookKeeper { protected final Graph graph; private final Map nodeFactoryByLabel; private final OdbStorage storage; - private final StringInterner stringInterner; public NodeDeserializer(Graph graph, Map nodeFactoryByLabel, boolean statsEnabled, OdbStorage storage) { super(statsEnabled); this.graph = graph; - this.stringInterner = graph.getStringInterner(); this.nodeFactoryByLabel = nodeFactoryByLabel; this.storage = storage; } @@ -113,7 +110,7 @@ private final Object unpackValue(final ArrayValue packedValueAndType) { case BOOLEAN: return value.asBooleanValue().getBoolean(); case STRING: - return stringInterner.intern(value.asStringValue().asString()); + return value.asStringValue().asString().intern(); case BYTE: return value.asIntegerValue().asByte(); case SHORT: diff --git a/core/src/main/java/overflowdb/storage/OdbStorage.java b/core/src/main/java/overflowdb/storage/OdbStorage.java index 90411f84..4c97c28e 100644 --- a/core/src/main/java/overflowdb/storage/OdbStorage.java +++ b/core/src/main/java/overflowdb/storage/OdbStorage.java @@ -5,7 +5,6 @@ import org.h2.mvstore.MVStore; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import overflowdb.util.StringInterner; import java.io.File; import java.io.IOException; @@ -31,7 +30,6 @@ public class OdbStorage implements AutoCloseable { private final Logger logger = LoggerFactory.getLogger(getClass()); private final File mvstoreFile; - private final StringInterner stringInterner; private FileStore mvstoreFileStore; protected MVStore mvstore; private MVMap nodesMVMap; @@ -42,20 +40,19 @@ public class OdbStorage implements AutoCloseable { private ArrayList stringToIntReverseMappings; private int libraryVersionsIdCurrentRun; - public static OdbStorage createWithTempFile(StringInterner stringInterner) { - return new OdbStorage(Optional.empty(), stringInterner); + public static OdbStorage createWithTempFile() { + return new OdbStorage(Optional.empty()); } /** * create with specific mvstore file - which may or may not yet exist. * mvstoreFile won't be deleted at the end (unlike temp file constructors above) */ - public static OdbStorage createWithSpecificLocation(final File mvstoreFile, StringInterner stringInterner) { - return new OdbStorage(Optional.ofNullable(mvstoreFile), stringInterner); + public static OdbStorage createWithSpecificLocation(final File mvstoreFile) { + return new OdbStorage(Optional.ofNullable(mvstoreFile)); } - private OdbStorage(final Optional mvstoreFileMaybe, StringInterner stringInterner) { - this.stringInterner = stringInterner; + private OdbStorage(final Optional mvstoreFileMaybe) { if (mvstoreFileMaybe.isPresent()) { mvstoreFile = mvstoreFileMaybe.get(); if (mvstoreFile.exists() && mvstoreFile.length() > 0) { @@ -206,7 +203,7 @@ private void ensureCapacity(ArrayList array, int requiredMinSize) { public String reverseLookupStringToIntMapping(int stringId) { getStringToIntMappings(); //ensure everything is initialized String string = stringToIntReverseMappings.get(stringId); - return stringInterner.intern(string); + return string.intern(); } private void ensureMVStoreAvailable() { diff --git a/core/src/main/java/overflowdb/util/StringInterner.java b/core/src/main/java/overflowdb/util/StringInterner.java deleted file mode 100644 index 13837835..00000000 --- a/core/src/main/java/overflowdb/util/StringInterner.java +++ /dev/null @@ -1,16 +0,0 @@ -package overflowdb.util; - -import java.util.concurrent.ConcurrentHashMap; - -public class StringInterner { - private ConcurrentHashMap internedStrings = new ConcurrentHashMap<>(); - - public String intern(String s){ - String interned = internedStrings.putIfAbsent(s, s); - return interned == null ? s : interned; - } - - public void clear() { - internedStrings.clear(); - } -}