2005-07-22 Chris Wiebe

Fix for PR 104605: slow MachO parsing
	First pass at optimization. My test case went from >20 minutes to 5 seconds
	* utils/org/eclipse/cdt/utils/macho/MachO.java
	* utils/org/eclipse/cdt/utils/macho/MachOHelper.java
	* utils/org/eclipse/cdt/utils/macho/parser/MachOBinaryObject.java

core/org.eclipse.cdt.core/ChangeLog

@@ -1,3 +1,10 @@
+2005-07-22 Chris Wiebe
+	Fix for PR 104605: MachO parsing
+	First pass at optimization. My test case went from >20 minutes to 5 seconds
+	* utils/org/eclipse/cdt/utils/macho/MachO.java
+	* utils/org/eclipse/cdt/utils/macho/MachOHelper.java
+	* utils/org/eclipse/cdt/utils/macho/parser/MachOBinaryObject.java
+
 2005-07-22 Chris Wiebe
 	Fix for PR 104725
 	* model/org/eclipse/cdt/internal/core/model/BinaryElement.java

core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/macho/MachO.java

@@ -8,16 +8,17 @@
  * Contributors:
  *     QNX Software Systems - Initial API and implementation
  *     Craig Watson.
+ *     Apple Computer - work on performance optimizations
  *******************************************************************************/
-
 package org.eclipse.cdt.utils.macho;
 
-
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Comparator;
-import java.util.Vector;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
 
 import org.eclipse.cdt.core.CCorePlugin;
 import org.eclipse.cdt.utils.CPPFilt;
