Merge remote-tracking branch 'origin/GT-3535_ghidravore_category_performance'

Author: ghidravore

Ghidra/Features/Base/src/test.slow/java/ghidra/program/database/data/CategoryTest.java

@@ -575,6 +575,37 @@ public void testDataTypeDeleted() throws Exception {
 		assertEquals(comps.length - 1, newDt.getDefinedComponents().length);
 	}
 
+	@Test
+	public void testDataTypeConflictHandling() throws Exception {
+		Category sub1 = root.createCategory("Cat1");
+		DataType dt1 = new StructureDataType("DT", 1);
+		DataType dt2 = new StructureDataType("DT", 2);
+		DataType added1 = sub1.addDataType(dt1, null);
+		DataType added2 = sub1.addDataType(dt2, null);
+		assertEquals("DT", added1.getName());
+		assertEquals("DT.conflict", added2.getName());
+
+		List<DataType> list = sub1.getDataTypesByBaseName("DT");
+		assertEquals(2, list.size());
+		assertEquals(added1, list.get(0));
+		assertEquals(added2, list.get(1));
+
+		list = sub1.getDataTypesByBaseName("DT.conflict");
+		assertEquals(2, list.size());
+		assertEquals(added1, list.get(0));
+		assertEquals(added2, list.get(1));
+
+		sub1.remove(added2, TaskMonitor.DUMMY);
+		list = sub1.getDataTypesByBaseName("DT");
+		assertEquals(1, list.size());
+		assertEquals(added1, list.get(0));
+
+		list = sub1.getDataTypesByBaseName("DT.conflict");
+		assertEquals(1, list.size());
+		assertEquals(added1, list.get(0));
+
+	}
+
 	@Test
 	public void testGetDataTypeManager() throws Exception {
 		Category sub1 = root.createCategory("SubCat-A");
@@ -772,10 +803,7 @@ public void testListenerDataTypeChanged() throws Exception {
 		clearEvents();
 
 		struct2 = (Structure) newDt.insert(3, struct2).getDataType();
-		int eventCount = getEventCount();
-		if (4 != eventCount) {
-			System.err.println("halt!");
-		}
+
 		assertEquals(5, getEventCount());
 		Event ev = getEvent(4);
 		assertEquals("DT Changed", ev.evName);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/CategoryDB.java

@@ -16,8 +16,7 @@
 package ghidra.program.database.data;
 
 import java.io.IOException;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
 
 import db.Record;
@@ -26,6 +25,7 @@
 import ghidra.program.model.data.*;
 import ghidra.program.model.data.DataTypeConflictHandler.ConflictResult;
 import ghidra.util.InvalidNameException;
+import ghidra.util.Lock;
 import ghidra.util.exception.AssertException;
 import ghidra.util.exception.DuplicateNameException;
 import ghidra.util.task.TaskMonitor;
@@ -41,6 +41,7 @@ class CategoryDB extends DatabaseObject implements Category {
 
 	private LazyLoadingCachingMap<String, CategoryDB> subcategoryMap;
 	private LazyLoadingCachingMap<String, DataType> dataTypeMap;
+	private ConflictMap conflictMap;
 
 	/**
 	 * Category Constructor
@@ -57,19 +58,19 @@ class CategoryDB extends DatabaseObject implements Category {
 		this.name = name;
 		this.parent = parent;
 
-		subcategoryMap = new LazyLoadingCachingMap<>(mgr.lock, CategoryDB.class) {
+		subcategoryMap = new LazyLoadingCachingMap<>(mgr.lock) {
 			@Override
 			public Map<String, CategoryDB> loadMap() {
 				return buildSubcategoryMap();
 			}
 		};
-		dataTypeMap = new LazyLoadingCachingMap<>(mgr.lock, DataType.class) {
+		dataTypeMap = new LazyLoadingCachingMap<>(mgr.lock) {
 			@Override
 			public Map<String, DataType> loadMap() {
 				return createDataTypeMap();
 			}
 		};
-
+		conflictMap = new ConflictMap(mgr.lock);
 	}
 
 	/**
@@ -102,6 +103,7 @@ protected boolean refresh() {
 	protected boolean refresh(Record rec) {
 		subcategoryMap.clear();
 		dataTypeMap.clear();
+		conflictMap.clear();
 
 		if (isRoot()) {
 			return true;
@@ -210,13 +212,26 @@ private Map<String, DataType> createDataTypeMap() {
 		return map;
 	}
 
+	private String getBaseName(String dataTypeName) {
+		int indexOf = dataTypeName.indexOf(DataType.CONFLICT_SUFFIX);
+		if (indexOf <= 0) {
+			return dataTypeName;
+		}
+		return dataTypeName.substring(0, indexOf);
+	}
+
+	private boolean isConflictName(String dataTypeName) {
+		return dataTypeName.contains(DataType.CONFLICT_SUFFIX);
+	}
+
 	/**
 	 * @see ghidra.program.model.data.Category#getCategories()
 	 */
 	@Override
 	public Category[] getCategories() {
 		validate(mgr.lock);
-		return subcategoryMap.valuesToArray();
+		Collection<CategoryDB> categories = subcategoryMap.values();
+		return categories.toArray(new Category[categories.size()]);
 	}
 
 	/**
@@ -225,7 +240,8 @@ public Category[] getCategories() {
 	@Override
 	public DataType[] getDataTypes() {
 		validate(mgr.lock);
-		return dataTypeMap.valuesToArray();
+		Collection<DataType> dataTypes = dataTypeMap.values();
+		return dataTypes.toArray(new DataType[dataTypes.size()]);
 	}
 
 	/**
@@ -587,19 +603,144 @@ private void catagoryRenamed(CategoryDB childCategory, String oldName) {
 	}
 
 	void dataTypeRenamed(DataType childDataType, String oldName) {
-		dataTypeMap.remove(oldName);
-		dataTypeMap.put(childDataType.getName(), childDataType);
+		dataTypeRemoved(oldName);
+		dataTypeAdded(childDataType);
 	}
 
-	void dataTypeAdded(DataType childDataType) {
-		dataTypeMap.put(childDataType.getName(), childDataType);
+	void dataTypeAdded(DataType dataType) {
+		String dtName = dataType.getName();
+		dataTypeMap.put(dtName, dataType);
+		if (isConflictName(dtName)) {
+			conflictMap.addDataType(dataType);
+		}
 	}
 
 	void dataTypeRemoved(String dataTypeName) {
 		dataTypeMap.remove(dataTypeName);
+		if (isConflictName(dataTypeName)) {
+			conflictMap.removeDataTypeName(dataTypeName);
+		}
 	}
 
 	void categoryAdded(CategoryDB cat) {
 		subcategoryMap.put(cat.getName(), cat);
 	}
+
+	@Override
+	public List<DataType> getDataTypesByBaseName(String dataTypeName) {
+		List<DataType> list = new ArrayList<>();
+		String baseName = getBaseName(dataTypeName);
+
+		DataType baseType = dataTypeMap.get(baseName);
+		if (baseType != null) {
+			list.add(baseType);
+		}
+
+		List<DataType> relatedNameDataTypes = conflictMap.getDataTypesByBaseName(baseName);
+		list.addAll(relatedNameDataTypes);
+		return list;
+	}
+
+	/**
+	 * Class to handle the complexities of having a map as the value in a LazyLoadingCachingMap
+	 * This map uses the data type's base name as the key (i.e. all .conflict suffixes stripped off.)
+	 * The value is another map that maps the actual data type's name to the data type. This map
+	 * effectively provides an efficient way to get all data types in a category that have the
+	 * same name, but possibly have had their name modified (by appending .conflict) to get around
+	 * the requirement that names have to be unique in the same category.
+	 */
+	private class ConflictMap extends LazyLoadingCachingMap<String, Map<String, DataType>> {
+
+		ConflictMap(Lock lock) {
+			super(lock);
+		}
+
+		/**
+		 * Creates a map of all data types whose name has a .conflict suffix where the key
+		 * is the base name and {@link LazyLoadingCachingMap} the value is a map of actual name 
+		 * to data type. This mapping is
+		 * maintained as a lazy cache map. This is only called by the super class when the
+		 * cached needs to be populated and we are depending on it to acquire the necessary
+		 * database lock. (See {@link LazyLoadingCachingMap#loadMap()}
+		 * @return the loaded map
+		 */
+		@Override
+		protected Map<String, Map<String, DataType>> loadMap() {
+			Map<String, Map<String, DataType>> map = new HashMap<>();
+			Collection<DataType> values = dataTypeMap.values();
+			for (DataType dataType : values) {
+				String dataTypeName = dataType.getName();
+				if (isConflictName(dataTypeName)) {
+					String baseName = getBaseName(dataTypeName);
+					Map<String, DataType> innerMap =
+						map.computeIfAbsent(baseName, b -> new HashMap<>());
+					innerMap.put(dataTypeName, dataType);
+				}
+			}
+			return map;
+		}
+
+		/**
+		 * Adds the data type to the conflict mapping structure. If the mapping is currently not
+		 * loaded then this method can safely do nothing. This method is synchronized to provide
+		 * thread safe access/manipulation of the map.
+		 * @param dataType the data type to add to the mapping if the mapping is already loaded
+		 */
+		synchronized void addDataType(DataType dataType) {
+			// if the cache is not currently populated, don't need to do anything
+			Map<String, Map<String, DataType>> map = getMap();
+			if (map == null) {
+				return;
+			}
+
+			String dataTypeName = dataType.getName();
+			String baseName = getBaseName(dataTypeName);
+			Map<String, DataType> innerMap = map.computeIfAbsent(baseName, b -> new HashMap<>());
+			innerMap.put(dataTypeName, dataType);
+		}
+
+		/**
+		 * Removes the data type with the given name from the conflict mapping structure. If the 
+		 * mapping is currently not loaded then this method can safely do nothing. This method is
+		 * synchronized to provide thread safe access/manipulate of the map.
+		 * @param dataTypeName the name of the data type  to remove from this mapping
+		 */
+		synchronized void removeDataTypeName(String dataTypeName) {
+			Map<String, Map<String, DataType>> map = getMap();
+			if (map == null) {
+				return;
+			}
+			String baseName = getBaseName(dataTypeName);
+			Map<String, DataType> innerMap = map.get(baseName);
+			if (innerMap == null) {
+				return;
+			}
+			innerMap.remove(dataTypeName);
+		}
+
+		/**
+		 * Returns a list of all data types that have conflict names for the given base name
+		 * @param baseName the data type base name to search for (i.e. the .conflict suffix removed)
+		 * @return a list of all conflict named data types that would have the given base name if
+		 * no conflicts existed
+		 */
+		List<DataType> getDataTypesByBaseName(String baseName) {
+
+			// Note that the following call to get MUST NOT be in a synchronized block because
+			// it may trigger a loading of the cache which requires a database lock and you
+			// can't be synchronized on this class when acquiring a database lock or else a
+			// deadlock will occur.
+			Map<String, DataType> map = get(baseName);
+			if (map == null) {
+				return Collections.emptyList();
+			}
+
+			// the following must be synchronized so that the implied iterator can complete without
+			// another thread changing the map's values.
+			synchronized (this) {
+				return new ArrayList<>(map.values());
+			}
+		}
+
+	}
 }

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/DataTypeManagerDB.java

@@ -934,14 +934,12 @@ private DataType findEquivalentDataTypeSameLocation(DataType dataType, String ex
 		if (category == null) {
 			return null;
 		}
-		String namePrefix = dtName + DataType.CONFLICT_SUFFIX;
-		DataType[] dataTypes = category.getDataTypes();
-		for (DataType candidate : dataTypes) {
+		List<DataType> relatedByName = category.getDataTypesByBaseName(dtName);
+
+		for (DataType candidate : relatedByName) {
 			String candidateName = candidate.getName();
-			if (candidateName.startsWith(namePrefix)) {
-				if (!candidateName.equals(excludedName) && candidate.isEquivalent(dataType)) {
-					return candidate;
-				}
+			if (!candidateName.equals(excludedName) && candidate.isEquivalent(dataType)) {
+				return candidate;
 			}
 		}
 		return null;
@@ -3208,13 +3206,12 @@ List<DataType> getParentDataTypes(long childID) {
 		lock.acquire();
 		try {
 			long[] ids = parentChildAdapter.getParentIds(childID);
-			// TODO: consider deduping ids using Set
 			List<DataType> dts = new ArrayList<>();
-			for (int i = 0; i < ids.length; i++) {
-				DataType dt = getDataType(ids[i]);
+			for (long id : ids) {
+				DataType dt = getDataType(id);
 				if (dt == null) {
 					// cleanup invalid records for missing parent
-					attemptRecordRemovalForParent(ids[i]);
+					attemptRecordRemovalForParent(id);
 				}
 				else {
 					dts.add(dt);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/data/LazyLoadingCachingMap.java

@@ -16,8 +16,7 @@
 package ghidra.program.database.data;
 
 import java.lang.ref.SoftReference;
-import java.lang.reflect.Array;
-import java.util.Map;
+import java.util.*;
 
 import ghidra.util.Lock;
 
@@ -39,15 +38,14 @@ public abstract class LazyLoadingCachingMap<K, V> {
 
 	private Lock lock;
 	private SoftReference<Map<K, V>> softRef;
-	private Class<V> valueClass;
 
-	protected LazyLoadingCachingMap(Lock lock, Class<V> valueClass) {
+	protected LazyLoadingCachingMap(Lock lock) {
 		this.lock = lock;
-		this.valueClass = valueClass;
 	}
 
 	/**
-	 * This method will reload the map data from scratch.
+	 * This method will reload the map data from scratch. Subclass may assume that the database
+	 * lock has been acquired.
 	 * @return a map containing all current key, value pairs.
 	 */
 	protected abstract Map<K, V> loadMap();
@@ -96,13 +94,13 @@ public V get(K key) {
 		}
 	}
 
-	public V[] valuesToArray() {
+	/**
+	 * Returns an unmodifiable view of the values in this map.
+	 * @return an unmodifiable view of the values in this map.
+	 */
+	public Collection<V> values() {
 		Map<K, V> map = getOrLoadMap();
-		synchronized (this) {
-			@SuppressWarnings("unchecked")
-			V[] array = (V[]) Array.newInstance(valueClass, map.size());
-			return map.values().toArray(array);
-		}
+		return Collections.unmodifiableCollection(map.values());
 	}
 
 	private Map<K, V> getOrLoadMap() {
@@ -113,6 +111,15 @@ private Map<K, V> getOrLoadMap() {
 				return map;
 			}
 		}
+
+		// We must get the database lock before calling loadMap().  Also, we can't get the
+		// database lock while having the synchronization lock for this class or a deadlock can
+		// occur, since the other methods may be called while the client has the db lock.
+		// Note: all other places where the map is being used or manipulated, it must be done
+		// while having the class's synchronization lock since the map itself is not thread safe.
+		// It should be safe here since it creates a new map and then in one operation it sets it
+		// as the map to be used elsewhere.
+
 		lock.acquire();
 		try {
 			map = getMap();
@@ -132,11 +139,12 @@ private Map<K, V> getOrLoadMap() {
 	 * "lock".
 	 * @return the underlying map of key,value pairs or null if it is currently not loaded.
 	 */
-	private Map<K, V> getMap() {
+	protected Map<K, V> getMap() {
 		if (softRef == null) {
 			return null;
 		}
 		return softRef.get();
 	}
 
+
 }

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/model/data/Category.java

@@ -15,6 +15,8 @@
  */
 package ghidra.program.model.data;
 
+import java.util.List;
+
 import ghidra.util.InvalidNameException;
 import ghidra.util.exception.DuplicateNameException;
 import ghidra.util.task.TaskMonitor;
@@ -48,6 +50,19 @@ public interface Category extends Comparable<Category> {
 	 */
 	public abstract DataType[] getDataTypes();
 
+	/**
+	 * Get all data types in this category whose base name matches the base name of the given name.
+	 * The base name of a name is the first part of the string up to where the first ".conflict"
+	 * occurs. In other words, finds all data types whose name matches the given name once
+	 * any conflict suffixes have been removed from both the given name and the data types
+	 * that are being scanned. 
+	 * @param name the name for which to get conflict related data types in this category. Note: the
+	 * name that is passed in will be normalized to its base name, so you may pass in names with .conflict
+	 * appended as a convenience.
+	 * @return a list of data types that have the same base name as the base name of the given name
+	 */
+	public abstract List<DataType> getDataTypesByBaseName(String name);
+
 	/**
 	 * Adds the given datatype to this category.
 	 * @param dt the datatype to add to this category.