GP-6947 improving performance of database text searching

2026-09-25 08:55:56 +08:00 · 2026-06-25 16:42:04 -04:00
parent 8711646775
commit 4154db3aaa
7 changed files with 232 additions and 106 deletions
@@ -102,6 +102,7 @@ public class SearchTextPlugin extends ProgramPlugin implements OptionsChangeList
 	private int searchLimit;
 	private SearchTask currentTask;
 	private String lastSearchedText;
+	private LastSearchHit lastSearchHit;
 	private boolean doHighlight;
 	private Navigatable navigatable;

@@ -150,15 +151,18 @@ public class SearchTextPlugin extends ProgramPlugin implements OptionsChangeList
 		if (result == null) {
 			searchDialog.setStatusText("Not found");
 		}
-		else if (result.programLocation().equals(currentLocation)) {
-			searchNext(searchTask.getProgram(), searchNavigatable, textSearcher);
-		}
 		else {
 			searchDialog.setStatusText("");
 			ProgramLocation loc = result.programLocation();
 			if (goToService.goTo(searchNavigatable, loc, program)) {
 				new SearchTextHighlightProvider(searchNavigatable, searchOptions, null, program,
 					result);
+
+				// The navigatable may change its location if it does not have the search result
+				// location visible.  We store that here so we can later detect that case in order
+				// to keep the search from getting stuck.
+				ProgramLocation navigatableLoc = navigatable.getLocation();
+				lastSearchHit = new LastSearchHit(loc, navigatableLoc);
 			}
 		}

