GP-5494: Fix byte cache and increase number of cached pages (memory viewer).

Ghidra/Debug/Debugger/src/main/java/ghidra/app/plugin/core/debug/gui/memory/CachedBytePage.java

@@ -4,9 +4,9 @@
  * 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.
@@ -16,49 +16,93 @@
 package ghidra.app.plugin.core.debug.gui.memory;
 
 import java.nio.ByteBuffer;
+import java.util.*;
 
 import ghidra.debug.api.tracemgr.DebuggerCoordinates;
 import ghidra.program.model.address.Address;
 
 public class CachedBytePage {
-	private boolean valid = true;
+	private static final int PAGE_SIZE = 4096;
-	private DebuggerCoordinates coordinates;
+	/**
-	private Address start;
+	 * There seem to be a handful of layout indices which are routinely gathered:
-	private byte[] page = new byte[4096];
+	 * <ol>
-	private ByteBuffer buf = ByteBuffer.wrap(page);
+	 * <li>The visible layout, to paint it.</li>
+	 * <li>Layout[0], to get measurements for scrolling amounts.</li>
+	 * <li>The layout containing the cursor, for accessibility.</li>
+	 * </ol>
+	 * <p>
+	 * We include an extra slack entry so that when a new address is accessed, we don't necessarily
+	 * invalidate an entry that is about to be used. Since the entries often are accessed in a
+	 * cycle, the entry about to be used is often the least-recently used.
+	 * <p>
+	 * NOTE: We already instantiate a separate cache for previous vs current coordinates, so no need
+	 * to multiply this by 2.
+	 */
+	private static final int CACHE_SIZE = 4;
+
+	private static boolean coordsEqualForMemory(DebuggerCoordinates c1, DebuggerCoordinates c2) {
+		return c1.getTrace() == c2.getTrace() && c1.getViewSnap() == c2.getViewSnap();
+	}
+
+	record CacheKey(DebuggerCoordinates coordinates, Address start) {
+		int computeOffset(DebuggerCoordinates coordinates, Address address) {
+			if (coordsEqualForMemory(this.coordinates, coordinates)) {
+				long offset = address.subtract(start);
+				if (0 <= offset && offset < PAGE_SIZE) {
+					return (int) offset;
+				}
+			}
+			return -1;
+		}
+	}
+
+	record CacheEntry(byte[] page, ByteBuffer buf) {
+		public CacheEntry(byte[] page) {
+			this(page, ByteBuffer.wrap(page));
+		}
+
+		public CacheEntry() {
+			this(new byte[PAGE_SIZE]);
+		}
+
+		CacheKey refresh(DebuggerCoordinates coordinates, Address address) {
+			buf.clear();
+			Address min = address.getAddressSpace().getMinAddress();
+			Address start = address.subtractWrap(page.length / 2);
+
+			if (start.compareTo(min) < 0 || start.compareTo(address) > 0) {
+				start = min;
+			}
+			coordinates.getTrace()
+					.getMemoryManager()
+					.getViewBytes(coordinates.getViewSnap(), start, buf);
+			return new CacheKey(coordinates, start);
+		}
+	}
+
+	private final SequencedMap<CacheKey, CacheEntry> map = new LinkedHashMap<>();
 
 	public byte getByte(DebuggerCoordinates coordinates, Address address) {
-		long offset;
+		for (Map.Entry<CacheKey, CacheEntry> ent : map.entrySet()) {
-		if (!valid || this.coordinates == null || !this.coordinates.equals(coordinates) ||
+			int offset = ent.getKey().computeOffset(coordinates, address);
-			start == null || !start.hasSameAddressSpace(address)) {
+			if (offset != -1) {
-			offset = refresh(coordinates, address);
+				// LRU logic: Reset the hit entry's age.
-		}
+				map.remove(ent.getKey());
-		else {
+				map.put(ent.getKey(), ent.getValue());
-			offset = address.subtract(start);
+				return ent.getValue().page[offset];
-			if (offset < 0 || 4096 <= offset) {
-				offset = refresh(coordinates, address);
 			}
 		}
-		return page[(int) offset];
+
+		CacheEntry entry =
+			map.size() >= CACHE_SIZE ? map.pollFirstEntry().getValue() : new CacheEntry();
+		CacheKey key = entry.refresh(coordinates, address);
+		int offset = key.computeOffset(coordinates, address);
+		assert offset != -1;
+		map.put(key, entry);
+		return entry.page[offset];
 	}
 
 	public void invalidate() {
-		valid = false;
+		map.clear();
-	}
-
-	private long refresh(DebuggerCoordinates coordinates, Address address) {
-		valid = false;
-		buf.clear();
-		Address min = address.getAddressSpace().getMinAddress();
-		start = address.subtractWrap(page.length / 2);
-
-		if (start.compareTo(min) < 0 || start.compareTo(address) > 0) {
-			start = min;
-		}
-		coordinates.getTrace()
-				.getMemoryManager()
-				.getViewBytes(coordinates.getViewSnap(), start, buf);
-		valid = true;
-		return address.subtract(start);
 	}
 }

Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/memory/DBTraceMemorySpace.java

@@ -336,6 +336,10 @@ public class DBTraceMemorySpace
 		}*/
 	}
 
