Restore MD5-Timestamp performance by removing try/except from every call. Also some optimized logic for updating and using the dependency_map built as part of the decider. Fixed tests. Note Deciders now need a fourth argument 'repo_node' which is the repository node for the file if it's to be used. This is currently only used by md5-timestamp decider File.changed_timestamp_then_content()

9 files changed, 103 insertions, 72 deletions

diff --git a/src/engine/SCons/Environment.py b/src/engine/SCons/Environment.py
index 7f9a76c..eb3284a 100644
--- a/src/engine/SCons/Environment.py
+++ b/src/engine/SCons/Environment.py
@@ -864,18 +864,21 @@ class SubstitutionEnvironment(object):
         return self
 
 
-def default_decide_source(dependency, target, prev_ni):
+def default_decide_source(dependency, target, prev_ni, repo_node=None):
     f = SCons.Defaults.DefaultEnvironment().decide_source
-    return f(dependency, target, prev_ni)
+    return f(dependency, target, prev_ni, repo_node)
 
-def default_decide_target(dependency, target, prev_ni):
+
+def default_decide_target(dependency, target, prev_ni, repo_node=None):
     f = SCons.Defaults.DefaultEnvironment().decide_target
-    return f(dependency, target, prev_ni)
+    return f(dependency, target, prev_ni, repo_node)
+
 
 def default_copy_from_cache(src, dst):
     f = SCons.Defaults.DefaultEnvironment().copy_from_cache
     return f(src, dst)
 
+
 class Base(SubstitutionEnvironment):
     """Base class for "real" construction Environments.  These are the
     primary objects used to communicate dependency and construction
@@ -1434,13 +1437,13 @@ class Base(SubstitutionEnvironment):
             _warn_copy_deprecated = False
         return self.Clone(*args, **kw)
 
-    def _changed_build(self, dependency, target, prev_ni):
-        if dependency.changed_state(target, prev_ni):
+    def _changed_build(self, dependency, target, prev_ni, repo_node=None):
+        if dependency.changed_state(target, prev_ni, repo_node):
             return 1
-        return self.decide_source(dependency, target, prev_ni)
+        return self.decide_source(dependency, target, prev_ni, repo_node)
 
-    def _changed_content(self, dependency, target, prev_ni):
-        return dependency.changed_content(target, prev_ni)
+    def _changed_content(self, dependency, target, prev_ni, repo_node=None):
+        return dependency.changed_content(target, prev_ni, repo_node)
 
     def _changed_source(self, dependency, target, prev_ni):
         target_env = dependency.get_build_env()
@@ -1450,14 +1453,14 @@ class Base(SubstitutionEnvironment):
         else:
             return target_env.decide_target(dependency, target, prev_ni)
 
-    def _changed_timestamp_then_content(self, dependency, target, prev_ni):
-        return dependency.changed_timestamp_then_content(target, prev_ni)
+    def _changed_timestamp_then_content(self, dependency, target, prev_ni, repo_node=None):
+        return dependency.changed_timestamp_then_content(target, prev_ni, repo_node)
 
-    def _changed_timestamp_newer(self, dependency, target, prev_ni):
-        return dependency.changed_timestamp_newer(target, prev_ni)
+    def _changed_timestamp_newer(self, dependency, target, prev_ni, repo_node=None):
+        return dependency.changed_timestamp_newer(target, prev_ni, repo_node)
 
-    def _changed_timestamp_match(self, dependency, target, prev_ni):
-        return dependency.changed_timestamp_match(target, prev_ni)
+    def _changed_timestamp_match(self, dependency, target, prev_ni, repo_node=None):
+        return dependency.changed_timestamp_match(target, prev_ni, repo_node)
 
     def _copy_from_cache(self, src, dst):
         return self.fs.copy(src, dst)
diff --git a/src/engine/SCons/Node/FS.py b/src/engine/SCons/Node/FS.py
index 91d349d..ac85638 100644
--- a/src/engine/SCons/Node/FS.py
+++ b/src/engine/SCons/Node/FS.py
@@ -2635,6 +2635,7 @@ class File(Base):
         if SCons.Debug.track_instances: logInstanceCreation(self, 'Node.FS.File')
         Base.__init__(self, name, directory, fs)
         self._morph()
+        self.attributes.changed_timestamp_then_content = None
 
     def Entry(self, name):
         """Create an entry node named 'name' relative to
@@ -3283,14 +3284,14 @@ class File(Base):
             self._memo['changed'] = has_changed
         return has_changed
 
