GH-5581: Only use value cache on value resolution

Author: Ostrzyciel

core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/ValueStore.java

@@ -527,28 +527,16 @@ private static int nextPowerOfTwo(int n) {
 	 * @throws IOException If an I/O error occurred.
 	 */
 	public LmdbValue getLazyValue(long id) throws IOException {
-		// Check value cache
-		LmdbValue resultValue = cachedValue(id);
-
-		if (resultValue == null) {
-			switch ((byte) (id & 0x3)) {
-			case URI_VALUE:
-				resultValue = new LmdbIRI(lazyRevision, id);
-				break;
-			case LITERAL_VALUE:
-				resultValue = new LmdbLiteral(lazyRevision, id);
-				break;
-			case BNODE_VALUE:
-				resultValue = new LmdbBNode(lazyRevision, id);
-				break;
-			default:
-				throw new IOException("Unsupported value with type id " + (id & 0x3));
-			}
-			// Store value in cache
-			cacheValue(id, resultValue);
+		switch ((byte) (id & 0x3)) {
+		case URI_VALUE:
+			return new LmdbIRI(lazyRevision, id);
+		case LITERAL_VALUE:
+			return new LmdbLiteral(lazyRevision, id);
+		case BNODE_VALUE:
+			return new LmdbBNode(lazyRevision, id);
+		default:
+			throw new IOException("Unsupported value with type id " + (id & 0x3));
 		}
-
-		return resultValue;
 	}
 
 	/**
@@ -589,10 +577,17 @@ public LmdbValue getValue(long id) throws IOException {
 	 * @return <code>true</code> if value could be successfully resolved, else <code>false</code>
 	 */
 	public boolean resolveValue(long id, LmdbValue value) {
+		// Try to get from cache
+		LmdbValue cached = cachedValue(id);
+		if (cached != null && this.getRevision().getRevisionId() == cached.getValueStoreRevision().getRevisionId()) {
+			value.setFromInitializedValue(cached);
+			return true;
+		}
 		try {
 			byte[] data = getData(id);
 			if (data != null) {
 				data2value(id, data, value);
+				cacheValue(id, value);
 				return true;
 			}
 		} catch (IOException e) {

core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/model/LmdbBNode.java

@@ -67,6 +67,16 @@ public ValueStoreRevision getValueStoreRevision() {
 		return revision;
 	}
 
+	@Override
+	public void setFromInitializedValue(LmdbValue initializedValue) {
+		if (initializedValue instanceof LmdbBNode) {
+			LmdbBNode lmdbBNode = (LmdbBNode) initializedValue;
+			super.setID(lmdbBNode.getID());
+		} else {
+			throw new IllegalArgumentException("Initialized value is not of type LmdbBNode");
+		}
+	}
+
 	@Override
 	public long getInternalID() {
 		return internalID;

core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/model/LmdbIRI.java

@@ -15,6 +15,7 @@
 
 import org.eclipse.rdf4j.model.IRI;
 import org.eclipse.rdf4j.model.util.URIUtil;
+import org.eclipse.rdf4j.sail.SailException;
 import org.eclipse.rdf4j.sail.lmdb.ValueStoreRevision;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -86,6 +87,17 @@ public ValueStoreRevision getValueStoreRevision() {
 		return revision;
 	}
 
+	@Override
+	public void setFromInitializedValue(LmdbValue initializedValue) {
+		if (initializedValue instanceof LmdbIRI) {
+			LmdbIRI initializedIRI = (LmdbIRI) initializedValue;
+			this.iriString = initializedIRI.iriString;
+			this.localNameIdx = initializedIRI.localNameIdx;
+		} else {
+			throw new SailException("Trying to initialize LmdbIRI from non-IRI value");
+		}
+	}
+
 	@Override
 	public long getInternalID() {
 		return internalID;

core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/model/LmdbLiteral.java

@@ -147,6 +147,19 @@ public ValueStoreRevision getValueStoreRevision() {
 		return revision;
 	}
 
+	@Override
+	public void setFromInitializedValue(LmdbValue initializedValue) {
+		if (initializedValue instanceof LmdbLiteral) {
+			LmdbLiteral lmdbLiteral = (LmdbLiteral) initializedValue;
+			this.label = lmdbLiteral.label;
+			this.language = lmdbLiteral.language;
+			this.datatype = lmdbLiteral.datatype;
+			this.coreDatatype = lmdbLiteral.coreDatatype;
+		} else {
+			throw new IllegalArgumentException("Initialized value is not of type LmdbLiteral");
+		}
+	}
+
 	@Override
 	public long getInternalID() {
 		return internalID;

core/sail/lmdb/src/main/java/org/eclipse/rdf4j/sail/lmdb/model/LmdbValue.java

@@ -10,6 +10,7 @@
  *******************************************************************************/
 package org.eclipse.rdf4j.sail.lmdb.model;
 
+import org.eclipse.rdf4j.common.annotation.InternalUseOnly;
 import org.eclipse.rdf4j.model.Value;
 import org.eclipse.rdf4j.sail.lmdb.ValueStoreRevision;
 
@@ -41,4 +42,14 @@ public interface LmdbValue extends Value {
 	 * @return The revision of the value store that created this value at the time it last set the value's internal ID.
 	 */
 	ValueStoreRevision getValueStoreRevision();
+
+	/**
+	 * Sets this value's data from an initialized value.
+	 * <p>
+	 * This must be only called within a synchronized block in the init() method of the uninitialized value.
+	 *
+	 * @param initializedValue the initialized value to copy data from
+	 */
+	@InternalUseOnly
+	void setFromInitializedValue(LmdbValue initializedValue);
 }

core/sail/lmdb/src/test/java/org/eclipse/rdf4j/sail/lmdb/ValueStoreCacheTest.java

@@ -10,7 +10,9 @@
  *******************************************************************************/
 package org.eclipse.rdf4j.sail.lmdb;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 
 import java.io.File;
@@ -38,12 +40,19 @@ void cachedValuePath(@TempDir File dataDir) throws Exception {
 			IRI iri = vf.createIRI("urn:example:foo");
 			long id = vs.getId(iri, true);
 
-			// Populate cache via lazy retrieval
+			// On lazy retrieval, the cache should not be touched
 			LmdbValue v1 = vs.getLazyValue(id);
+			assertNotNull(v1);
+			LmdbValue cached1 = vs.cachedValue(id);
+			assertNull(cached1);
+
+			// After initializing the value, it should be cached
+			assertEquals(v1.stringValue(), "urn:example:foo");
+			LmdbValue cached2 = vs.cachedValue(id);
+			assertSame(v1, cached2);
+
 			// Direct cache hit
 			LmdbValue v2 = vs.cachedValue(id);
-
-			assertNotNull(v1);
 			assertSame(v1, v2);
 		} finally {
 			store.shutDown();

core/sail/lmdb/src/test/java/org/eclipse/rdf4j/sail/lmdb/benchmark/QueryBenchmark.java

@@ -79,6 +79,7 @@ public class QueryBenchmark {
 	private static final String wild_card_chain_with_common_ends;
 	private static final String sub_select;
 	private static final String multiple_sub_select;
+	private static final String count_all;
 
 	static {
 		try {
@@ -116,6 +117,7 @@ public class QueryBenchmark {
 			sub_select = IOUtils.toString(getResourceAsStream("benchmarkFiles/sub-select.qr"), StandardCharsets.UTF_8);
 			multiple_sub_select = IOUtils.toString(
 					getResourceAsStream("benchmarkFiles/multiple-sub-select.qr"), StandardCharsets.UTF_8);
+			count_all = "SELECT (COUNT(*) as ?c) WHERE { ?s ?p ?o }";
 
 		} catch (IOException e) {
 			throw new RuntimeException(e);
@@ -415,6 +417,15 @@ public long multiple_sub_select() {
 		}
 	}
 
+	@Benchmark
+	public long count_all() {
+		try (SailRepositoryConnection connection = repository.getConnection()) {
+			return count(connection
+					.prepareTupleQuery(count_all)
+					.evaluate());
+		}
+	}
+
 //	@Benchmark
 //	public long wild_card_chain_with_common_ends() {
 //		try (SailRepositoryConnection connection = repository.getConnection()) {