GP-4534 Remove memory map address set cache lock contention

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/MemoryMapDB.java

@@ -135,14 +135,21 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 		}
 	}
 
-	void buildAddressSets() {
+	MemoryAddressSetView buildAddressSets() {
+		if (addrSetView != null) {
+			return addrSetView;
+		}
 		lock.acquire();
 		try {
-			// null addrSet signals address sets must be built otherwise return
+			// have to try and get it again, another thread may have already filled it out
 			if (addrSetView != null) {
-				return;
+				return addrSetView;
 			}
-			addrSetView = new MemoryAddressSetView();
+			
+			// set cached addressSet view to null, so other threads will end up here waiting
+			// on the above lock if they try to access the cached address sets
+			addrSetView = null;
+			MemoryAddressSetView newAddrSetView = new MemoryAddressSetView();
 
 			// The allAddrSet instance is generally maintained with all memory
 			// block addresses and need only be updated if currently empty.
@@ -155,15 +162,18 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 			for (MemoryBlockDB block : blocks) {
 				block.clearMappedBlockList();
 				if (!block.isMapped()) {
-					addBlockAddresses(block, addToAll);
+					addBlockAddresses(newAddrSetView, block, addToAll);
 				}
 			}
 			// process all mapped blocks after non-mapped-blocks above
 			for (MemoryBlockDB block : blocks) {
 				if (block.isMapped()) {
-					addBlockAddresses(block, addToAll);
+					addBlockAddresses(newAddrSetView, block, addToAll);
 				}
 			}
+			addrSetView = newAddrSetView;
+			
+			return addrSetView;
 		}
 		finally {
 			lock.release();
@@ -177,17 +187,17 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 	 * @param block memory block
 	 * @param addToAll if true the allAddrSet should be updated with the specified block's address range
 	 */
-	private void addBlockAddresses(MemoryBlockDB block, boolean addToAll) {
+	private void addBlockAddresses(MemoryAddressSetView newSet, MemoryBlockDB block, boolean addToAll) {
 		Address start = block.getStart();
 		Address end = block.getEnd();
 		if (addToAll) {
 			allAddrSet.add(start, end);
 		}
 		if (block.isExternalBlock()) {
-			addrSetView.externalBlock.add(start, end);
+			newSet.externalBlock.add(start, end);
 		}
 		else if (block.isExecute()) {
-			addrSetView.execute.add(start, end);
+			newSet.execute.add(start, end);
 		}
 		if (block.isMapped()) {
 			// Identify source-blocks which block maps onto and add as a mapped-block to each of these
@@ -198,15 +208,15 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 					b.addMappedBlock(block);
 				}
 			}
-			AddressSet mappedSet = getMappedIntersection(block, addrSetView.initialized);
-			addrSetView.initialized.add(mappedSet);
-			addrSetView.initializedAndLoaded
-					.add(getMappedIntersection(block, addrSetView.initializedAndLoaded));
+			AddressSet mappedSet = getMappedIntersection(block, newSet.initialized);
+			newSet.initialized.add(mappedSet);
+			newSet.initializedAndLoaded
+					.add(getMappedIntersection(block, newSet.initializedAndLoaded));
 		}
 		else if (block.isInitialized()) {
-			addrSetView.initialized.add(block.getStart(), block.getEnd());
+			newSet.initialized.add(block.getStart(), block.getEnd());
 			if (block.isLoaded()) {
-				addrSetView.initializedAndLoaded.add(block.getStart(), block.getEnd());
+				newSet.initializedAndLoaded.add(block.getStart(), block.getEnd());
 			}
 		}
 	}
@@ -312,16 +322,8 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 
 	@Override
 	public AddressSetView getAllInitializedAddressSet() {
-		lock.acquire();
-		try {
-			if (addrSetView == null) {
-				buildAddressSets();
-			}
-			return new AddressSetViewAdapter(addrSetView.initialized);
-		}
-		finally {
-			lock.release();
-		}
+		MemoryAddressSetView localAddrSetView = buildAddressSets();
+		return new AddressSetViewAdapter(localAddrSetView.initialized);
 	}
 
 	@Override
@@ -329,16 +331,21 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 		if (liveMemory != null) {
 			return this; // all memory is initialized!
 		}
-		lock.acquire();
-		try {
-			if (addrSetView == null) {
-				buildAddressSets();
-			}
-			return new AddressSetViewAdapter(addrSetView.initializedAndLoaded);
-		}
-		finally {
-			lock.release();
-		}
+		
+		MemoryAddressSetView localAddrSetView = buildAddressSets();
+		return new AddressSetViewAdapter(localAddrSetView.initializedAndLoaded);
+	}
+	
+	@Override
+	public boolean isExternalBlockAddress(Address addr) {
+		MemoryAddressSetView localAddrSetView = buildAddressSets();
+		return localAddrSetView.externalBlock.contains(addr);
+	}
+	
+	@Override
+	public AddressSetView getExecuteSet() {
+		MemoryAddressSetView localAddrSetView = buildAddressSets();
+		return new AddressSetViewAdapter(localAddrSetView.execute);
 	}
 
 	void checkMemoryWrite(MemoryBlockDB block, Address start, long length)
@@ -920,13 +927,7 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 
 	@Override
 	public long getSize() {
-		lock.acquire();
-		try {
-			return allAddrSet.getNumAddresses();
-		}
-		finally {
-			lock.release();
-		}
+		return allAddrSet.getNumAddresses();
 	}
 
 	@Override
@@ -1404,22 +1405,15 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 
 	@Override
 	public byte getByte(Address addr) throws MemoryAccessException {
-		lock.acquire();
-		try {
-			if (liveMemory != null) {
-				return liveMemory.getByte(addr);
-			}
-			MemoryBlock block = getBlockDB(addr);
-			if (block == null) {
-				throw new MemoryAccessException(
-					"Address " + addr.toString(true) + " does not exist in memory");
-			}
-			return block.getByte(addr);
+		if (liveMemory != null) {
+			return liveMemory.getByte(addr);
 		}
-		finally {
-			lock.release();
+		MemoryBlock block = getBlockDB(addr);
+		if (block == null) {
+			throw new MemoryAccessException(
+				"Address " + addr.toString(true) + " does not exist in memory");
 		}
-
+		return block.getByte(addr);
 	}
 
 	@Override
@@ -1804,13 +1798,7 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 
 	@Override
 	public boolean contains(Address addr) {
-		lock.acquire();
-		try {
-			return allAddrSet.contains(addr);
-		}
-		finally {
-			lock.release();
-		}
+		return allAddrSet.contains(addr);
 	}
 
 	@Override
@@ -1828,20 +1816,6 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 		return allAddrSet.isEmpty();
 	}
 
-	@Override
-	public boolean isExternalBlockAddress(Address addr) {
-		lock.acquire();
-		try {
-			if (addrSetView == null) {
-				buildAddressSets();
-			}
-			return addrSetView.externalBlock.contains(addr);
-		}
-		finally {
-			lock.release();
-		}
-	}
-
 	@Override
 	public Address getMinAddress() {
 		return allAddrSet.getMinAddress();
@@ -2147,20 +2121,6 @@ public class MemoryMapDB implements Memory, ManagerDB, LiveMemoryListener {
 		return super.hashCode();
 	}
 
-	@Override
-	public AddressSetView getExecuteSet() {
-		lock.acquire();
-		try {
-			if (addrSetView == null) {
-				buildAddressSets();
-			}
-			return new AddressSetViewAdapter(addrSetView.execute);
-		}
-		finally {
-			lock.release();
-		}
-	}
-
 	@Override
 	public void memoryChanged(Address addr, int size) {
 		fireBytesChanged(addr, size);

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/RecoverableAddressIterator.java

@@ -40,10 +40,12 @@ import ghidra.program.model.address.*;
  */
 class RecoverableAddressIterator implements AddressIterator {
 
-	private AddressSetView set;
+	private SynchronizedAddressSet synchSet;
+	private AddressSet internalSet;
 	private boolean forward;
 	private AddressIterator iterator;
 	private Address next;
+	private int changeID;
 
 	/**
 	 * Construct iterator
@@ -51,8 +53,10 @@ class RecoverableAddressIterator implements AddressIterator {
 	 * @param start address to start iterating at in the address set or null for all addresses
 	 * @param forward if true address are return from lowest to highest, else from highest to lowest
 	 */
-	RecoverableAddressIterator(AddressSetView set, Address start, boolean forward) {
-		this.set = set;
+	RecoverableAddressIterator(SynchronizedAddressSet set, Address start, boolean forward) {
+		this.synchSet = set;
+		this.internalSet = set.getInternalSet();
+		changeID = set.getModificationID();
 		this.forward = forward;
 		initIterator(start);
 		this.next = iterator.next();
@@ -60,10 +64,10 @@ class RecoverableAddressIterator implements AddressIterator {
 
 	private void initIterator(Address start) {
 		if (start == null) {
-			iterator = set.getAddresses(forward);
+			iterator = internalSet.getAddresses(forward);
 		}
 		else {
-			iterator = set.getAddresses(start, forward);
+			iterator = internalSet.getAddresses(start, forward);
 		}
 	}
 
@@ -77,9 +81,14 @@ class RecoverableAddressIterator implements AddressIterator {
 		Address addr = next;
 		if (addr != null) {
 			try {
-				next = iterator.next();
+				if (synchSet.hasChanged(changeID)) {
+					next = recoverNext(addr);
+				} else {
+					next = iterator.next();
+				}
 			}
 			catch (ConcurrentModificationException e) {
+				// will never happen, set in hand is never changed
 				next = recoverNext(addr);
 			}
 		}
@@ -87,6 +96,8 @@ class RecoverableAddressIterator implements AddressIterator {
 	}
 
 	private Address recoverNext(Address lastAddr) {
+		changeID = synchSet.getModificationID();
+		internalSet = synchSet.getInternalSet();
 		while (true) {
 			try {
 				initIterator(lastAddr);
@@ -97,6 +108,7 @@ class RecoverableAddressIterator implements AddressIterator {
 				return a;
 			}
 			catch (ConcurrentModificationException e) {
+				// will never happen, set in hand is never changed
 				// set must have changed - try re-initializing again
 			}
 		}

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/RecoverableAddressRangeIterator.java

@@ -39,10 +39,12 @@ import ghidra.program.model.address.*;
  */
 class RecoverableAddressRangeIterator implements AddressRangeIterator {
 
-	private AddressSetView set;
+	private SynchronizedAddressSet synchSet;
+	private AddressSet internalSet;
 	private boolean forward;
 	private AddressRangeIterator iterator;
 	private AddressRange next;
+	private int changeID;
 
 	/**
 	 * Construct iterator
@@ -50,8 +52,10 @@ class RecoverableAddressRangeIterator implements AddressRangeIterator {
 	 * @param start the address the the first range should contain.
 	 * @param forward true iterators forward, false backwards
 	 */
-	RecoverableAddressRangeIterator(AddressSetView set, Address start, boolean forward) {
-		this.set = set;
+	RecoverableAddressRangeIterator(SynchronizedAddressSet set, Address start, boolean forward) {
+		this.synchSet = set;
+		this.internalSet = set.getInternalSet();
+		changeID = set.getModificationID();
 		this.forward = forward;
 		initIterator(start);
 		try {
@@ -64,10 +68,10 @@ class RecoverableAddressRangeIterator implements AddressRangeIterator {
 
 	private void initIterator(Address start) {
 		if (start == null) {
-			iterator = set.getAddressRanges(forward);
+			iterator = internalSet.getAddressRanges(forward);
 		}
 		else {
-			iterator = set.getAddressRanges(start, forward);
+			iterator = internalSet.getAddressRanges(start, forward);
 		}
 	}
 
@@ -83,9 +87,14 @@ class RecoverableAddressRangeIterator implements AddressRangeIterator {
 			throw new NoSuchElementException();
 		}
 		try {
-			next = iterator.next();
+			if (synchSet.hasChanged(changeID)) {
+				next = recoverNext(range);
+			} else {
+				next = iterator.next();
+			}
 		}
 		catch (ConcurrentModificationException e) {
+			// will never happen, set in hand is never changed
 			next = recoverNext(range);
 		}
 		catch (NoSuchElementException e) {
@@ -95,6 +104,8 @@ class RecoverableAddressRangeIterator implements AddressRangeIterator {
 	}
 
 	private AddressRange recoverNext(AddressRange lastRange) {
+		changeID = synchSet.getModificationID();
+		internalSet = synchSet.getInternalSet();
 		while (true) {
 			try {
 				Address lastAddr = forward ? lastRange.getMaxAddress() : lastRange.getMinAddress();
@@ -114,6 +125,7 @@ class RecoverableAddressRangeIterator implements AddressRangeIterator {
 				return iterator.next(); // skip range and return next
 			}
 			catch (ConcurrentModificationException e) {
+				// will never happen, set in hand is never changed
 				// set must have changed - try re-initializing again
 			}
 			catch (NoSuchElementException e) {

Ghidra/Framework/SoftwareModeling/src/main/java/ghidra/program/database/mem/SynchronizedAddressSet.java

@@ -28,10 +28,20 @@ import ghidra.program.model.address.*;
 class SynchronizedAddressSet implements AddressSetView {
 
 	private AddressSet set;
+	private int modificationID = 1; // updated if set above ever replaced
 
 	SynchronizedAddressSet() {
 		set = new AddressSet();
 	}
+	
+	/**
+	 * get the modification number of the internal address set
+	 * If the underlying set if ever replaced, modification id is changed
+	 * @return current modification id
+	 */
+	int getModificationID() {
+		return modificationID;
+	}
 
 	/**
 	 * Add all addresses of the given AddressSet to this set.
@@ -39,7 +49,10 @@ class SynchronizedAddressSet implements AddressSetView {
 	 * @see AddressSet#add(AddressSetView)
 	 */
 	synchronized void add(AddressSet addrSet) {
-		set.add(addrSet);
+		AddressSet newSet = new AddressSet(set);
+		newSet.add(addrSet);
+		set = newSet;
+		modificationID++;
 	}
 
 	/**
@@ -49,7 +62,10 @@ class SynchronizedAddressSet implements AddressSetView {
 	 * @see AddressSet#add(Address, Address)
 	 */
 	synchronized void add(Address start, Address end) {
-		set.add(start, end);
+		AddressSet newSet = new AddressSet(set);
+		newSet.add(start, end);
+		set = newSet;
+		modificationID++;
 	}
 
 	/**
@@ -59,57 +75,75 @@ class SynchronizedAddressSet implements AddressSetView {
 	 * @see AddressSet#delete(Address, Address)
 	 */
 	synchronized void delete(Address start, Address end) {
-		set.delete(start, end);
+		AddressSet newSet = new AddressSet(set);
+		newSet.delete(start, end);
+		set = newSet;
+		modificationID++;
+	}
+	
+	synchronized AddressSet getInternalSet() {
+		return set;
+	}
+	
+	/**
+	 * Check if the internal set has been modified
+	 * If the mod id is different, then the set has changed.
+	 * 
+	 * @param modID modification id to check
+	 * @return true if internal mod id is different
+	 */
+	public boolean hasChanged(int modID) {
+		return modID != modificationID;
 	}
 
 	@Override
-	public synchronized boolean contains(Address addr) {
+	public boolean contains(Address addr) {
 		return set.contains(addr);
 	}
 
 	@Override
-	public synchronized boolean contains(Address start, Address end) {
+	public boolean contains(Address start, Address end) {
 		return set.contains(start, end);
 	}
 
 	@Override
-	public synchronized boolean contains(AddressSetView addrSet) {
+	public boolean contains(AddressSetView addrSet) {
 		return set.contains(addrSet);
 	}
 
 	@Override
-	public synchronized boolean isEmpty() {
+	public boolean isEmpty() {
 		return set.isEmpty();
 	}
 
 	@Override
-	public synchronized Address getMinAddress() {
+	public Address getMinAddress() {
 		return set.getMinAddress();
 	}
 
 	@Override
-	public synchronized Address getMaxAddress() {
+	public Address getMaxAddress() {
 		return set.getMaxAddress();
 	}
 
 	@Override
-	public synchronized int getNumAddressRanges() {
+	public int getNumAddressRanges() {
 		return set.getNumAddressRanges();
 	}
 
 	@Override
-	public synchronized AddressRangeIterator getAddressRanges() {
+	public AddressRangeIterator getAddressRanges() {
 		return set.getAddressRanges();
 	}
 
 	@Override
-	public synchronized AddressRangeIterator getAddressRanges(boolean forward) {
+	public AddressRangeIterator getAddressRanges(boolean forward) {
 		return set.getAddressRanges(forward);
 	}
 
 	@Override
 	public synchronized AddressRangeIterator getAddressRanges(Address start, boolean forward) {
-		return new RecoverableAddressRangeIterator(set, start, forward);
+		return new RecoverableAddressRangeIterator(this, start, forward);
 	}
 
 	@Override
@@ -118,103 +152,102 @@ class SynchronizedAddressSet implements AddressSetView {
 	}
 
 	@Override
-	public synchronized Iterator<AddressRange> iterator(boolean forward) {
+	public Iterator<AddressRange> iterator(boolean forward) {
 		return set.getAddressRanges(forward);
 	}
 
 	@Override
-	public synchronized Iterator<AddressRange> iterator(Address start, boolean forward) {
+	public Iterator<AddressRange> iterator(Address start, boolean forward) {
 		return set.getAddressRanges(start, forward);
 	}
 
 	@Override
-	public synchronized long getNumAddresses() {
+	public long getNumAddresses() {
 		return set.getNumAddresses();
 	}
 
 	@Override
 	public synchronized AddressIterator getAddresses(boolean forward) {
-		return new RecoverableAddressIterator(set, null, forward);
+		return new RecoverableAddressIterator(this, null, forward);
 	}
 
 	@Override
 	public synchronized AddressIterator getAddresses(Address start, boolean forward) {
-		return new RecoverableAddressIterator(set, start, forward);
+		return new RecoverableAddressIterator(this, start, forward);
 	}
 
 	@Override
-	public synchronized boolean intersects(AddressSetView addrSet) {
+	public boolean intersects(AddressSetView addrSet) {
 		return set.intersects(addrSet);
 	}
 
 	@Override
-	public synchronized boolean intersects(Address start, Address end) {
+	public boolean intersects(Address start, Address end) {
 		return set.intersects(start, end);
 	}
 
 	@Override
-	public synchronized AddressSet intersect(AddressSetView addrSet) {
+	public AddressSet intersect(AddressSetView addrSet) {
 		return set.intersect(addrSet);
 	}
 
 	@Override
-	public synchronized AddressSet intersectRange(Address start, Address end) {
+	public AddressSet intersectRange(Address start, Address end) {
 		return set.intersectRange(start, end);
 	}
 
 	@Override
-	public synchronized AddressSet union(AddressSetView addrSet) {
+	public AddressSet union(AddressSetView addrSet) {
 		return set.union(addrSet);
 	}
 
 	@Override
-	public synchronized AddressSet subtract(AddressSetView addrSet) {
+	public AddressSet subtract(AddressSetView addrSet) {
 		return set.subtract(addrSet);
 	}
 
 	@Override
-	public synchronized AddressSet xor(AddressSetView addrSet) {
+	public AddressSet xor(AddressSetView addrSet) {
 		return set.xor(addrSet);
 	}
 
 	@Override
-	public synchronized boolean hasSameAddresses(AddressSetView addrSet) {
+	public boolean hasSameAddresses(AddressSetView addrSet) {
 		return set.hasSameAddresses(addrSet);
 	}
 
 	@Override
-	public synchronized AddressRange getFirstRange() {
+	public AddressRange getFirstRange() {
 		return set.getFirstRange();
 	}
 
 	@Override
-	public synchronized AddressRange getLastRange() {
+	public AddressRange getLastRange() {
 		return set.getLastRange();
 	}
 
 	@Override
-	public synchronized AddressRange getRangeContaining(Address address) {
+	public AddressRange getRangeContaining(Address address) {
 		return set.getRangeContaining(address);
 	}
 
 	@Override
-	public synchronized Address findFirstAddressInCommon(AddressSetView addrSet) {
+	public Address findFirstAddressInCommon(AddressSetView addrSet) {
 		return set.findFirstAddressInCommon(addrSet);
 	}
 
 	@Override
-	public synchronized int hashCode() {
+	public int hashCode() {
 		return set.hashCode();
 	}
 
 	@Override
-	public synchronized boolean equals(Object obj) {
+	public boolean equals(Object obj) {
 		return set.equals(obj);
 	}
 
 	@Override
-	public synchronized String toString() {
+	public String toString() {
 		return set.toString();
 	}
-
 }