-    def changed_content(self, target, prev_ni):
+    def changed_content(self, target, prev_ni, repo_node=None):
         cur_csig = self.get_csig()
         try:
             return cur_csig != prev_ni.csig
         except AttributeError:
             return 1
 
-    def changed_state(self, target, prev_ni):
+    def changed_state(self, target, prev_ni, repo_node=None):
         return self.state != SCons.Node.up_to_date
 
 
@@ -3324,6 +3325,21 @@ class File(Base):
 
         return binfo.dependency_map
 
+    # @profile
+    def _add_strings_to_dependency_map(self, dmap):
+        """
+        In the case comparing node objects isn't sufficient, we'll add the strings for the nodes to the dependency map
+        :return:
+        """
+
+        first_string = str(next(iter(dmap)))
+
+        # print("DMAP:%s"%id(dmap))
+        if first_string not in dmap:
+                string_dict = {str(child): signature for child, signature in dmap.items()}
+                dmap.update(string_dict)
+        return dmap
+
     def _get_previous_signatures(self, dmap):
         """
         Return a list of corresponding csigs from previous
@@ -3342,37 +3358,62 @@ class File(Base):
         if len(dmap) == 0:
             if MD5_TIMESTAMP_DEBUG: print("Nothing dmap shortcutting")
             return None
+        elif MD5_TIMESTAMP_DEBUG: print("len(dmap):%d"%len(dmap))
+
 
-        if MD5_TIMESTAMP_DEBUG: print("len(dmap):%d"%len(dmap))
-        # First try the simple name for node
-        c_str = str(self)
-        if MD5_TIMESTAMP_DEBUG: print("Checking   :%s"%c_str)
-        df = dmap.get(c_str, None)
+        # First try retrieving via Node
+        if MD5_TIMESTAMP_DEBUG: print("Checking if self is in  map:%s id:%s type:%s"%(str(self), id(self), type(self)))
+        df = dmap.get(self, False)
         if df:
             return df
 
+        # Now check if self's repository file is in map.
+        rf = self.rfile()
+        if MD5_TIMESTAMP_DEBUG: print("Checking if self.rfile  is in  map:%s id:%s type:%s"%(str(rf), id(rf), type(rf)))
+        rfm = dmap.get(rf, False)
+        if rfm:
+            return rfm
+
+        # get default string for node and then also string swapping os.altsep for os.sep (/ for \)
+        c_strs = [str(self)]
+
         if os.altsep:
-            c_str = c_str.replace(os.sep, os.altsep)
-            df = dmap.get(c_str, None)
-            if MD5_TIMESTAMP_DEBUG: print("-->%s"%df)
+            c_strs.append(c_strs[0].replace(os.sep, os.altsep))
+
+        # In some cases the dependency_maps' keys are already strings check.
+        # Check if either string is now in dmap.
+        for s in c_strs:
+            if MD5_TIMESTAMP_DEBUG: print("Checking if str(self) is in map  :%s" % s)
+            df = dmap.get(s, False)
             if df:
                 return df
 
+        # Strings didn't existing in map, add them and try again
+        # If there are no strings in this dmap, then add them.
+        # This may not be necessary, we could walk the nodes in the dmap and check each string
+        # rather than adding ALL the strings to dmap. In theory that would be n/2 vs 2n str() calls on node
+        # if not dmap.has_strings:
+        dmap = self._add_strings_to_dependency_map(dmap)
+
+        # In some cases the dependency_maps' keys are already strings check.
+        # Check if either string is now in dmap.
+        for s in c_strs:
+            if MD5_TIMESTAMP_DEBUG: print("Checking if str(self) is in map (now with strings)  :%s" % s)
+            df = dmap.get(s, False)
+            if df:
+                return df
+
+        # Lastly use nodes get_path() to generate string and see if that's in dmap
         if not df:
             try:
                 # this should yield a path which matches what's in the sconsign
                 c_str = self.get_path()
-                df = dmap.get(c_str, None)
-                if MD5_TIMESTAMP_DEBUG: print("-->%s"%df)
-                if df:
-                    return df
-
                 if os.altsep:
                     c_str = c_str.replace(os.sep, os.altsep)
-                    df = dmap.get(c_str, None)
-                    if MD5_TIMESTAMP_DEBUG: print("-->%s"%df)
-                    if df:
-                        return df
+
+                if MD5_TIMESTAMP_DEBUG: print("Checking if self.get_path is in map (now with strings)  :%s" % s)
+
+                df = dmap.get(c_str, None)
 
             except AttributeError as e:
                 raise FileBuildInfoFileToCsigMappingError("No mapping from file name to content signature for :%s"%c_str)
@@ -3399,9 +3440,6 @@ class File(Base):
         Returns:
             Boolean - Indicates if node(File) has changed.
         """
