Skip to content

Commit 0536335

Browse files
ghidravoreghidravore
committed
GT-3535 improving data type category performance
1 parent 6de9613 commit 0536335

File tree

5 files changed

+216
-37
lines changed

5 files changed

+216
-37
lines changed

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

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -575,6 +575,37 @@ public void testDataTypeDeleted() throws Exception {
575575
assertEquals(comps.length - 1, newDt.getDefinedComponents().length);
576576
}
577577

578+
@Test
579+
public void testDataTypeConflictHandling() throws Exception {
580+
Category sub1 = root.createCategory("Cat1");
581+
DataType dt1 = new StructureDataType("DT", 1);
582+
DataType dt2 = new StructureDataType("DT", 2);
583+
DataType added1 = sub1.addDataType(dt1, null);
584+
DataType added2 = sub1.addDataType(dt2, null);
585+
assertEquals("DT", added1.getName());
586+
assertEquals("DT.conflict", added2.getName());
587+
588+
List<DataType> list = sub1.getDataTypesByBaseName("DT");
589+
assertEquals(2, list.size());
590+
assertEquals(added1, list.get(0));
591+
assertEquals(added2, list.get(1));
592+
593+
list = sub1.getDataTypesByBaseName("DT.conflict");
594+
assertEquals(2, list.size());
595+
assertEquals(added1, list.get(0));
596+
assertEquals(added2, list.get(1));
597+
598+
sub1.remove(added2, TaskMonitor.DUMMY);
599+
list = sub1.getDataTypesByBaseName("DT");
600+
assertEquals(1, list.size());
601+
assertEquals(added1, list.get(0));
602+
603+
list = sub1.getDataTypesByBaseName("DT.conflict");
604+
assertEquals(1, list.size());
605+
assertEquals(added1, list.get(0));
606+
607+
}
608+
578609
@Test
579610
public void testGetDataTypeManager() throws Exception {
580611
Category sub1 = root.createCategory("SubCat-A");
@@ -772,10 +803,7 @@ public void testListenerDataTypeChanged() throws Exception {
772803
clearEvents();
773804

774805
struct2 = (Structure) newDt.insert(3, struct2).getDataType();
775-
int eventCount = getEventCount();
776-
if (4 != eventCount) {
777-
System.err.println("halt!");
778-
}
806+
779807
assertEquals(5, getEventCount());
780808
Event ev = getEvent(4);
781809
assertEquals("DT Changed", ev.evName);

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

Lines changed: 145 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,7 @@
1616
package ghidra.program.database.data;
1717

1818
import java.io.IOException;
19-
import java.util.List;
20-
import java.util.Map;
19+
import java.util.*;
2120
import java.util.concurrent.ConcurrentHashMap;
2221

2322
import db.Record;
@@ -26,6 +25,7 @@
2625
import ghidra.program.model.data.*;
2726
import ghidra.program.model.data.DataTypeConflictHandler.ConflictResult;
2827
import ghidra.util.InvalidNameException;
28+
import ghidra.util.Lock;
2929
import ghidra.util.exception.AssertException;
3030
import ghidra.util.exception.DuplicateNameException;
3131
import ghidra.util.task.TaskMonitor;
@@ -41,6 +41,7 @@ class CategoryDB extends DatabaseObject implements Category {
4141

4242
private LazyLoadingCachingMap<String, CategoryDB> subcategoryMap;
4343
private LazyLoadingCachingMap<String, DataType> dataTypeMap;
44+
private ConflictMap conflictMap;
4445

4546
/**
4647
* Category Constructor
@@ -57,19 +58,19 @@ class CategoryDB extends DatabaseObject implements Category {
5758
this.name = name;
5859
this.parent = parent;
5960

60-
subcategoryMap = new LazyLoadingCachingMap<>(mgr.lock, CategoryDB.class) {
61+
subcategoryMap = new LazyLoadingCachingMap<>(mgr.lock) {
6162
@Override
6263
public Map<String, CategoryDB> loadMap() {
6364
return buildSubcategoryMap();
6465
}
6566
};
66-
dataTypeMap = new LazyLoadingCachingMap<>(mgr.lock, DataType.class) {
67+
dataTypeMap = new LazyLoadingCachingMap<>(mgr.lock) {
6768
@Override
6869
public Map<String, DataType> loadMap() {
6970
return createDataTypeMap();
7071
}
7172
};
72-
73+
conflictMap = new ConflictMap(mgr.lock);
7374
}
7475

7576
/**
@@ -102,6 +103,7 @@ protected boolean refresh() {
102103
protected boolean refresh(Record rec) {
103104
subcategoryMap.clear();
104105
dataTypeMap.clear();
106+
conflictMap.clear();
105107

106108
if (isRoot()) {
107109
return true;
@@ -210,13 +212,26 @@ private Map<String, DataType> createDataTypeMap() {
210212
return map;
211213
}
212214

215+
private String getBaseName(String dataTypeName) {
216+
int indexOf = dataTypeName.indexOf(DataType.CONFLICT_SUFFIX);
217+
if (indexOf <= 0) {
218+
return dataTypeName;
219+
}
220+
return dataTypeName.substring(0, indexOf);
221+
}
222+
223+
private boolean isConflictName(String dataTypeName) {
224+
return dataTypeName.contains(DataType.CONFLICT_SUFFIX);
225+
}
226+
213227
/**
214228
* @see ghidra.program.model.data.Category#getCategories()
215229
*/
216230
@Override
217231
public Category[] getCategories() {
218232
validate(mgr.lock);
219-
return subcategoryMap.valuesToArray();
233+
Collection<CategoryDB> categories = subcategoryMap.values();
234+
return categories.toArray(new Category[categories.size()]);
220235
}
221236

222237
/**
@@ -225,7 +240,8 @@ public Category[] getCategories() {
225240
@Override
226241
public DataType[] getDataTypes() {
227242
validate(mgr.lock);
228-
return dataTypeMap.valuesToArray();
243+
Collection<DataType> dataTypes = dataTypeMap.values();
244+
return dataTypes.toArray(new DataType[dataTypes.size()]);
229245
}
230246

231247
/**
@@ -587,19 +603,137 @@ private void catagoryRenamed(CategoryDB childCategory, String oldName) {
587603
}
588604

589605
void dataTypeRenamed(DataType childDataType, String oldName) {
590-
dataTypeMap.remove(oldName);
591-
dataTypeMap.put(childDataType.getName(), childDataType);
606+
dataTypeRemoved(oldName);
607+
dataTypeAdded(childDataType);
592608
}
593609

594-
void dataTypeAdded(DataType childDataType) {
595-
dataTypeMap.put(childDataType.getName(), childDataType);
610+
void dataTypeAdded(DataType dataType) {
611+
String dtName = dataType.getName();
612+
dataTypeMap.put(dtName, dataType);
613+
if (isConflictName(dtName)) {
614+
conflictMap.addDataType(dataType);
615+
}
596616
}
597617

598618
void dataTypeRemoved(String dataTypeName) {
599619
dataTypeMap.remove(dataTypeName);
620+
if (isConflictName(dataTypeName)) {
621+
conflictMap.removeDataTypeName(dataTypeName);
622+
}
600623
}
601624

602625
void categoryAdded(CategoryDB cat) {
603626
subcategoryMap.put(cat.getName(), cat);
604627
}
628+
629+
@Override
630+
public List<DataType> getDataTypesByBaseName(String dataTypeName) {
631+
List<DataType> list = new ArrayList<>();
632+
String baseName = getBaseName(dataTypeName);
633+
634+
DataType baseType = dataTypeMap.get(baseName);
635+
if (baseType != null) {
636+
list.add(baseType);
637+
}
638+
639+
List<DataType> relatedNameDataTypes = conflictMap.getDataTypesForBaseName(baseName);
640+
list.addAll(relatedNameDataTypes);
641+
return list;
642+
}
643+
644+
/**
645+
* Class to handle complexities of having a map as the value in a LazyLoadingCachingMap
646+
* This map uses data type's base name as the key (i.e. all .conflict suffixex stripped off.)
647+
* The value is another map that maps the actual data type's name to the datatype. This map
648+
* effectively provides an efficient way to get all data types in a category that have the
649+
* same name, but possibly have had their name modified (by appending .conflict) to get around
650+
* the requirement that names have to be unique in the same category.
651+
*/
652+
class ConflictMap extends LazyLoadingCachingMap<String, Map<String, DataType>> {
653+
654+
ConflictMap(Lock lock) {
655+
super(lock);
656+
}
657+
658+
/**
659+
* Creates a map of all data types whose name has a .conflict suffix where the key
660+
* is the base name and the value is a map of actual name to data type. This mapping is
661+
* maintained as a lazy cache map.
662+
* @return the loaded map
663+
*/
664+
@Override
665+
protected Map<String, Map<String, DataType>> loadMap() {
666+
Map<String, Map<String, DataType>> map = new HashMap<>();
667+
Collection<DataType> values = dataTypeMap.values();
668+
for (DataType dataType : values) {
669+
String dataTypeName = dataType.getName();
670+
if (isConflictName(dataTypeName)) {
671+
String baseName = getBaseName(dataTypeName);
672+
Map<String, DataType> innerMap = map.get(baseName);
673+
if (innerMap == null) {
674+
innerMap = new HashMap<>();
675+
map.put(baseName, innerMap);
676+
}
677+
innerMap.put(dataTypeName, dataType);
678+
}
679+
}
680+
return map;
681+
}
682+
683+
/**
684+
* Adds the data type to the conflict mapping structure. If the mapping is currently not
685+
* loaded then this method can safely do nothing.
686+
* @param dataType the data type to add to the mapping if the mapping is already loaded
687+
*/
688+
synchronized void addDataType(DataType dataType) {
689+
Map<String, Map<String, DataType>> map = getMap();
690+
if (map == null) {
691+
return;
692+
}
693+
694+
String dataTypeName = dataType.getName();
695+
String baseName = getBaseName(dataTypeName);
696+
Map<String, DataType> innerMap = map.get(baseName);
697+
if (innerMap == null) {
698+
innerMap = new HashMap<>();
699+
put(baseName, innerMap);
700+
}
701+
innerMap.put(dataTypeName, dataType);
702+
}
703+
704+
/**
705+
* Removes the data type with the given name from the conflict mapping structure. If the
706+
* mapping is currently not loaded then this method can safely do nothing.
707+
* @param dataTypeName the name of the data type to remove from this mapping
708+
*/
709+
synchronized void removeDataTypeName(String dataTypeName) {
710+
Map<String, Map<String, DataType>> map = getMap();
711+
if (map == null) {
712+
return;
713+
}
714+
String baseName = getBaseName(dataTypeName);
715+
Map<String, DataType> innerMap = map.get(baseName);
716+
if (innerMap == null) {
717+
return;
718+
}
719+
innerMap.remove(dataTypeName);
720+
}
721+
722+
/**
723+
* Returns a list of all data types that have conflict names for the given base name
724+
* @param baseName the data type base name to search for (i.e. the .conflict suffix removed)
725+
* @return a list of all conflict named data types that would have the given base name if
726+
* no conflicts existed
727+
*/
728+
List<DataType> getDataTypesForBaseName(String baseName) {
729+
Map<String, DataType> map = get(baseName);
730+
if (map == null) {
731+
return Collections.emptyList();
732+
}
733+
synchronized (this) {
734+
return new ArrayList<>(map.values());
735+
}
736+
}
737+
738+
}
605739
}

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

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -934,14 +934,12 @@ private DataType findEquivalentDataTypeSameLocation(DataType dataType, String ex
934934
if (category == null) {
935935
return null;
936936
}
937-
String namePrefix = dtName + DataType.CONFLICT_SUFFIX;
938-
DataType[] dataTypes = category.getDataTypes();
939-
for (DataType candidate : dataTypes) {
937+
List<DataType> relatedByName = category.getDataTypesByBaseName(dtName);
938+
939+
for (DataType candidate : relatedByName) {
940940
String candidateName = candidate.getName();
941-
if (candidateName.startsWith(namePrefix)) {
942-
if (!candidateName.equals(excludedName) && candidate.isEquivalent(dataType)) {
943-
return candidate;
944-
}
941+
if (!candidateName.equals(excludedName) && candidate.isEquivalent(dataType)) {
942+
return candidate;
945943
}
946944
}
947945
return null;
@@ -3210,11 +3208,11 @@ List<DataType> getParentDataTypes(long childID) {
32103208
long[] ids = parentChildAdapter.getParentIds(childID);
32113209
// TODO: consider deduping ids using Set
32123210
List<DataType> dts = new ArrayList<>();
3213-
for (int i = 0; i < ids.length; i++) {
3214-
DataType dt = getDataType(ids[i]);
3211+
for (long id : ids) {
3212+
DataType dt = getDataType(id);
32153213
if (dt == null) {
32163214
// cleanup invalid records for missing parent
3217-
attemptRecordRemovalForParent(ids[i]);
3215+
attemptRecordRemovalForParent(id);
32183216
}
32193217
else {
32203218
dts.add(dt);

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

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,7 @@
1616
package ghidra.program.database.data;
1717

1818
import java.lang.ref.SoftReference;
19-
import java.lang.reflect.Array;
20-
import java.util.Map;
19+
import java.util.*;
2120

2221
import ghidra.util.Lock;
2322

@@ -39,11 +38,9 @@ public abstract class LazyLoadingCachingMap<K, V> {
3938

4039
private Lock lock;
4140
private SoftReference<Map<K, V>> softRef;
42-
private Class<V> valueClass;
4341

44-
protected LazyLoadingCachingMap(Lock lock, Class<V> valueClass) {
42+
protected LazyLoadingCachingMap(Lock lock) {
4543
this.lock = lock;
46-
this.valueClass = valueClass;
4744
}
4845

4946
/**
@@ -96,13 +93,13 @@ public V get(K key) {
9693
}
9794
}
9895

99-
public V[] valuesToArray() {
96+
/**
97+
* Returns an unmodifiable view of the values in this map.
98+
* @return an unmodifiable view of the values in this map.
99+
*/
100+
public Collection<V> values() {
100101
Map<K, V> map = getOrLoadMap();
101-
synchronized (this) {
102-
@SuppressWarnings("unchecked")
103-
V[] array = (V[]) Array.newInstance(valueClass, map.size());
104-
return map.values().toArray(array);
105-
}
102+
return Collections.unmodifiableCollection(map.values());
106103
}
107104

108105
private Map<K, V> getOrLoadMap() {
@@ -113,6 +110,14 @@ private Map<K, V> getOrLoadMap() {
113110
return map;
114111
}
115112
}
113+
114+
// We must get the database lock before calling loadMap(). Also, we can't get the
115+
// database lock while having the synchronization lock for this class or a deadlock can
116+
// occur. Note: all other places where the map is being used or manipulated, it must be done
117+
// while having the class's synchronization lock since the map itself is not thread safe.
118+
// It should be safe here since it creates a new map and then in one operation it sets it
119+
// as the map to be used elsewhere.
120+
116121
lock.acquire();
117122
try {
118123
map = getMap();
@@ -132,11 +137,12 @@ private Map<K, V> getOrLoadMap() {
132137
* "lock".
133138
* @return the underlying map of key,value pairs or null if it is currently not loaded.
134139
*/
135-
private Map<K, V> getMap() {
140+
protected Map<K, V> getMap() {
136141
if (softRef == null) {
137142
return null;
138143
}
139144
return softRef.get();
140145
}
141146

147+
142148
}

0 commit comments

Comments
 (0)