From 8db624109ad8136a39d63632073d36cae1fa346b Mon Sep 17 00:00:00 2001 From: Dan <46821332+nsadeveloper789@users.noreply.github.com> Date: Tue, 16 Feb 2021 10:09:30 -0500 Subject: [PATCH] GP-691: Fixed R*-Tree implementation (closes #2760) --- ...TraceAddressSnapRangePropertyMapSpace.java | 9 ++ ...BTraceAddressSnapRangePropertyMapTree.java | 2 +- .../trace/model/TraceAddressSnapSpace.java | 14 +++- .../AbstractDBTraceMemoryManagerTest.java | 19 +++++ .../spatial/AbstractConstraintsTree.java | 3 +- .../spatial/AbstractRStarConstraintsTree.java | 82 ++++++++++++++++--- .../database/spatial/RStarTreeMapTest.java | 22 ++++- 7 files changed, 131 insertions(+), 20 deletions(-) diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/map/DBTraceAddressSnapRangePropertyMapSpace.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/map/DBTraceAddressSnapRangePropertyMapSpace.java index fb19a381fc..e4462a6ee1 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/map/DBTraceAddressSnapRangePropertyMapSpace.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/database/map/DBTraceAddressSnapRangePropertyMapSpace.java @@ -23,6 +23,7 @@ import java.util.function.Predicate; import com.google.common.collect.Range; +import ghidra.lifecycle.Internal; import ghidra.program.model.address.*; import ghidra.trace.database.map.DBTraceAddressSnapRangePropertyMap.DBTraceAddressSnapRangePropertyMapDataFactory; import ghidra.trace.database.map.DBTraceAddressSnapRangePropertyMapTree.AbstractDBTraceAddressSnapRangePropertyMapData; @@ -200,4 +201,12 @@ public class DBTraceAddressSnapRangePropertyMapSpace getMapSpace() { diff --git a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/model/TraceAddressSnapSpace.java b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/model/TraceAddressSnapSpace.java index 8b42d6855b..36e1e474ea 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/model/TraceAddressSnapSpace.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/main/java/ghidra/trace/model/TraceAddressSnapSpace.java @@ -61,7 +61,12 @@ public class TraceAddressSnapSpace implements EuclideanSpace2D { @Override public double distX(Address x1, Address x2) { - return UnsignedUtils.unsignedLongToDouble(x2.subtract(x1)); + if (x2.compareTo(x1) > 0) { + return UnsignedUtils.unsignedLongToDouble(x2.subtract(x1)); + } + else { + return UnsignedUtils.unsignedLongToDouble(x1.subtract(x2)); + } } @Override @@ -72,7 +77,12 @@ public class TraceAddressSnapSpace implements EuclideanSpace2D { if (y1 == null) { return Double.NEGATIVE_INFINITY; } - return y2 - y1; + if (y2 > y1) { + return y2 - y1; + } + else { + return y1 - y2; + } } @Override diff --git a/Ghidra/Debug/Framework-TraceModeling/src/test/java/ghidra/trace/database/memory/AbstractDBTraceMemoryManagerTest.java b/Ghidra/Debug/Framework-TraceModeling/src/test/java/ghidra/trace/database/memory/AbstractDBTraceMemoryManagerTest.java index ff2171e27c..84cf80f294 100644 --- a/Ghidra/Debug/Framework-TraceModeling/src/test/java/ghidra/trace/database/memory/AbstractDBTraceMemoryManagerTest.java +++ b/Ghidra/Debug/Framework-TraceModeling/src/test/java/ghidra/trace/database/memory/AbstractDBTraceMemoryManagerTest.java @@ -964,4 +964,23 @@ public abstract class AbstractDBTraceMemoryManagerTest frame.getValue(0, r0).getUnsignedValue()); } } + + /** + * This test is based on the MWE submitted in GitHub issue #2760. + */ + @Test + public void testManyStateEntries() throws Exception { + Register pc = toyLanguage.getRegister("pc"); + DBTraceThread thread; + try (UndoableTransaction tid = UndoableTransaction.start(trace, "Testing", true)) { + thread = trace.getThreadManager().addThread("Thread1", Range.atLeast(0L)); + DBTraceMemoryRegisterSpace regs = memory.getMemoryRegisterSpace(thread, true); + + for (int i = 1; i < 2000; i++) { + //System.err.println("Snap " + i); + regs.setState(i, pc, TraceMemoryState.KNOWN); + //regs.stateMapSpace.checkIntegrity(); + } + } + } } diff --git a/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/database/spatial/AbstractConstraintsTree.java b/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/database/spatial/AbstractConstraintsTree.java index 6ac0c9d4dc..835545db40 100644 --- a/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/database/spatial/AbstractConstraintsTree.java +++ b/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/database/spatial/AbstractConstraintsTree.java @@ -882,11 +882,12 @@ public abstract class AbstractConstraintsTree< // /** * An integrity checker for use by tree developers and testers. * + *

* To incorporate additional checks, please prefer to override * {@link #checkNodeIntegrity(DBTreeNodeRecord)} and/or * {@link #checkDataIntegrity(DBTreeDataRecord)} instead of this method. */ - protected void checkIntegrity() { + public void checkIntegrity() { // Before we visit, integrity check that cache. Visiting will affect cache. for (Entry> ent : cachedDataChildren.entrySet()) { Set databasedChildren = new TreeSet<>(Comparator.comparing(DR::getKey)); diff --git a/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/database/spatial/AbstractRStarConstraintsTree.java b/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/database/spatial/AbstractRStarConstraintsTree.java index 9be1393218..161dd47346 100644 --- a/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/database/spatial/AbstractRStarConstraintsTree.java +++ b/Ghidra/Debug/ProposedUtils/src/main/java/ghidra/util/database/spatial/AbstractRStarConstraintsTree.java @@ -24,6 +24,23 @@ import ghidra.util.database.DBCachedObjectStoreFactory; import ghidra.util.database.spatial.DBTreeNodeRecord.NodeType; import ghidra.util.exception.VersionException; +/** + * An R*-Tree implementation of {@link AbstractConstraintsTree} + * + *

+ * The implementation follows + * The R*-tree: + * An Efficient and Robust Access Method for Points and Rectangles. Comments in code referring + * to "the paper", specific sections, or steps of algorithms, are referring specifically to that + * paper. + * + * @param The shape of each data entry + * @param The record type for each data entry + * @param The shape of each node + * @param The record type for each node + * @param The type of value stored in a data entry + * @param The type of supported queries + */ public abstract class AbstractRStarConstraintsTree< // DS extends BoundedShape, // DR extends DBTreeDataRecord, // @@ -169,6 +186,7 @@ public abstract class AbstractRStarConstraintsTree< // * For ChooseSubtree, the part which chooses a leaf node using the nearly minimum * overlap enlargement cost as defined in Section 4.1 of the paper, at the bottom of page 325. * + *

* Ties are resolved using the minimum area enlargement cost. * * @param n the node whose children are leaf nodes @@ -212,6 +230,7 @@ public abstract class AbstractRStarConstraintsTree< // /** * Computes the overlap of a bounding shape (with respect to its siblings) * + *

* This measure is defined in Section 4.1 of the paper. * * @param n the shape to measure @@ -404,6 +423,33 @@ public abstract class AbstractRStarConstraintsTree< // return bestIndex + minChildren; } + protected static class LevelInfo { + int dstLevel; + long reinsertedLevels = 0; // MAX_LEVELS = 64 + + public LevelInfo(int dstLevel) { + this.dstLevel = dstLevel; + } + + public boolean checkAndSetReinserted() { + if ((reinsertedLevels >> dstLevel & 0x1) != 0) { + return true; + } + reinsertedLevels |= (1 << dstLevel); + return false; + } + + public LevelInfo decLevel() { + dstLevel--; + return this; + } + + public void incDepth() { + dstLevel++; + reinsertedLevels <<= 1; + } + } + @Override protected DR doInsertData(DS shape, T value) { // ID1 @@ -411,15 +457,14 @@ public abstract class AbstractRStarConstraintsTree< // entry.setParentKey(-1); // TODO: Probably unnecessary, except error recovery? entry.setShape(shape); entry.setRecordValue(value); - doInsert(entry, leafLevel, new BitSet(MAX_LEVELS)); + doInsert(entry, new LevelInfo(leafLevel)); return entry; } // NOTE: entry may actually be a node - protected void doInsert(DBTreeRecord entry, int dstLevel, - BitSet reinsertedLevels) { + protected void doInsert(DBTreeRecord entry, LevelInfo levelInfo) { // I1 - NR node = doChooseSubtree(dstLevel, entry.getBounds()); + NR node = doChooseSubtree(levelInfo.dstLevel, entry.getBounds()); // I2 if (node.getType() == NodeType.LEAF) { @@ -453,15 +498,17 @@ public abstract class AbstractRStarConstraintsTree< // // I3 NR split = null; if (newChildCount > maxChildren) { - split = doOverflowTreatment(node, dstLevel, reinsertedLevels); + split = doOverflowTreatment(node, levelInfo); } + // NOTE: Depth should never increase more than once per insert + int savedLevel = levelInfo.dstLevel; for (NR propa = node, parent = getParentOf(propa); split != null; // propa = parent, // parent = getParentOf(propa), // - split = doOverflowTreatment(propa, --dstLevel, reinsertedLevels)) { + split = doOverflowTreatment(propa, levelInfo.decLevel())) { if (parent == null) { assert propa == root; - assert dstLevel == 0; + assert levelInfo.dstLevel == 0; root = nodeStore.create(); root.setParentKey(-1); cachedNodeChildren.put(root.getKey(), new ArrayList<>(maxChildren)); @@ -472,6 +519,8 @@ public abstract class AbstractRStarConstraintsTree< // doSetParentKey(propa, root.getKey(), cachedNodeChildren); doSetParentKey(split, root.getKey(), cachedNodeChildren); leafLevel++; + levelInfo.dstLevel = savedLevel; + levelInfo.incDepth(); return; } newChildCount = parent.getChildCount() + 1; @@ -480,20 +529,21 @@ public abstract class AbstractRStarConstraintsTree< // break; } } + levelInfo.dstLevel = savedLevel; } - protected NR doOverflowTreatment(NR n, int level, BitSet reinsertedLevels) { + protected NR doOverflowTreatment(NR n, LevelInfo levelInfo) { // OT1 - if (n != root && !reinsertedLevels.get(level)) { - reinsertedLevels.set(level); - doReInsert(n, level, reinsertedLevels); + if (n != root && !levelInfo.checkAndSetReinserted()) { + doReInsert(n, levelInfo); return null; } return doSplit(n); } - protected void doReInsert(NR n, int level, BitSet reinsertedLevels) { + protected void doReInsert(NR n, LevelInfo levelInfo) { // RI1, RI2 + // Create a "max heap" PriorityQueue farthest = new PriorityQueue<>(); Iterator> it = getChildrenOf(n).iterator(); for (int i = 0; i < reinsertCount; i++) { @@ -501,6 +551,12 @@ public abstract class AbstractRStarConstraintsTree< // DBTreeRecord next = it.next(); farthest.add(new LeastDistanceFromCenterToPoint(next, n.getShape())); } + /** + * Now that the heap is sized "reinsertCount", after each new entry, I can remove the + * nearest, knowing it can't possibly be selected for reinsertion. In the meantime, since I + * know each removed entry will remain in its parent, I can compute the new bounds of the + * parent. + */ NS boundsNearest = null; int dataCountNearest = 0; while (it.hasNext()) { @@ -537,7 +593,7 @@ public abstract class AbstractRStarConstraintsTree< // // NOTE: I know all children will be processed before we could possibly cause a split of n while (!farthest.isEmpty()) { LeastDistanceFromCenterToPoint far = farthest.poll(); - doInsert(far.record, level, reinsertedLevels); + doInsert(far.record, levelInfo); } } diff --git a/Ghidra/Debug/ProposedUtils/src/test/java/ghidra/util/database/spatial/RStarTreeMapTest.java b/Ghidra/Debug/ProposedUtils/src/test/java/ghidra/util/database/spatial/RStarTreeMapTest.java index d48ba1580f..b673901650 100644 --- a/Ghidra/Debug/ProposedUtils/src/test/java/ghidra/util/database/spatial/RStarTreeMapTest.java +++ b/Ghidra/Debug/ProposedUtils/src/test/java/ghidra/util/database/spatial/RStarTreeMapTest.java @@ -627,6 +627,7 @@ public class RStarTreeMapTest { } public static class MyDomainObject extends DBCachedDomainObjectAdapter { + private static final int MAX_CHILDREN = 5; private final DBCachedObjectStoreFactory storeFactory; private final IntRStarTree tree; private final SpatialMap map; @@ -636,8 +637,8 @@ public class RStarTreeMapTest { consumer); storeFactory = new DBCachedObjectStoreFactory(this); try (UndoableTransaction tid = UndoableTransaction.start(this, "CreateMaps", true)) { - tree = - new IntRStarTree(storeFactory, DBIntRectStringDataRecord.TABLE_NAME, true, 5); + tree = new IntRStarTree(storeFactory, DBIntRectStringDataRecord.TABLE_NAME, + true, MAX_CHILDREN); map = tree.asSpatialMap(); } } @@ -647,7 +648,8 @@ public class RStarTreeMapTest { 1000, consumer); storeFactory = new DBCachedObjectStoreFactory(this); // No transaction, as tree should already exist - tree = new IntRStarTree(storeFactory, DBIntRectStringDataRecord.TABLE_NAME, true, 5); + tree = new IntRStarTree(storeFactory, DBIntRectStringDataRecord.TABLE_NAME, + true, MAX_CHILDREN); map = tree.asSpatialMap(); } @@ -900,6 +902,20 @@ public class RStarTreeMapTest { //Thread.sleep(Long.MAX_VALUE); // Meh } + @Test + public void testIntegrityWith2000VerticallyStackedRects() throws Exception { + try (UndoableTransaction tid = UndoableTransaction.start(obj, "AddVertical", true)) { + for (int i = 0; i < 2000; i++) { + System.err.println("Adding " + i); + obj.map.put(rect(0, 10, i, i + 1), "Ent" + i); + // Note, underlying tree is not synchronized, but map is + /*try (LockHold hold = LockHold.lock(obj.getReadWriteLock().readLock())) { + obj.tree.checkIntegrity(); + }*/ + } + } + } + @Test public void testSaveAndLoad() throws IOException, CancelledException, VersionException { try (UndoableTransaction tid = UndoableTransaction.start(obj, "AddRecord", true)) {