-        if node is None:
-            # We need required node argument to get BuildInfo to function
-            raise DeciderNeedsNode(self.changed_timestamp_then_content)
 
         # Now get sconsign name -> csig map and then get proper prev_ni if possible
         bi = node.get_stored_info().binfo
@@ -3433,7 +3471,6 @@ class File(Base):
                 print("Mismatch self.changed_timestamp_match(%s, prev_ni) old:%s new:%s"%(str(target), old, new))
                 new_prev_ni = self._get_previous_signatures(dependency_map)
 
-
         if not new:
             try:
                 # NOTE: We're modifying the current node's csig in a query.
@@ -3443,13 +3480,13 @@ class File(Base):
             return False
         return self.changed_content(target, new_prev_ni)
 
-    def changed_timestamp_newer(self, target, prev_ni):
+    def changed_timestamp_newer(self, target, prev_ni, repo_node=None):
         try:
             return self.get_timestamp() > target.get_timestamp()
         except AttributeError:
             return 1
 
-    def changed_timestamp_match(self, target, prev_ni):
+    def changed_timestamp_match(self, target, prev_ni, repo_node=None):
         """
         Return True if the timestamps don't match or if there is no previous timestamp
         :param target:
diff --git a/src/engine/SCons/Node/__init__.py b/src/engine/SCons/Node/__init__.py
index aeb7092..ff98e2d 100644
--- a/src/engine/SCons/Node/__init__.py
+++ b/src/engine/SCons/Node/__init__.py
@@ -271,7 +271,7 @@ class DeciderNeedsNode(Exception):
 #
 # First, the single decider functions
 #
-def changed_since_last_build_node(node, target, prev_ni):
+def changed_since_last_build_node(node, target, prev_ni, repo_node=None):
     """
 
     Must be overridden in a specific subclass to return True if this
@@ -292,7 +292,7 @@ def changed_since_last_build_node(node, target, prev_ni):
     raise NotImplementedError
 
 
-def changed_since_last_build_alias(node, target, prev_ni):
+def changed_since_last_build_alias(node, target, prev_ni, repo_node=None):
     cur_csig = node.get_csig()
     try:
         return cur_csig != prev_ni.csig
@@ -300,24 +300,24 @@ def changed_since_last_build_alias(node, target, prev_ni):
         return 1
 
 
-def changed_since_last_build_entry(node, target, prev_ni):
+def changed_since_last_build_entry(node, target, prev_ni, repo_node=None):
     node.disambiguate()
-    return _decider_map[node.changed_since_last_build](node, target, prev_ni)
+    return _decider_map[node.changed_since_last_build](node, target, prev_ni, repo_node)
 
 
-def changed_since_last_build_state_changed(node, target, prev_ni):
+def changed_since_last_build_state_changed(node, target, prev_ni, repo_node=None):
     return node.state != SCons.Node.up_to_date
 
 
-def decide_source(node, target, prev_ni):
-    return target.get_build_env().decide_source(node, target, prev_ni)
+def decide_source(node, target, prev_ni, repo_node=None):
+    return target.get_build_env().decide_source(node, target, prev_ni, repo_node)
 
 
-def decide_target(node, target, prev_ni):
-    return target.get_build_env().decide_target(node, target, prev_ni)
+def decide_target(node, target, prev_ni, repo_node=None):
+    return target.get_build_env().decide_target(node, target, prev_ni, repo_node)
 
 
-def changed_since_last_build_python(node, target, prev_ni):
+def changed_since_last_build_python(node, target, prev_ni, repo_node=None):
     cur_csig = node.get_csig()
     try:
         return cur_csig != prev_ni.csig
@@ -1505,17 +1505,11 @@ class Node(object, with_metaclass(NoSlotsPyPy)):
             result = True
 
         for child, prev_ni in zip(children, then):
-            try:
-                if _decider_map[child.changed_since_last_build](child, self, prev_ni):
-                    if t: Trace(': %s changed' % child)
-                    result = True
-            except DeciderNeedsNode as e:
-                if e.decider(self, prev_ni, node=node):
-                    if t: Trace(': %s changed' % child)
-                    result = True
+            if _decider_map[child.changed_since_last_build](child, self, prev_ni, node):
+                if t: Trace(': %s changed' % child)
+                result = True
 
         if self.has_builder():
-            import SCons.Util
             contents = self.get_executor().get_contents()
             newsig = SCons.Util.MD5signature(contents)
             if bi.bactsig != newsig:
