Memoize graph walks on Context

src/python/pants/build_graph/build_graph.py

@@ -6,6 +6,7 @@
 
 import itertools
 import logging
+import weakref
 from abc import abstractmethod
 from builtins import filter, object
 from collections import defaultdict, deque
@@ -98,6 +99,7 @@ def closure(*vargs, **kwargs):
     return Target.closure_for_targets(*vargs, **kwargs)
 
   def __init__(self):
+    self._invalidation_callbacks = []
     self.reset()
 
   def __len__(self):
@@ -126,13 +128,30 @@ def reset(self):
 
     :API: public
     """
+    self._invalidated()
     self._target_by_address = OrderedDict()
     self._target_dependencies_by_address = defaultdict(OrderedSet)
     self._target_dependees_by_address = defaultdict(OrderedSet)
     self._derived_from_by_derivative = {}  # Address -> Address.
     self._derivatives_by_derived_from = defaultdict(list)   # Address -> list of Address.
     self.synthetic_addresses = set()
 
+  def add_invalidation_callback(self, callback):
+    """Adds a no-arg function that will be called whenever the graph is changed.
+
+    This interface exists to allow for invalidation of caches as the graph changes (due to injected
+    targets: generally synthetic). We should continue to move away from mutability in order to make
+    this kind of interface unnecessary, but synthetic targets are a fundamental part of the v1 model
+    that can likely only be removed via v2.
+    """
+    self._invalidation_callbacks.append(weakref.ref(callback))
+
+  def _invalidated(self):
+    for callback_ref in self._invalidation_callbacks:
+      callback = callback_ref()
+      if callback is not None:
+        callback()
+
   def contains_address(self, address):
     """
     :API: public