@@ -40,8 +41,10 @@ public class MachO {
     private Section[] sections;			/* sections from SegmentCommand */
     SymtabCommand symtab;		/* SymtabCommand that contains the symbol table */
     
-	protected String EMPTY_STRING = ""; //$NON-NLS-1$
+	protected static final String EMPTY_STRING = ""; //$NON-NLS-1$
 
+	protected static final SymbolComparator symbol_comparator = new SymbolComparator();
+	
 
 	public class MachOhdr {
 
@@ -572,19 +575,11 @@ public class MachO {
     }
     
 	protected String string_from_macho_symtab(MachO.SymtabCommand symtab, int index) throws IOException {
-		StringBuffer str = new StringBuffer();
-		byte tmp;
 		if ( index > symtab.strsize ) {
 				return EMPTY_STRING;
 			}
 		efile.seek(symtab.stroff + index);
-		while( true ) {
+		return getCStr();
-			tmp = efile.readByte();
-			if ( tmp == 0 )
-				break;
-			str.append((char)tmp);
-		}
-		return str.toString();
 	}
 	
 	public class Symbol implements Comparable {
@@ -713,18 +708,6 @@ public class MachO {
 			return name;
 		}
 
-		private Line getLine(long value) {
-			if (!debugsym) {
-				return null;
-			}
-			for (int l = 0; l < lines.length; l++) {
-				Line line = lines[l];
-				if (value <= line.address)
-					return line;
-			}
-			return null;
-		}
-		
 		/**
 		 * Returns line information in the form of filename:line
 		 * and if the information is not available may return null
@@ -817,13 +800,33 @@ public class MachO {
 		}
 	}
 
+	private Line getLine(long value) {
+		if (!debugsym) {
+			return null;
+		}
+		
+		int ndx = Arrays.binarySearch(lines, new Long(value));
+		if ( ndx >= 0 )
+			return lines[ndx];
+		if ( ndx == -1 ) {
+			return null;
+		}
+		ndx = -ndx - 1;
+		for (int l = ndx - 1; l < lines.length; l++) {
+			Line line = lines[l];
+			if (value <= line.address)
+				return line;
+		}
+		return null;
+	}
+	
 	/**
 	 * We have to implement a separate compararator since when we do the
 	 * binary search down below we are using a Long and a Symbol object
 	 * and the Long doesn't know how to compare against a Symbol so if
 	 * we compare Symbol vs Long it is ok, but not if we do Long vs Symbol.
 	 */
-	class SymbolComparator implements Comparator {
+	public static class SymbolComparator implements Comparator {
 		long val1, val2;
 		public int compare(Object o1, Object o2) {
 
@@ -850,7 +853,7 @@ public class MachO {
 	/**
 	 * Simple class to implement a line table
 	 */
-	public class Line implements Comparable {
+	public static class Line implements Comparable {
 		public long address;
 		public int lineno;
 		public String file;
@@ -1124,7 +1127,7 @@ public class MachO {
 		}
 		
 		/* now create line table, sorted on address */
-		ArrayList lineList = new ArrayList(nlines);
+		Map lineList = new HashMap(nlines);
 		for (int s = 0; s < symbols.length; s++) {
 			Symbol sym = symbols[s];
 			if (sym.n_type == Symbol.N_SLINE || sym.n_type == Symbol.N_FUN) {
@@ -1132,15 +1135,16 @@ public class MachO {
 				lentry.address = sym.n_value;
 				lentry.lineno = sym.n_desc;
 				
-				int l = lineList.indexOf(lentry);
+				Line lookup = (Line)lineList.get(lentry);
-				if (l >= 0)
+				if (lookup != null) {
-					lentry = (Line)lineList.get(l);
+					lentry = lookup;
-				else
+				} else {
-					lineList.add(lentry);
+					lineList.put(lentry, lentry);
-
+				}
-				if (sym.n_type == Symbol.N_FUN) {
+				
+				if (lentry.function == null && sym.n_type == Symbol.N_FUN) {
 					String func = sym.toString();
-					if (func != null) {
+					if (func != null && func.length() > 0) {
 						int colon = func.indexOf(':');
 						if (colon > 0)
 							lentry.function = func.substring(0, colon);
@@ -1152,19 +1156,17 @@ public class MachO {
 				}
 			}
 		}
-		lineList.trimToSize();
+		Set k = lineList.keySet();
-		lines = (Line[])lineList.toArray(new Line[0]);
+		lines = (Line[]) k.toArray(new Line[k.size()]);
 		Arrays.sort(lines);
 		
 		/* now check for file names */
 		for (int s = 0; s < symbols.length; s++) {
 			Symbol sym = symbols[s];
 			if (sym.n_type == Symbol.N_SO) {
-				for (int l = 0; l < lines.length; l++) {
+				Line line = getLine(sym.n_value);
-					if (sym.n_value <= lines[l].address) {
+				if (line != null) {
-						lines[l].file = sym.toString();
+					line.file = sym.toString();
-						break;
-					}
 				}
 			}
 		}
@@ -1213,6 +1215,17 @@ public class MachO {
 		return tlhints;
 	}
 	
+	private String getCStr() throws IOException {
+		StringBuffer str = new StringBuffer();
+		while( true ) {
+			byte tmp = efile.readByte();
+			if (tmp == 0)
+				break;
+			str.append((char)tmp);
+		}
+		return str.toString();
+	}
+	
 	private String getLCStr(int len) throws IOException {
 		if (len == 0)
 			return EMPTY_STRING;
@@ -1523,15 +1536,14 @@ public class MachO {
     }
 
     public DyLib[] getDyLibs(int type) {
-		Vector v = new Vector();
+		ArrayList v = new ArrayList();
-
 		for (int i = 0; i < loadcommands.length; i++) {
 			if (loadcommands[i].cmd == type) {
 				DyLibCommand dl = (DyLibCommand)loadcommands[i];
 				v.add(dl.dylib);				
 			}
 		}
-		return (DyLib[]) v.toArray(new DyLib[0]);
+		return (DyLib[]) v.toArray(new DyLib[v.size()]);
     }
 	
 	/* return the address of the function that address is in */
@@ -1540,9 +1552,6 @@ public class MachO {
 			return null;
 		}
 
-		//@@@ If this works, move it to a single instance in this class.
-		SymbolComparator symbol_comparator = new SymbolComparator();
-		
 		int ndx = Arrays.binarySearch(symbols, new Long(vma), symbol_comparator);
 		if ( ndx > 0 )
 			return symbols[ndx];

core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/macho/MachOHelper.java

@@ -8,8 +8,8 @@
  * Contributors:
  *     QNX Software Systems - Initial API and implementation
  *     Craig Watson
+ *     Apple Computer - work on performance optimizations
  *******************************************************************************/
-
 package org.eclipse.cdt.utils.macho;
 
 import java.io.IOException;
@@ -301,6 +301,8 @@ public class MachOHelper {
 		data = 0;
 		bss = 0;
 
+		// TODO further optimization
+		// TODO we only need to load the sections, not the whole shebang
 		loadBinary();
 
 		for (int i = 0; i < sections.length; i++) {

core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/macho/parser/MachOBinaryObject.java

@@ -6,17 +6,20 @@
  * http://www.eclipse.org/legal/epl-v10.html
  *
  * Contributors:
- * QNX Software Systems - Initial API and implementation
+ *     QNX Software Systems - Initial API and implementation
+ *     Apple Computer - work on performance optimizations
  *******************************************************************************/
 package org.eclipse.cdt.utils.macho.parser;
 
 import java.io.ByteArrayInputStream;
+import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 
+import org.eclipse.cdt.core.IAddress;
 import org.eclipse.cdt.core.IAddressFactory;
 import org.eclipse.cdt.core.IBinaryParser;
 import org.eclipse.cdt.core.IBinaryParser.IBinaryFile;
@@ -37,11 +40,17 @@ import org.eclipse.core.runtime.Path;
  */
 public class MachOBinaryObject extends BinaryObjectAdapter {
 
-	private BinaryObjectInfo info;
+	protected AR.ARHeader header;
-	private ISymbol[] symbols;
+	protected IAddressFactory addressFactory;
-	private AR.ARHeader header;
+	protected MachO.Attribute attributes;
-	private IAddressFactory addressFactory;
+	protected MachOHelper.Sizes sizes;
+	protected ISymbol[] symbols;
+	protected String soname;
+	protected String[] needed;
+	protected long timeStamp;
+	private static final String[] NO_NEEDED = new String[0];
 
+	
 	/**
 	 * @param parser
 	 * @param path
@@ -49,6 +58,7 @@ public class MachOBinaryObject extends BinaryObjectAdapter {
 	 */
 	public MachOBinaryObject(IBinaryParser parser, IPath path, AR.ARHeader header) {
 		super(parser, path, IBinaryFile.OBJECT);
+		this.header = header;
 	}
 
 	/**
@@ -60,36 +70,14 @@ public class MachOBinaryObject extends BinaryObjectAdapter {
 		super(parser, path, type);
 	}
 
-	/* (non-Javadoc)
-	 * @see org.eclipse.cdt.core.IBinaryParser.IBinaryObject#getSymbols()
-	 */
-	public ISymbol[] getSymbols() {
-		// Call the hasChanged first, to initialize the timestamp
-		if (hasChanged() || symbols == null) {
-			try {
-				loadAll();
-			} catch (IOException e) {
-				symbols = NO_SYMBOLS;
-			}
-		}
-		return symbols;
-	}
-
 	/* (non-Javadoc)
 	 * @see org.eclipse.cdt.utils.BinaryObjectAdapter#getBinaryObjectInfo()
 	 */
 	protected BinaryObjectInfo getBinaryObjectInfo() {
-		// Call the hasChanged first, to initialize the timestamp
+		// we don't use this method
-		if (hasChanged() || info == null) {
+		// overload to do nothing
-			try {
+		return new BinaryObjectInfo();
-				loadInfo();
-			} catch (IOException e) {
-				info = new BinaryObjectInfo();
-			}
-		}
-		return info;
 	}
-
 	
 	/* (non-Javadoc)
 	 * @see org.eclipse.cdt.core.IBinaryParser.IBinaryFile#getContents()
@@ -102,10 +90,15 @@ public class MachOBinaryObject extends BinaryObjectAdapter {
 	}
 	
 	protected MachOHelper getMachOHelper() throws IOException {
-		if (header != null) {
+		IPath path = getPath();
-			return new MachOHelper(getPath().toOSString(), header.getObjectDataOffset());
+		if (path != null) {
+			if (header != null) {
+				return new MachOHelper(path.toOSString(), header.getObjectDataOffset());
+			} else {
+				return new MachOHelper(path.toOSString());
+			}
 		}
-		return new MachOHelper(getPath().toOSString());
+		return null;
 	}
 	
 	/**
@@ -118,12 +111,128 @@ public class MachOBinaryObject extends BinaryObjectAdapter {
 		return super.getName();
 	}
 
-	protected void loadAll() throws IOException {
+	/* (non-Javadoc)
+	 * @see org.eclipse.cdt.utils.BinaryObjectAdapter#getAddressFactory()
+	 */
+	public IAddressFactory getAddressFactory() {
+		if (addressFactory == null) {
+			addressFactory = new Addr32Factory();
+		}
+		return addressFactory;
+	}
+	
+	protected void clearCachedValues() {
+		attributes = null;
+		sizes = null;
+		symbols = null;
+		soname = null;
+		needed = null;
+	}
+	
+	protected MachO.Attribute internalGetAttributes() {
+		if (hasChanged()) {
+			clearCachedValues();
+		}
+		if (attributes == null) {
+			MachOHelper helper = null;
+			try {
+				helper = getMachOHelper();
+				if (helper != null) {
+					attributes = helper.getMachO().getAttributes();
+				}
+			} catch (IOException e) {
+			} finally {
+				if (helper != null) {
+					helper.dispose();
+				}
+			}
+		}
+		return attributes;
+	}
+	
+	protected MachOHelper.Sizes internalGetSizes() {
+		if (hasChanged()) {
+			clearCachedValues();
+		}
+		if (sizes == null) {
+			MachOHelper helper = null;
+			try {
+				helper = getMachOHelper();
+				if (helper != null) {
+					sizes = helper.getSizes();
+					// since we're invoking the helper we might as well update
+					// the attributes since it's a pretty lightweight operation
+					if (attributes == null) {
+						attributes = helper.getMachO().getAttributes();
+					}
+				}
+			} catch (IOException e) {
+			} finally {
+				if (helper != null) {
+					helper.dispose();
+				}
+			}
+		}
+		return sizes;
+	}
+
+	protected ISymbol[] internalGetSymbols() {
+		if (hasChanged()) {
+			clearCachedValues();
+		}
+		if (symbols == null) {
+			loadBinaryInfo();
+		}
+		return symbols;
+	}
+	
+	protected String internalGetSoName() {
+		if (hasChanged()) {
+			clearCachedValues();
+		}
+		if (soname == null) {
+			loadBinaryInfo();
+		}
+		return soname;
+	}
+
+	protected String[] internalGetNeeded() {
+		if (hasChanged()) {
+			clearCachedValues();
+		}
+		if (needed == null) {
+			loadBinaryInfo();
+		}
+		return needed;
+	}
+
+	protected void loadBinaryInfo() {
 		MachOHelper helper = null;
 		try {
 			helper = getMachOHelper();
-			loadInfo(helper);
+			if (helper != null) {
-			loadSymbols(helper);
+				//TODO we can probably optimize this further in MachOHelper
+
+				symbols = loadSymbols(helper);
+				//TODO is the sort necessary?
+				Arrays.sort(symbols);
+				
+				soname = helper.getSoname();
+				needed = helper.getNeeded();
+				
+				// since we're invoking the helper we might as well update the
+				// sizes since it's a pretty lightweight operation by comparison
+				if (sizes == null) {
+					sizes = helper.getSizes();
+				}
+				// since we're invoking the helper we might as well update the
+				// attributes since it's a pretty lightweight operation by comparison
+				if (attributes == null) {
+					attributes = helper.getMachO().getAttributes();
+				}
+			}
+		} catch (IOException e) {
+			symbols = NO_SYMBOLS;
 		} finally {
 			if (helper != null) {
 				helper.dispose();
@@ -131,32 +240,24 @@ public class MachOBinaryObject extends BinaryObjectAdapter {
 		}
 	}
 
-	protected void loadInfo() throws IOException {
+	protected ISymbol[] loadSymbols(MachOHelper helper) throws IOException {
-		MachOHelper helper = null;
+		CPPFilt cppfilt =  null;
 		try {
-			helper = getMachOHelper();
+			ArrayList list = new ArrayList();
-			loadInfo(helper);
+			// Hack should be remove when Elf is clean
+			helper.getMachO().setCppFilter(false);
+			cppfilt = getCPPFilt();
+			//TODO we can probably optimize this further in MachOHelper
+			addSymbols(helper.getExternalFunctions(), ISymbol.FUNCTION, cppfilt, list);
+			addSymbols(helper.getLocalFunctions(), ISymbol.FUNCTION, cppfilt, list);
+			addSymbols(helper.getExternalObjects(), ISymbol.VARIABLE, cppfilt, list);
+			addSymbols(helper.getLocalObjects(), ISymbol.VARIABLE, cppfilt, list);
+			return (ISymbol[]) list.toArray(new ISymbol[list.size()]);
 		} finally {
-			if (helper != null) {
+			if (cppfilt != null) {
-				helper.dispose();
+				cppfilt.dispose();
 			}
-		}		
+		}
-	}
-	
-	protected void loadInfo(MachOHelper helper) throws IOException {
-		info = new BinaryObjectInfo();
-		info.needed = helper.getNeeded();
-		MachOHelper.Sizes sizes = helper.getSizes();
-		info.bss = sizes.bss;
-		info.data = sizes.data;
-		info.text = sizes.text;
-	
-		info.soname = helper.getSoname();
-		
-		MachO.Attribute attribute = helper.getMachO().getAttributes();
-		info.isLittleEndian = attribute.isLittleEndian();
-		info.hasDebug = attribute.hasDebug();
-		info.cpu = attribute.getCPU();
 	}
 
 	protected CPPFilt getCPPFilt() {
@@ -164,29 +265,7 @@ public class MachOBinaryObject extends BinaryObjectAdapter {
 		return parser.getCPPFilt();
 	}
 
-	protected void loadSymbols(MachOHelper helper) throws IOException {
+	private void addSymbols(MachO.Symbol[] array, int type, CPPFilt cppfilt, List list) {
-		ArrayList list = new ArrayList();
-		// Hack should be remove when Elf is clean
-		helper.getMachO().setCppFilter(false);
-
-		CPPFilt cppfilt = getCPPFilt();
-
-		addSymbols(helper.getExternalFunctions(), ISymbol.FUNCTION, cppfilt, list);
-		addSymbols(helper.getLocalFunctions(), ISymbol.FUNCTION, cppfilt, list);
-		addSymbols(helper.getExternalObjects(), ISymbol.VARIABLE, cppfilt, list);
-		addSymbols(helper.getLocalObjects(), ISymbol.VARIABLE, cppfilt, list);
-		list.trimToSize();
-
-		if (cppfilt != null) {
-			cppfilt.dispose();
-		}
-
-		symbols = (ISymbol[])list.toArray(NO_SYMBOLS);
-		Arrays.sort(symbols);
-		list.clear();
-	}
-
-	protected void addSymbols(MachO.Symbol[] array, int type, CPPFilt cppfilt, List list) {
 		for (int i = 0; i < array.length; i++) {
 			String name = array[i].toString();
 			if (cppfilt != null) {
@@ -204,13 +283,96 @@ public class MachOBinaryObject extends BinaryObjectAdapter {
 		}
 	}
 	
-	/* (non-Javadoc)
+	public String getCPU() {
-	 * @see org.eclipse.cdt.utils.BinaryObjectAdapter#getAddressFactory()
+		MachO.Attribute attribute = internalGetAttributes();
-	 */
+		if (attribute != null) {
-	public IAddressFactory getAddressFactory() {
+			return attribute.getCPU();
-		if (addressFactory == null) {
-			addressFactory = new Addr32Factory();
 		}
-		return addressFactory;
+		return ""; //$NON-NLS-1$
+	}
+
+	public boolean hasDebug() {
+		MachO.Attribute attribute = internalGetAttributes();
+		if (attribute != null) {
+			return attribute.hasDebug();
+		}
+		return false;
+	}
+
+	public boolean isLittleEndian() {
+		MachO.Attribute attribute = internalGetAttributes();
+		if (attribute != null) {
+			return attribute.isLittleEndian();
+		}
+		return false;
+	}
+
+	public long getBSS() {
+		MachOHelper.Sizes size = internalGetSizes();
+		if (size != null) {
+			return size.bss;
+		}
+		return 0;
+	}
+
+	public long getData() {
+		MachOHelper.Sizes size = internalGetSizes();
+		if (size != null) {
+			return size.data;
+		}
+		return 0;
+	}
+
+	public long getText() {
+		MachOHelper.Sizes size = internalGetSizes();
+		if (size != null) {
+			return size.text;
+		}
+		return 0;
+	}
+
+	public ISymbol[] getSymbols() {
+		ISymbol[] syms = internalGetSymbols();
+		if (syms != null) {
+			return syms;
+		}
+		return NO_SYMBOLS;
+	}
+
+	public ISymbol getSymbol(IAddress addr) {
+		//TODO should this be cached?
+		// fall back to super implementation for now
+		return super.getSymbol(addr);
+	}
+	
+	public String[] getNeededSharedLibs() {
+		String[] libs = internalGetNeeded();
+		if (libs != null) {
+			return libs;
+		}
+		return NO_NEEDED;
+	}
+
+	public String getSoName() {
+		String name = internalGetSoName();
+		if (name != null) {
+			return name;
+		}
+		return ""; //$NON-NLS-1$
+	}
+
+	protected boolean hasChanged() {
+		IPath path = getPath();
+		if (path != null) {
+			File file = path.toFile();
+			if (file != null) {
+				long modification = file.lastModified();
+				if (modification != timeStamp) {
+					timeStamp = modification;
+					return true;
+				}
+			}
+		}
+		return false;
 	}
 }