+	public void checkStateMapIntegrity() {
+		stateMapSpace.checkIntegrity();
+	}
+
 	@Override
 	// TODO: Ensure a code unit is not having rug taken out from under it?
 	public void setState(long snap, Address start, Address end, TraceMemoryState state) {
@@ -757,7 +761,8 @@ public class DBTraceMemorySpace
 
 		spans: for (Lifespan span : viewport.getOrderedSpans(snap)) {
 			Iterator<AddressRange> arit =
-				getAddressesWithState(span, s -> s == TraceMemoryState.KNOWN).iterator(start, true);
+				getAddressesWithState(span, remains, s -> s == TraceMemoryState.KNOWN)
+						.iterator(start, true);
 			while (arit.hasNext()) {
 				AddressRange rng = arit.next();
 				if (rng.getMinAddress().compareTo(toRead.getMaxAddress()) > 0) {

Ghidra/Test/DebuggerIntegrationTest/src/test/java/ghidra/app/plugin/core/debug/gui/memory/DebuggerMemoryBytesProviderTest.java

@@ -17,6 +17,7 @@ package ghidra.app.plugin.core.debug.gui.memory;
 
 import static ghidra.lifecycle.Unfinished.TODO;
 import static org.junit.Assert.*;
+import static org.junit.Assume.assumeFalse;
 
 import java.awt.*;
 import java.awt.datatransfer.Clipboard;
@@ -62,13 +63,16 @@ import ghidra.program.model.lang.RegisterValue;
 import ghidra.program.util.ProgramLocation;
 import ghidra.trace.database.ToyDBTraceBuilder;
 import ghidra.trace.database.memory.DBTraceMemoryManager;
+import ghidra.trace.database.memory.DBTraceMemorySpace;
 import ghidra.trace.database.stack.DBTraceStackManager;
+import ghidra.trace.database.time.DBTraceTimeManager;
 import ghidra.trace.model.Lifespan;
 import ghidra.trace.model.memory.*;
 import ghidra.trace.model.modules.TraceModule;
 import ghidra.trace.model.stack.TraceStack;
 import ghidra.trace.model.target.TraceObject;
 import ghidra.trace.model.thread.TraceThread;
+import ghidra.util.SystemUtilities;
 
 @Category(NightlyCategory.class)
 public class DebuggerMemoryBytesProviderTest extends AbstractGhidraHeadedDebuggerIntegrationTest {
@@ -1184,4 +1188,33 @@ public class DebuggerMemoryBytesProviderTest extends AbstractGhidraHeadedDebugge
 
 		rmiCx.withdrawTarget(tool, tb.trace);
 	}
+
+	@Test
+	public void testPerformanceManuallyWithManyManySnaps() throws Exception {
+		assumeFalse(SystemUtilities.isInTestingBatchMode());
+		createAndOpenTrace();
+
+		// LATER (GP-5594): 100_000 without checkStateMapIntegrity will crash.
+		final long snapCount = 100_000;
+		try (Transaction tx = tb.startTransaction()) {
+			tb.trace.getMemoryManager()
+					.addRegion("Processes[1].Memory[exe:.text]", Lifespan.nowOn(0L),
+						tb.range(0x55550000, 0x5555ffff), TraceMemoryFlag.READ,
+						TraceMemoryFlag.EXECUTE);
+			DBTraceTimeManager time = tb.trace.getTimeManager();
+			DBTraceMemorySpace space = tb.trace.getMemoryManager()
+					.getForSpace(tb.host.getAddressFactory().getDefaultAddressSpace(), true);
+			for (int i = 0; i < snapCount; i++) {
+				time.getSnapshot(i, true);
+				space.putBytes(i, tb.addr(0x55550000 + (i & 0xffff)), tb.buf(i & 0xff));
+				if (i % 1000 == 0) {
+					space.checkStateMapIntegrity();
+				}
+			}
+		}
+		traceManager.activateTrace(tb.trace);
+		traceManager.activateSnap(snapCount - 1);
+
+		Thread.sleep(1); // breakpoint here
+	}
 }