@@ -240,6 +259,7 @@ def inject_target(self, target, dependencies=None, derived_from=None, synthetic=
     :param bool synthetic: Whether to flag this target as synthetic, even if it isn't derived
       from another target.
     """
+    self._invalidated()
     if self.contains_address(target.address):
       raise ValueError('Attempted to inject synthetic {target} derived from {derived_from}'
                        ' into the BuildGraph with address {address}, but there is already a Target'
@@ -290,6 +310,7 @@ def inject_dependency(self, dependent, dependency):
       is being added.
     :param Address dependency: The dependency to be injected.
     """
+    self._invalidated()
     if dependent not in self._target_by_address:
       raise ValueError('Cannot inject dependency from {dependent} on {dependency} because the'
                        ' dependent is not in the BuildGraph.'
@@ -533,7 +554,6 @@ def transitive_subgraph_of_addresses_bfs(self,
           to_walk.append((level + 1, dep_address))
     return ordered_closure
 
-  @abstractmethod
   def inject_synthetic_target(self,
                               address,
                               target_type,
@@ -553,6 +573,14 @@ def inject_synthetic_target(self,
       from the `derived_from`.
     :param Target derived_from: The Target this Target will derive from.
     """
+    target = target_type(name=address.target_name,
+                         address=address,
+                         build_graph=self,
+                         **kwargs)
+    self.inject_target(target,
+                       dependencies=dependencies,
+                       derived_from=derived_from,
+                       synthetic=True)
 
   def maybe_inject_address_closure(self, address):
     """If the given address is not already injected to the graph, calls inject_address_closure.

src/python/pants/engine/legacy/graph.py

@@ -180,21 +180,6 @@ def _instantiate_remote_sources(self, kwargs):
     kwargs['dest'] = _DestWrapper((self._target_types[kwargs['dest']],))
     return RemoteSources(build_graph=self, **kwargs)
 
-  def inject_synthetic_target(self,
-                              address,
-                              target_type,
-                              dependencies=None,
-                              derived_from=None,
-                              **kwargs):
-    target = target_type(name=address.target_name,
-                         address=address,
-                         build_graph=self,
-                         **kwargs)
-    self.inject_target(target,
-                       dependencies=dependencies,
-                       derived_from=derived_from,
-                       synthetic=True)
-
   def inject_address_closure(self, address):
     self.inject_addresses_closure([address])
 

src/python/pants/goal/context.py

@@ -46,7 +46,14 @@ def __init__(self, options, run_tracker, target_roots,
                address_mapper=None, console_outstream=None, scm=None,
                workspace=None, invalidation_report=None, scheduler=None):
     self._options = options
+
+    # We register a callback that will cause build graph edits to invalidate our caches, and we hold
+    # a handle directly to the callback function to ensure that it is not GC'd until the context is.
     self.build_graph = build_graph
+    self._clear_target_cache_handle = self._clear_target_cache
+    self._targets_cache = dict()
+    self.build_graph.add_invalidation_callback(self._clear_target_cache_handle)
+
     self._build_file_parser = build_file_parser
     self.build_configuration = build_configuration
     self.address_mapper = address_mapper
@@ -269,6 +276,14 @@ def _replace_targets(self, target_roots):
     # only 1 remaining known use case in the Foursquare codebase that will be able to go away with
     # the post RoundEngine engine - kill the method at that time.
     self._target_roots = list(target_roots)
+    self._clear_target_cache()
+
+  def _clear_target_cache(self):
+    """A callback for cases where the graph or target roots have been mutated.
+
+    See BuildGraph.add_invalidation_callback.
+    """
+    self._targets_cache.clear()
 
   def add_new_target(self, address, target_type, target_base=None, dependencies=None,
                      derived_from=None, **kwargs):
@@ -279,6 +294,7 @@ def add_new_target(self, address, target_type, target_base=None, dependencies=No
 
     :API: public
     """
+    self._clear_target_cache()
     rel_target_base = target_base or address.spec_path
     abs_target_base = os.path.join(get_buildroot(), rel_target_base)
     if not os.path.exists(abs_target_base):
@@ -317,21 +333,28 @@ def targets(self, predicate=None, **kwargs):
                           `False` or preorder by default.
     :returns: A list of matching targets.
     """
-    target_set = self._collect_targets(self.target_roots, **kwargs)
+    targets_cache_key = tuple(sorted(kwargs))
+    targets = self._targets_cache.get(targets_cache_key)
+    if targets is None:
+      self._targets_cache[targets_cache_key] = targets = self._unfiltered_targets(**kwargs)
+    return list(filter(predicate, targets))
+
+  def _unfiltered_targets(self, **kwargs):
+    def _collect_targets(root_targets, **kwargs):
+      return Target.closure_for_targets(
+        target_roots=root_targets,
+        **kwargs
+      )
+
+    target_set = _collect_targets(self.target_roots, **kwargs)
 
     synthetics = OrderedSet()
     for synthetic_address in self.build_graph.synthetic_addresses:
       if self.build_graph.get_concrete_derived_from(synthetic_address) in target_set:
         synthetics.add(self.build_graph.get_target(synthetic_address))
-    target_set.update(self._collect_targets(synthetics, **kwargs))
+    target_set.update(_collect_targets(synthetics, **kwargs))
 
-    return list(filter(predicate, target_set))
-
-  def _collect_targets(self, root_targets, **kwargs):
-    return Target.closure_for_targets(
-      target_roots=root_targets,
-      **kwargs
-    )
+    return target_set
 
   def dependents(self, on_predicate=None, from_predicate=None):
     """Returns  a map from targets that satisfy the from_predicate to targets they depend on that

tests/python/pants_test/build_graph/BUILD

@@ -41,6 +41,7 @@ python_tests(
   sources = ['test_build_file_aliases.py'],
   dependencies = [
     'src/python/pants/build_graph',
+    'src/python/pants/engine/legacy:graph',
     'tests/python/pants_test/subsystem:subsystem_utils',
   ]
 )

tests/python/pants_test/build_graph/test_build_file_aliases.py

@@ -9,8 +9,8 @@
 
 from pants.build_graph.address import Address
 from pants.build_graph.build_file_aliases import BuildFileAliases, TargetMacro
-from pants.build_graph.mutable_build_graph import MutableBuildGraph
 from pants.build_graph.target import Target
+from pants.engine.legacy.graph import LegacyBuildGraph
 from pants_test.subsystem.subsystem_util import init_subsystem
 
 
@@ -79,7 +79,7 @@ def test_create_bad_targets(self):
     with self.assertRaises(TypeError):
       BuildFileAliases(targets={'fred': object()})
 
-    target = Target('fred', Address.parse('a:b'), MutableBuildGraph(address_mapper=None))
+    target = Target('fred', Address.parse('a:b'), LegacyBuildGraph(None, None))
     with self.assertRaises(TypeError):
       BuildFileAliases(targets={'fred': target})