@@ -262,7 +266,20 @@ public class SearchTextPlugin extends ProgramPlugin implements OptionsChangeList
 	}

 	private ProgramLocation getStartLocation() {
-		return currentLocation = navigatable.getLocation();
+
+		ProgramLocation currentNavLoc = navigatable.getLocation();
+		if (lastSearchHit != null) {
+			ProgramLocation lastNavLoc = lastSearchHit.navigatableLocation();
+
+			// The navigatable's location has not changed since the last search. Use the last search
+			// location as the start point for the next search.  This ensures the searching does not
+			// get stuck when the navigatable cannot display the last search hit.
+			if (lastNavLoc.equals(currentNavLoc)) {
+				return lastSearchHit.searchLocation();
+			}
+		}
+
+		return currentNavLoc;
 	}

 	private void searchNext(Program program, Navigatable searchNavigatable, Searcher textSearcher) {
@@ -534,7 +551,12 @@ public class SearchTextPlugin extends ProgramPlugin implements OptionsChangeList
 // Inner Classes
 //==================================================================================================

-	class TableLoadingListener implements ThreadedTableModelListener {
+	private record LastSearchHit(ProgramLocation searchLocation,
+			ProgramLocation navigatableLocation) {
+		//
+	}
+
+	private class TableLoadingListener implements ThreadedTableModelListener {

 		private ThreadedTableModel<?, ?> model;
 		private TableComponentProvider<ProgramLocation> provider;
@@ -572,8 +594,9 @@ public class SearchTextPlugin extends ProgramPlugin implements OptionsChangeList
 					"Stopped search after finding " + matchCount + " matches.\n" +
 						"The search limit can be changed at Edit->Tool Options, under Search.");
 			}
+
 			// there was a suggestion that the dialog should not go way after a search all
-//			searchDialog.close();
+			// searchDialog.close();
 		}

 		private Component getParentComponent() {
@@ -0,0 +1,100 @@
+/* ###
+ * IP: GHIDRA
+ *
+ * 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.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package ghidra.app.plugin.core.searchtext.databasesearcher;
+
+import ghidra.program.model.address.*;
+import ghidra.program.model.listing.*;
+
+/**
+ * Shared supplier of comments at addresses as they are traversed in address order (or reverse
+ * address order). 
+ * <p>
+ * This object is used as a part of a larger search operation and is shared by the various comment
+ * searchers for efficiency since all comment types are actually stored in the same database
+ * record.
+ * <p>
+ * At any given time, this supplier has a current address, which represents the next address at
+ * or beyond the overall search operation's address. When asked to advance, a current search
+ * address is passed, which is the last address that has been fully searched. So if our current
+ * address is at that address, we need to advance our current address to the next comment address.
+ * Otherwise, we simply return our current address which the higher level search can use
+ * to determine the next overall search address.
+ */
+public class CommentAddressSupplier {
+
+	private Address currentAddress;
+	private CodeUnitComments currentComments;
+	private AddressIterator iterator;
+	private Listing listing;
+
+	public CommentAddressSupplier(Program program, AddressSetView addresses, boolean forward) {
+		listing = program.getListing();
+		iterator = listing.getCommentAddressIterator(addresses, forward);
+		doAdvance();
+	}
+
+	/**
+	 * {@return the address of the currently available comments}
+	 */
+	public Address getCurrentAddress() {
+		return currentAddress;
+	}
+
+	/**
+	 * Returns the comment of the specified type at the current address or null if no comment of
+	 * that type exists at the current address.
+	 * @param type the type of comment to retrieve
+	 * @return the comment of the specified type at the current address
+	 */
+	public String getCurrentComment(CommentType type) {
+		if (currentComments != null) {
+			return currentComments.getComment(type);
+		}
+		return null;
+	}
+
+	/**
+	 * Advance the current address to the next address that contains any type of comment if the 
+	 * passed in address is null or equal to the current address. The idea is that we are part
+	 * of a larger search operation that is marching through the address space. We only want to 
+	 * advance our current address if the overall address of the search matches our address.
+	 * (meaning we have already served up comments for the given address so we can advance to the
+	 * next address that has comments, but may not be the next address of the overall search
+	 * operation is considering.)
+	 * @param address the address that has been already processed, so we need to make sure our
+	 * current address is past this address.
+	 * @return an address that is past the given address. Could be our current address our 
+	 * address hasn't been reached yet, or we search forward to the next address containing a
+	 * comment.
+	 */
+	public Address advance(Address address) {
+		if (address != null && address.equals(currentAddress)) {
+			doAdvance();
+		}
+		return currentAddress;
+	}
+
+	private void doAdvance() {
+		if (iterator.hasNext()) {
+			currentAddress = iterator.next();
+			currentComments = listing.getAllComments(currentAddress);
+		}
+		else {
+			currentAddress = null;
+			currentComments = null;
+		}
+	}
+}
@@ -21,64 +21,62 @@ import java.util.regex.Pattern;

 import ghidra.app.plugin.core.searchtext.Searcher.TextSearchResult;
 import ghidra.app.util.viewer.field.CommentUtils;
-import ghidra.program.model.address.*;
+import ghidra.program.model.address.Address;
+import ghidra.program.model.address.AddressSetView;
 import ghidra.program.model.listing.*;
 import ghidra.program.util.*;
 import ghidra.util.StringUtilities;

+/**
+ * Field searcher for comments. Since all comments types for an address are stored in the same
+ * database record, comment searchers share a common {@link CommentAddressSupplier} which the
+ * overall searcher will manually advance. The advance here will just assume the the comment
+ * supplier has been advanced and it will just return the suppliers current address.
+ */
 public class CommentFieldSearcher extends ProgramDatabaseFieldSearcher {
-	private AddressIterator iterator;
-	private final CommentType commentType;
+	private CommentType commentType;
+	private CommentAddressSupplier supplier;
 	private Program program;

-	public CommentFieldSearcher(Program program, ProgramLocation startLoc, AddressSetView set,
-			boolean forward, Pattern pattern, CommentType commentType) {
+	public CommentFieldSearcher(CommentAddressSupplier supplier, Program program,
+			ProgramLocation startLoc,
+			AddressSetView set, boolean forward, Pattern pattern, CommentType commentType) {

 		super(pattern, forward, startLoc, set);
-		this.commentType = commentType;
+		this.supplier = supplier;
 		this.program = program;
-		if (set != null) {
-			iterator = program.getListing().getCommentAddressIterator(commentType, set, forward);
-		}
-		else {
-			AddressSetView addressSet = program.getMemory();
-			if (forward) {
-				addressSet.intersectRange(startLoc.getAddress(), addressSet.getMaxAddress());
-			}
-			else {
-				addressSet.intersectRange(addressSet.getMinAddress(), startLoc.getAddress());
-			}
-			iterator =
-				program.getListing().getCommentAddressIterator(commentType, addressSet, forward);
-		}
+		this.commentType = commentType;
 	}

 	@Override
 	protected Address advance(List<TextSearchResult> currentMatches) {
-		Address nextAddress = iterator.next();
-		if (nextAddress != null) {
-			findMatchesForCurrentAddress(nextAddress, currentMatches);
+		Address address = supplier.getCurrentAddress();
+		if (address == null) {
+			return null;	// we are at the end of the iterator
 		}
-		return nextAddress;
+		findMatchesForCurrentAddress(address, currentMatches);
+		return address;
 	}

 	private void findMatchesForCurrentAddress(Address address,
 			List<TextSearchResult> currentMatches) {
-		String comment = program.getListing().getComment(commentType, address);
+		String comment = supplier.getCurrentComment(commentType);
 		if (comment == null) {
 			return;
 		}

 		// Remove newlines; turn any annotations into the display version so the screen positions
 		// of the program locations work correctly.
-		String cleanedUpComment = comment.replace('\n', ' ');
+		String cleanedUpComment = comment.replaceAll("\n", "");
 		String updatedLine = CommentUtils.getDisplayString(cleanedUpComment, program);

 		Matcher matcher = pattern.matcher(updatedLine);
-		while (matcher.find()) {
+		int pos = 0;
+		while (matcher.find(pos)) {
 			int index = matcher.start();
 			ProgramLocation commentLocation = getCommentLocation(comment, index, address);
 			currentMatches.add(new TextSearchResult(commentLocation, index));
+			pos = index + 1;
 		}
 	}

@@ -97,7 +95,7 @@ public class CommentFieldSearcher extends ProgramDatabaseFieldSearcher {
 					comments, rowIndex);
 			case REPEATABLE:
 				return new RepeatableCommentFieldLocation(program, address, dataPath, comments,
-					rowIndex, charOffset, rowIndex); // TODO One of searchStrIndex parameters is wrong.
+					rowIndex, charOffset, rowIndex);
 			case POST:
 				return new PostCommentFieldLocation(program, address, dataPath, comments, rowIndex,
 					charOffset);
@@ -64,6 +64,7 @@ public class ProgramDatabaseSearcher implements Searcher {
 	private long totalSearchCount;
 	private AddressSet remainingAddresses;
 	private TaskMonitor monitor;
+	private CommentAddressSupplier commentSupplier;

 	public ProgramDatabaseSearcher(ServiceProvider serviceProvider, Program program,
 			ProgramLocation startLoc, AddressSetView set, SearchOptions options,
@@ -131,7 +132,7 @@ public class ProgramDatabaseSearcher implements Searcher {
 	}

 	private Address findNextSignificantAddress() {
-		Address nextAddress = null;
+		Address nextAddress = commentSupplier.advance(currentAddress);
 		for (ProgramDatabaseFieldSearcher searcher : searchers) {
 			if (monitor.isCancelled()) {
 				return null;
@@ -179,17 +180,22 @@ public class ProgramDatabaseSearcher implements Searcher {
 			UserSearchUtils.createSearchPattern(options.getText(), options.isCaseSensitive());
 		BrowserCodeUnitFormat format = new BrowserCodeUnitFormat(serviceProvider, false);

+		// this comment supplier will be used in all the comment searchers and is explicitly 
+		// advanced in the findNextSignificantAddress() method
+		commentSupplier = new CommentAddressSupplier(program, trimmedSet, forward);
+
+		// create the searchers in the order as displayed in the default listing panel field layout
 		if (options.searchComments()) {
-			searchers.add(new CommentFieldSearcher(program, adjustedStart, trimmedSet, forward,
-				pattern, CommentType.PLATE));
+			searchers.add(new CommentFieldSearcher(commentSupplier, program, adjustedStart,
+				trimmedSet, forward, pattern, CommentType.PLATE));
 		}
 		if (options.searchFunctions()) {
 			searchers.add(
 				new FunctionFieldSearcher(program, adjustedStart, trimmedSet, forward, pattern));
 		}
 		if (options.searchComments()) {
-			searchers.add(new CommentFieldSearcher(program, adjustedStart, trimmedSet, forward,
-				pattern, CommentType.PRE));
+			searchers.add(new CommentFieldSearcher(commentSupplier, program, adjustedStart,
+				trimmedSet, forward, pattern, CommentType.PRE));
 		}
 		if (options.searchLabels()) {
 			searchers.add(
@@ -224,12 +230,12 @@ public class ProgramDatabaseSearcher implements Searcher {
 					program, adjustedStart, trimmedSet, forward, pattern, format));
 		}
 		if (options.searchComments()) {
-			searchers.add(new CommentFieldSearcher(program, adjustedStart, trimmedSet, forward,
-				pattern, CommentType.EOL));
-			searchers.add(new CommentFieldSearcher(program, adjustedStart, trimmedSet, forward,
-				pattern, CommentType.REPEATABLE));
-			searchers.add(new CommentFieldSearcher(program, adjustedStart, trimmedSet, forward,
-				pattern, CommentType.POST));
+			searchers.add(new CommentFieldSearcher(commentSupplier, program, adjustedStart,
+				trimmedSet, forward, pattern, CommentType.EOL));
+			searchers.add(new CommentFieldSearcher(commentSupplier, program, adjustedStart,
+				trimmedSet, forward, pattern, CommentType.REPEATABLE));
+			searchers.add(new CommentFieldSearcher(commentSupplier, program, adjustedStart,
+				trimmedSet, forward, pattern, CommentType.POST));
 		}
 	}

@@ -306,26 +306,39 @@ public class SearchTextPlugin2Test extends AbstractGhidraHeadedIntegrationTest {
 	public void testWilcardEntry2() throws Exception {

 		Address addr = getAddr(0x1002d6d);
-		int transactionID = program.startTransaction("test");
-		CodeUnit cu = program.getListing().getCodeUnitAt(addr);
-		try {
-			cu.setComment(CommentType.POST, "********** my entry Exit **********");
-		}
-		finally {
-			program.endTransaction(transactionID, true);
-		}
+		tx(program, () -> {
+			CodeUnit cu = program.getListing().getCodeUnitAt(addr);
+			cu.setComment(CommentType.POST, "** my entry Exit **");
+		});

 		JTextField tf = findComponent(container, JTextField.class);
 		assertNotNull(tf);

-		setTextAndPressEnter(tf, "********** entry Exit **********");
+		setTextAndPressEnter(tf, "** entry Exit **");

 		waitForSearchTasks(dialog);

-		waitForSwing();
-		cbPlugin.updateNow();
 		ProgramLocation loc = cbPlugin.getCurrentLocation();
-		assertEquals(addr, loc.getAddress());
+
+		assertEquals(addr, loc.getAddress()); // ** my entry Exit **
+
+		pressSearchButton();
+		assertEquals(addr, loc.getAddress()); // * my entry Exit **
+
+		pressSearchButton();
+		assertEquals(addr, loc.getAddress()); //  my entry Exit **
+
+		pressSearchButton();
+		assertEquals(addr, loc.getAddress()); // my entry Exit **
+
+		pressSearchButton();
+		assertEquals(addr, loc.getAddress()); // y entry Exit **
+
+		pressSearchButton();
+		assertEquals(addr, loc.getAddress()); //  entry Exit **
+
+		pressSearchButton();
+		assertEquals(addr, loc.getAddress()); // entry Exit **

 		pressSearchButton();

@@ -333,35 +346,6 @@ public class SearchTextPlugin2Test extends AbstractGhidraHeadedIntegrationTest {

 	}

-	@Test
-	public void testWilcardEntry3() throws Exception {
-
-		Address addr = getAddr(0x1002d6d);
-		int transactionID = program.startTransaction("test");
-		CodeUnit cu = program.getListing().getCodeUnitAt(addr);
-		try {
-			cu.setComment(CommentType.POST, "********** ___sbh_find_block Exit **********");
-		}
-		finally {
-			program.endTransaction(transactionID, true);
-		}
-
-		JTextField tf = findComponent(container, JTextField.class);
-		assertNotNull(tf);
-		setTextAndPressEnter(tf, "********** Exit **********");
-
-		waitForSearchTasks(dialog);
-
-		waitForSwing();
-		cbPlugin.updateNow();
-		ProgramLocation loc = cbPlugin.getCurrentLocation();
-		assertEquals(addr, loc.getAddress());
-
-		pressSearchButton();
-
-		assertEquals("Not found", dialog.getStatusText());
-	}
-
 	@Test
 	public void testWilcardEntry4() throws Exception {

@@ -650,6 +634,7 @@ public class SearchTextPlugin2Test extends AbstractGhidraHeadedIntegrationTest {
 			t = dialog1.getTaskScheduler().getCurrentThread();
 		}
 		waitForSwing();
+		cbPlugin.updateNow();
 	}

 	private void selectRadioButton(Container guiContainer, String buttonText) throws Exception {
@@ -15,6 +15,7 @@
 */
 package ghidra.app.plugin.core.searchtext.quicksearch;

+import static ghidra.program.model.listing.CommentType.*;
 import static org.junit.Assert.*;

 import java.util.ArrayList;
@@ -54,6 +55,7 @@ public class ProgramDatabaseSearchIteratorTest extends AbstractGhidraHeadedInteg
 	private Address currentAddress;
 	private ToyProgramBuilder builder;
 	private TaskMonitor monitor = TaskMonitor.DUMMY;
+	private CommentAddressSupplier commentSupplier;

 	private void createIMM(long address) throws MemoryAccessException {
 		builder.addBytesMoveImmediate(address, (short) 5);
@@ -118,6 +120,13 @@ public class ProgramDatabaseSearchIteratorTest extends AbstractGhidraHeadedInteg
 			if (searcher.hasMatch(currentAddress)) {
 				return searcher.getMatch().programLocation();
 			}
+
+			// for tests that search comments, we have to advance the comment supplier explicitly
+			// just like the searcher algorithm does
+			if (commentSupplier != null) {
+				commentSupplier.advance(currentAddress);
+			}
+
 			currentAddress = searcher.getNextSignificantAddress(currentAddress);
 		}
 		return null;
@@ -127,23 +136,20 @@ public class ProgramDatabaseSearchIteratorTest extends AbstractGhidraHeadedInteg
 	public void testEOLCommentIterator() {

 		Pattern pattern = UserSearchUtils.createSearchPattern("XXZ*", false);
-		ProgramLocation startLocation = new ProgramLocation(program, program.getMinAddress());
-		CommentFieldSearcher searcher =
-			new CommentFieldSearcher(program, startLocation, null, true, pattern, CommentType.EOL);
+		ProgramLocation start = new ProgramLocation(program, program.getMinAddress());
+		CommentFieldSearcher searcher = createCommentSearcher(pattern, start, CommentType.EOL);
 		currentAddress = searcher.getNextSignificantAddress(null);
 		assertNull(getNextMatch(searcher));

 		// add a comment with no match
 		addEolComment(0x1005146L, "Test EOL comments...");
-		searcher =
-			new CommentFieldSearcher(program, startLocation, null, true, pattern, CommentType.EOL);
+		searcher = createCommentSearcher(pattern, start, CommentType.EOL);
 		currentAddress = searcher.getNextSignificantAddress(null);
 		assertNull(getNextMatch(searcher));

 		// add a comment that has one match
 		addEolComment(0x1005d4bL, "Test something with eXXZabc");
-		searcher =
-			new CommentFieldSearcher(program, startLocation, null, true, pattern, CommentType.EOL);
+		searcher = createCommentSearcher(pattern, start, CommentType.EOL);
 		currentAddress = searcher.getNextSignificantAddress(null);
 		ProgramLocation loc = getNextMatch(searcher);
 		assertNotNull(loc);
@@ -151,8 +157,7 @@ public class ProgramDatabaseSearchIteratorTest extends AbstractGhidraHeadedInteg

 		// add a comment with two matches for a total of 3 matches
 		addEolComment(0x100595f, "Hit found: eXXZabc followed by XXZabc");
-		searcher =
-			new CommentFieldSearcher(program, startLocation, null, true, pattern, CommentType.EOL);
+		searcher = createCommentSearcher(pattern, start, CommentType.EOL);
 		currentAddress = searcher.getNextSignificantAddress(null);

 		loc = getNextMatch(searcher);
@@ -168,20 +173,27 @@ public class ProgramDatabaseSearchIteratorTest extends AbstractGhidraHeadedInteg

 	}

+	private CommentFieldSearcher createCommentSearcher(Pattern pattern, ProgramLocation start,
+			CommentType type) {
+		AddressSetView set = program.getMemory().getAllInitializedAddressSet();
+		commentSupplier = new CommentAddressSupplier(program, set, true);
+		return new CommentFieldSearcher(commentSupplier, program, start, null, true, pattern, type);
+	}
+
 	@Test
 	public void testSingleWildcard() {
-		addEolComment(0x100101cL, "Test EOL comments...");
-		addEolComment(0x100101dL, "Test something with eXXZabc");
-		addEolComment(0x100101fL, "Hit found: eXXZabc followed by XXZabc");
+		addEolComment(0x100101cL, "Hi");
+		addEolComment(0x100101dL, "Jo");

 		Pattern pattern = UserSearchUtils.createSearchPattern("*", false);
 		ProgramLocation startLocation = new ProgramLocation(program, program.getMinAddress());
-		CommentFieldSearcher searcher =
-			new CommentFieldSearcher(program, startLocation, null, true, pattern, CommentType.EOL);
-		currentAddress = searcher.getNextSignificantAddress(null);
+
+		CommentFieldSearcher searcher = createCommentSearcher(pattern, startLocation, EOL);
 		int count = 0;
-		Address[] addrs =
-			new Address[] { getAddr(0x100101cL), getAddr(0x100101dL), getAddr(0x100101fL) };
+		// '*' will find a match of the full comment starting at the current char being searched 
+		// (meaning we get a hit starting at every character)
+		Address[] addrs = new Address[] { getAddr(0x100101cL), getAddr(0x100101cL),
+			getAddr(0x100101dL), getAddr(0x100101dL) };

 		ProgramLocation loc = null;
 		while ((loc = getNextMatch(searcher)) != null) {
@@ -197,9 +209,8 @@ public class ProgramDatabaseSearchIteratorTest extends AbstractGhidraHeadedInteg
 		addEolComment(0x100101fL, "Hit found: ABCxyzvvXXZ123 followed by ABCxqa123");

 		Pattern pattern = UserSearchUtils.createSearchPattern("ABC*123", false);
-		ProgramLocation startLocation = new ProgramLocation(program, program.getMinAddress());
-		CommentFieldSearcher searcher =
-			new CommentFieldSearcher(program, startLocation, null, true, pattern, CommentType.EOL);
+		ProgramLocation start = new ProgramLocation(program, program.getMinAddress());
+		CommentFieldSearcher searcher = createCommentSearcher(pattern, start, CommentType.EOL);
 		currentAddress = searcher.getNextSignificantAddress(null);

 		ProgramLocation loc = getNextMatch(searcher);
@@ -2940,6 +2940,9 @@ public class CodeManager implements ErrorHandler, ManagerDB {
 		try {
 			long addr = addrMap.getKey(address, false);
 			DBRecord commentRec = getCommentAdapter().getRecord(addr);
+			if (commentRec == null) {
+				return null;
+			}
 			CommentType[] types = CommentType.values();
 			String[] comments = new String[types.length];