I noticed Expansion.resolve still comes up really high on perf charts…

…. This patch makes it much easier to resolve expansions which are just literals, which is very common for variable names. Unfortunately, this makes the code a fair bit more complex, and doesn't help nearly as much as I'd like.

I'm beginning to wonder if getting gmake performance parity is impossible, or at least improbable given the current architecture: but I can't think of an alternate architecture that is better.

--HG--
branch : resolve-perf

pymake/data.py

@@ -60,6 +60,47 @@ def _if_else(c, t, f):
         return t()
     return f()
 
+class StringExpansion(object):
+    __slots__ = ('s', 't')
+    loc = None
+
+    def __init__(self, s):
+        assert isinstance(s, str)
+        self.s = s
+        self.t = tuple(s)
+
+    def lstrip(self):
+        self.s = self.s.lstrip()
+
+    def rstrip(self):
+        self.s = self.s.rstrip()
+
+    def _firstisstring(self):
+        return True
+
+    def _firststring(self):
+        return self.s
+
+    def _afterfirst(self):
+        return ()
+
+    def isempty(self):
+        return self.s == ''
+
+    def resolve(self, i, j, k=None):
+        return self.t
+
+    def resolvestr(self, i, j, k=None):
+        return self.s
+
+    def resolvesplit(self, i, j, k=None):
+        return self.s.split()
+
+    def clone(self):
+        e = Expansion()
+        e.appendstr(self.s)
+        return e
+
 class Expansion(list):
     """
     A representation of expanded data, such as that for a recursively-expanded variable, a command, etc.
@@ -74,20 +115,27 @@ def __init__(self, loc=None):
 
     @staticmethod
     def fromstring(s):
-        e = Expansion()
-        e.appendstr(s)
-        return e
+        return StringExpansion(s)
 
     def clone(self):
         e = Expansion()
         e.extend(self)
         return e
 
+    def _firstisstring(self):
+        return len(self) and not self[0][1]
+
+    def _firststring(self):
+        return self[0][0]
+
+    def _afterfirst(self):
+        return self[1:]
+
     def _lastisstring(self):
         return len(self) and not self[-1][1]
 
-    def _firstisstring(self):
-        return len(self) and not self[0][1]
+    def _laststring(self):
+        return self[-1][0]
 
     def append(self, e):
         raise NotImplementedError("Don't call me!")
@@ -112,10 +160,8 @@ def appendfunc(self, func):
     def concat(self, o):
         """Concatenate the other expansion on to this one."""
         if o._firstisstring() and self._lastisstring():
-            mystr = self[-1][0]
-            ostr = o[0][0]
-            self[-1] = mystr + ostr, False
-            self.extend(o[1:])
+            self[-1] = self._laststring() + o._firststring(), False
+            self.extend(o._afterfirst())
         else:
             self.extend(o)
 
@@ -134,6 +180,15 @@ def rstrip(self):
             s = self[-1][0].rstrip()
             self[-1] = s, False
 
+    def finish(self):
+        if len(self) == 0:
+            return StringExpansion('')
+
+        if len(self) == 1 and not self[0][1]:
+            return StringExpansion(self[0][0])
+
+        return self
+
     def resolve(self, makefile, variables, setting=[]):
         """
         Resolve this variable into a value, by interpolating the value
@@ -172,7 +227,7 @@ def resolvesplit(self, makefile, variables, setting=[]):
         return util.itersplit(self.resolve(makefile, variables, setting))
 
     def __repr__(self):