diff --git a/src/engine/SCons/SConf.py b/src/engine/SCons/SConf.py
index 59afb40..71729c9 100644
--- a/src/engine/SCons/SConf.py
+++ b/src/engine/SCons/SConf.py
@@ -56,7 +56,6 @@ import SCons.Warnings
 import SCons.Conftest
 
 from SCons.Debug import Trace
-from SCons.Node import DeciderNeedsNode
 
 # Turn off the Conftest error logging
 SCons.Conftest.LogInputFiles = 0
@@ -407,12 +406,10 @@ class SConfBase(object):
             # that the correct .sconsign info will get calculated
             # and keep the build state consistent.
             def force_build(dependency, target, prev_ni,
-                            env_decider=env.decide_source,
-                            node=None):
+                            repo_node=None,
+                            env_decider=env.decide_source):
                 try:
-                    env_decider(dependency, target, prev_ni)
-                except DeciderNeedsNode as e:
-                    e.decider(target, prev_ni, node=target)
+                    env_decider(dependency, target, prev_ni, repo_node)
                 except Exception as e:
                     raise e
                 return True
diff --git a/test/Decider/Environment.py b/test/Decider/Environment.py
index 58cd57b..4a23b5c 100644
--- a/test/Decider/Environment.py
+++ b/test/Decider/Environment.py
@@ -38,7 +38,7 @@ DefaultEnvironment(tools=[])
 import os.path
 env = Environment(tools=[])
 env.Command('file.out', 'file.in', Copy('$TARGET', '$SOURCE'))
-def my_decider(dependency, target, prev_ni):
+def my_decider(dependency, target, prev_ni, repo_node=None):
     return os.path.exists('has-changed')
 env.Decider(my_decider)
 """)
diff --git a/test/Decider/Node.py b/test/Decider/Node.py
index c1910de..0fa9726 100644
--- a/test/Decider/Node.py
+++ b/test/Decider/Node.py
@@ -38,7 +38,7 @@ import os.path
 file_in = File('file.in')
 file_out = File('file.out')
 Command(file_out, file_in, Copy('$TARGET', '$SOURCE'))
-def my_decider(dependency, target, prev_ni):
+def my_decider(dependency, target, prev_ni, repo_node):
     return os.path.exists('has-changed')
 file_in.Decider(my_decider)
 """)
diff --git a/test/Decider/default.py b/test/Decider/default.py
index 5d0a452..78f981e 100644
--- a/test/Decider/default.py
+++ b/test/Decider/default.py
@@ -36,7 +36,7 @@ test.write('SConstruct', """
 DefaultEnvironment(tools=[])
 import os.path
 Command('file.out', 'file.in', Copy('$TARGET', '$SOURCE'))
-def my_decider(dependency, target, prev_ni):
+def my_decider(dependency, target, prev_ni, repo_node=None):
     return os.path.exists('has-changed')
 Decider(my_decider)
 """)
diff --git a/test/Decider/mixed.py b/test/Decider/mixed.py
index 08daa7d..711bd2b 100644
--- a/test/Decider/mixed.py
+++ b/test/Decider/mixed.py
@@ -47,11 +47,11 @@ denv.Command('ddd.out', 'ddd.in',   Copy('$TARGET', '$SOURCE'))
 denv.Command('n2.out',  n2_in,      Copy('$TARGET', '$SOURCE'))
 env.Command( 'eee.out', 'eee.in',   Copy('$TARGET', '$SOURCE'))
 env.Command( 'n3.out',  n3_in,      Copy('$TARGET', '$SOURCE'))
-def default_decider(dependency, target, prev_ni):
+def default_decider(dependency, target, prev_ni, repo_node=None):
     return os.path.exists('default-has-changed')
-def env_decider(dependency, target, prev_ni):
+def env_decider(dependency, target, prev_ni, repo_node=None):
     return os.path.exists('env-has-changed')
-def node_decider(dependency, target, prev_ni):
+def node_decider(dependency, target, prev_ni, repo_node=None):
     return os.path.exists('node-has-changed')
 Decider(default_decider)
 env.Decider(env_decider)
diff --git a/test/packaging/rpm/explicit-target.py b/test/packaging/rpm/explicit-target.py
index 553ce27..48b5c83 100644
--- a/test/packaging/rpm/explicit-target.py
+++ b/test/packaging/rpm/explicit-target.py
@@ -77,7 +77,7 @@ env.Package( NAME           = 'foo',
 
 expect = """
 scons: *** Setting target is not supported for rpm.
-""" + test.python_file_line(test.workpath('SConstruct'), 12)
+""" + test.python_file_line(test.workpath('SConstruct'), 23)
 
 test.run(arguments='', status=2, stderr=expect)