-        return "<Expansion with elements: %r>" % ([repr(e) for e, isfunc in self],)
+        return "<Expansion with elements: %r>" % ([e for e, isfunc in self],)
 
 class Variables(object):
     """
@@ -384,9 +439,12 @@ def match(self, word):
                 return word
             return None
 
-        l1 = len(self.data[0])
-        l2 = len(self.data[1])
-        if len(word) >= l1 + l2 and word.startswith(self.data[0]) and word.endswith(self.data[1]):
+        if word.startswith(self.data[0]) and word.endswith(self.data[1]):
+            l1 = len(self.data[0])
+            l2 = len(self.data[1])
+            if len(word) < l1 + l2:
+                return None
+
             if l2 == 0:
                 return word[l1:]
             return word[l1:-l2]
@@ -979,7 +1037,7 @@ def __init__(self, prereqs, doublecolon, loc):
         self.loc = loc
 
     def addcommand(self, c):
-        assert isinstance(c, Expansion)
+        assert isinstance(c, (Expansion, StringExpansion))
         self.commands.append(c)
 
     def getcommands(self, target, makefile):
@@ -1029,7 +1087,7 @@ def __init__(self, targetpatterns, prerequisites, doublecolon, loc):
         self.commands = []
 
     def addcommand(self, c):
-        assert isinstance(c, Expansion)
+        assert isinstance(c, (Expansion, StringExpansion))
         self.commands.append(c)
 
     def ismatchany(self):

pymake/functions.py

@@ -38,7 +38,7 @@ def setup(self):
         assert self.maxargs == 0 or argc <= self.maxargs, "Parser screwed up, gave us too many args"
 
     def append(self, arg):
-        assert isinstance(arg, data.Expansion)
+        assert isinstance(arg, (data.Expansion, data.StringExpansion))
         self._arguments.append(arg)
 
     def __len__(self):
@@ -47,7 +47,7 @@ def __len__(self):
 class VariableRef(Function):
     def __init__(self, loc, vname):
         self.loc = loc
-        assert isinstance(vname, data.Expansion)
+        assert isinstance(vname, (data.Expansion, data.StringExpansion))
         self.vname = vname
 
     def setup(self):
@@ -384,6 +384,7 @@ def setup(self):
 
     def resolve(self, makefile, variables, setting):
         condition = self._arguments[0].resolvestr(makefile, variables, setting)
+
         if len(condition):
             return self._arguments[1].resolve(makefile, variables, setting)
 
@@ -459,6 +460,7 @@ def resolve(self, makefile, variables, setting):
             v.set(str(i), data.Variables.FLAVOR_SIMPLE, data.Variables.SOURCE_AUTOMATIC, param)
 
         flavor, source, e = variables.get(vname)
+
         if e is None:
             return ()
 

pymake/parser.py

@@ -750,8 +750,7 @@ def parsemakesyntax(d, startat, stopon, iterfunc):
                 if fname is not None:
                     fn = functions.functionmap[fname](loc)
                     e = data.Expansion()
-                    fn.append(e)
-                    if len(fn) == fn.maxargs:
+                    if len(fn) + 1 == fn.maxargs:
                         tokenlist = TokenList.get((c, closebrace, '$'))
                     else:
                         tokenlist = TokenList.get((',', c, closebrace, '$'))
@@ -786,19 +785,20 @@ def parsemakesyntax(d, startat, stopon, iterfunc):
             stack.pop()
         elif stacktop.parsestate == PARSESTATE_TOPLEVEL:
             assert len(stack) == 1
-            return stacktop.expansion, token, offset
+            return stacktop.expansion.finish(), token, offset
         elif stacktop.parsestate == PARSESTATE_FUNCTION:
             if token == ',':
-                stacktop.expansion = data.Expansion()
-                stacktop.function.append(stacktop.expansion)
+                stacktop.function.append(stacktop.expansion.finish())
 
-                if len(stacktop.function) == stacktop.function.maxargs:
+                stacktop.expansion = data.Expansion()
+                if len(stacktop.function) + 1 == stacktop.function.maxargs:
                     tokenlist = TokenList.get((stacktop.openbrace, stacktop.closebrace, '$'))
                     stacktop.tokenlist = tokenlist
             elif token in (')', '}'):
-                    stacktop.function.setup()
-                    stack.pop()
-                    stack[-1].expansion.appendfunc(stacktop.function)
+                stacktop.function.append(stacktop.expansion.finish())
+                stacktop.function.setup()
+                stack.pop()
+                stack[-1].expansion.appendfunc(stacktop.function)
             else:
                 assert False, "Not reached, PARSESTATE_FUNCTION"
         elif stacktop.parsestate == PARSESTATE_VARNAME:
@@ -809,7 +809,7 @@ def parsemakesyntax(d, startat, stopon, iterfunc):
                 stacktop.tokenlist = TokenList.get(('=', stacktop.openbrace, stacktop.closebrace, '$'))
             elif token in (')', '}'):
                 stack.pop()
-                stack[-1].expansion.appendfunc(functions.VariableRef(stacktop.loc, stacktop.expansion))
+                stack[-1].expansion.appendfunc(functions.VariableRef(stacktop.loc, stacktop.expansion.finish()))
             else:
                 assert False, "Not reached, PARSESTATE_VARNAME"
         elif stacktop.parsestate == PARSESTATE_SUBSTFROM:
@@ -826,15 +826,15 @@ def parsemakesyntax(d, startat, stopon, iterfunc):
                 stacktop.varname.appendstr(':')
                 stacktop.varname.concat(stacktop.expansion)
                 stack.pop()
-                stack[-1].expansion.appendfunc(functions.VariableRef(stacktop.loc, stacktop.varname))
+                stack[-1].expansion.appendfunc(functions.VariableRef(stacktop.loc, stacktop.varname.finish()))
             else:
                 assert False, "Not reached, PARSESTATE_SUBSTFROM"
         elif stacktop.parsestate == PARSESTATE_SUBSTTO:
             assert token in  (')','}'), "Not reached, PARSESTATE_SUBSTTO"
 
             stack.pop()
-            stack[-1].expansion.appendfunc(functions.SubstitutionRef(stacktop.loc, stacktop.varname,
-                                                                     stacktop.substfrom, stacktop.expansion))
+            stack[-1].expansion.appendfunc(functions.SubstitutionRef(stacktop.loc, stacktop.varname.finish(),
+                                                                     stacktop.substfrom.finish(), stacktop.expansion.finish()))
         else:
             assert False, "Unexpected parse state %s" % stacktop.parsestate
 
@@ -845,4 +845,4 @@ def parsemakesyntax(d, startat, stopon, iterfunc):
 
     assert stack[0].parsestate == PARSESTATE_TOPLEVEL
 
-    return stack[0].expansion, None, None
+    return stack[0].expansion.finish(), None, None

pymake/parserdata.py

@@ -107,8 +107,8 @@ def addcommand(self, r):
 
 class Rule(Statement):
     def __init__(self, targetexp, depexp, doublecolon):
-        assert isinstance(targetexp, data.Expansion)
-        assert isinstance(depexp, data.Expansion)
+        assert isinstance(targetexp, (data.Expansion, data.StringExpansion))
+        assert isinstance(depexp, (data.Expansion, data.StringExpansion))
 
         self.targetexp = targetexp
         self.depexp = depexp
@@ -144,9 +144,9 @@ def dump(self, fd, indent):
 
 class StaticPatternRule(Statement):
     def __init__(self, targetexp, patternexp, depexp, doublecolon):
-        assert isinstance(targetexp, data.Expansion)
-        assert isinstance(patternexp, data.Expansion)
-        assert isinstance(depexp, data.Expansion)
+        assert isinstance(targetexp, (data.Expansion, data.StringExpansion))
+        assert isinstance(patternexp, (data.Expansion, data.StringExpansion))
+        assert isinstance(depexp, (data.Expansion, data.StringExpansion))
 
         self.targetexp = targetexp
         self.patternexp = patternexp
@@ -185,7 +185,7 @@ def dump(self, fd, indent):
 
 class Command(Statement):
     def __init__(self, exp):
-        assert isinstance(exp, data.Expansion)
+        assert isinstance(exp, (data.Expansion, data.StringExpansion))
         self.exp = exp
 
     def execute(self, makefile, context):
@@ -197,9 +197,9 @@ def dump(self, fd, indent):
 
 class SetVariable(Statement):
     def __init__(self, vnameexp, token, value, valueloc, targetexp, source=None):
-        assert isinstance(vnameexp, data.Expansion)
+        assert isinstance(vnameexp, (data.Expansion, data.StringExpansion))
         assert isinstance(value, str)
-        assert targetexp is None or isinstance(targetexp, data.Expansion)
+        assert targetexp is None or isinstance(targetexp, (data.Expansion, data.StringExpansion))
 
         if source is None:
             source = data.Variables.SOURCE_MAKEFILE
@@ -266,8 +266,8 @@ class EqCondition(Condition):
     expected = True
 
     def __init__(self, exp1, exp2):
-        assert isinstance(exp1, data.Expansion)
-        assert isinstance(exp2, data.Expansion)
+        assert isinstance(exp1, (data.Expansion, data.StringExpansion))
+        assert isinstance(exp2, (data.Expansion, data.StringExpansion))
 
         self.exp1 = exp1
         self.exp2 = exp2
@@ -284,7 +284,7 @@ class IfdefCondition(Condition):
     expected = True
 
     def __init__(self, exp):
-        assert isinstance(exp, data.Expansion)
+        assert isinstance(exp, (data.Expansion, data.StringExpansion))
         self.exp = exp
 
     def evaluate(self, makefile):
@@ -352,7 +352,7 @@ def dump(self, fd, indent):
 
 class Include(Statement):
     def __init__(self, exp, required):
-        assert isinstance(exp, data.Expansion)
+        assert isinstance(exp, (data.Expansion, data.StringExpansion))
         self.exp = exp
         self.required = required
 
@@ -366,7 +366,7 @@ def dump(self, fd, indent):
 
 class VPathDirective(Statement):
     def __init__(self, exp):
-        assert isinstance(exp, data.Expansion)
+        assert isinstance(exp, (data.Expansion, data.StringExpansion))
         self.exp = exp
 
     def execute(self, makefile, context):
@@ -392,7 +392,7 @@ def dump(self, fd, indent):
 
 class ExportDirective(Statement):
     def __init__(self, exp, single):
-        assert isinstance(exp, data.Expansion)
+        assert isinstance(exp, (data.Expansion, data.StringExpansion))
         self.exp = exp
         self.single = single
 
@@ -412,7 +412,7 @@ def dump(self, fd, indent):
 
 class EmptyDirective(Statement):
     def __init__(self, exp):
-        assert isinstance(exp, data.Expansion)
+        assert isinstance(exp, (data.Expansion, data.StringExpansion))
         self.exp = exp
 
     def execute(self, makefile, context):

tests/var-set.mk

@@ -23,6 +23,9 @@ all: IMMVAR += $(BASIC)
 all: UNSET += more
 all: OBASIC += allmore
 
+CHECKLIT = $(NULL) check
+all: CHECKLIT += appendliteral
+
 RECVAR = blimey
 
 TESTEMPTY = \
@@ -35,6 +38,7 @@ all: other
 	test "$(IMMVAR)" = "bloo foo var baz  valall"
 	test "$(UNSET)" = "more"
 	test "$(OBASIC)" = "oval"
+	test "$(CHECKLIT)" = " check appendliteral"
 	test "$(TESTEMPTY)" = ""
 	@echo TEST-PASS