diff --git a/src/python/pants/build_graph/build_graph.py b/src/python/pants/build_graph/build_graph.py index 4a6af6e05d5..d533657c3b2 100644 --- a/src/python/pants/build_graph/build_graph.py +++ b/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,6 +128,7 @@ 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) @@ -133,6 +136,22 @@ def reset(self): 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. diff --git a/src/python/pants/build_graph/mutable_build_graph.py b/src/python/pants/build_graph/mutable_build_graph.py deleted file mode 100644 index f7a6760cb8b..00000000000 --- a/src/python/pants/build_graph/mutable_build_graph.py +++ /dev/null @@ -1,142 +0,0 @@ -# coding=utf-8 -# Copyright 2014 Pants project contributors (see CONTRIBUTORS.md). -# Licensed under the Apache License, Version 2.0 (see LICENSE). - -from __future__ import absolute_import, division, print_function, unicode_literals - -import logging -import traceback - -from pants.build_graph.address import Address -from pants.build_graph.address_lookup_error import AddressLookupError -from pants.build_graph.build_graph import BuildGraph - - -logger = logging.getLogger(__name__) - - -class MutableBuildGraph(BuildGraph): - """A directed acyclic graph of Targets and dependencies. Not necessarily connected.""" - - def __init__(self, address_mapper): - self._address_mapper = address_mapper - super(MutableBuildGraph, self).__init__() - - def clone_new(self): - """Returns a new BuildGraph instance of the same type and with the same __init__ params.""" - return MutableBuildGraph(self._address_mapper) - - def reset(self): - super(MutableBuildGraph, self).reset() - self._addresses_already_closed = set() - - 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): - if self.contains_address(address): - # The address was either mapped in or synthetically injected already. - return - - if address in self._addresses_already_closed: - # We've visited this address already in the course of the active recursive injection. - return - - mapper = self._address_mapper - - target_address, target_addressable = mapper.resolve(address) - - self._addresses_already_closed.add(target_address) - try: - dep_addresses = list(mapper.specs_to_addresses(target_addressable.dependency_specs, - relative_to=target_address.spec_path)) - deps_seen = set() - for dep_address in dep_addresses: - if dep_address in deps_seen: - raise self.DuplicateAddressError( - "Addresses in dependencies must be unique. '{spec}' is referenced more than once." - .format(spec=dep_address.spec)) - deps_seen.add(dep_address) - self.inject_address_closure(dep_address) - - if not self.contains_address(target_address): - target = self._target_addressable_to_target(target_address, target_addressable) - self.apply_injectables([target]) - self.inject_target(target, dependencies=dep_addresses) - else: - for dep_address in dep_addresses: - if dep_address not in self.dependencies_of(target_address): - self.inject_dependency(target_address, dep_address) - target = self.get_target(target_address) - - for traversable_spec in target.compute_dependency_specs(payload=target.payload): - traversable_address = Address.parse(traversable_spec, relative_to=target_address.spec_path) - self.maybe_inject_address_closure(traversable_address) - - if not any(traversable_address == t.address for t in target.dependencies): - self.inject_dependency(dependent=target.address, dependency=traversable_address) - target.mark_transitive_invalidation_hash_dirty() - - for traversable_spec in target.compute_injectable_specs(payload=target.payload): - traversable_address = Address.parse(traversable_spec, relative_to=target_address.spec_path) - self.maybe_inject_address_closure(traversable_address) - target.mark_transitive_invalidation_hash_dirty() - - except AddressLookupError as e: - raise self.TransitiveLookupError("{message}\n referenced from {spec}" - .format(message=e, spec=target_address.spec)) - - def inject_roots_closure(self, target_roots, fail_fast=None): - for address in self._address_mapper.scan_specs(target_roots.specs, - fail_fast=fail_fast): - self.inject_address_closure(address) - yield address - - def inject_specs_closure(self, specs, fail_fast=None): - for address in self._address_mapper.scan_specs(specs, - fail_fast=fail_fast): - self.inject_address_closure(address) - yield address - - def _target_addressable_to_target(self, address, addressable): - """Realizes a TargetAddressable into a Target at `address`. - - :param TargetAddressable addressable: - :param Address address: - """ - try: - # TODO(John Sirois): Today - in practice, Addressable is unusable. BuildGraph assumes - # addressables are in fact TargetAddressables with dependencies (see: - # `inject_address_closure` for example), ie: leaf nameable things with - by definition - no - # deps cannot actually be used. Clean up BuildGraph to handle addressables as they are - # abstracted today which does not necessarily mean them having dependencies and thus forming - # graphs. They may only be multiply-referred to leaf objects. - target = addressable.instantiate(build_graph=self, address=address) - return target - except Exception: - traceback.print_exc() - logger.exception('Failed to instantiate Target with type {target_type} with name "{name}"' - ' at address {address}' - .format(target_type=addressable.addressed_type, - name=addressable.addressed_name, - address=address)) - raise - - def resolve_address(self, address): - if self.contains_address(address): - return self.get_target(address) - else: - _, addressable = self._address_mapper.resolve(address) - return addressable diff --git a/src/python/pants/engine/legacy/graph.py b/src/python/pants/engine/legacy/graph.py index 9e361bfaa61..3cfe2c02651 100644 --- a/src/python/pants/engine/legacy/graph.py +++ b/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]) diff --git a/src/python/pants/goal/context.py b/src/python/pants/goal/context.py index 39498be0c00..7da1e41a2be 100644 --- a/src/python/pants/goal/context.py +++ b/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 diff --git a/tests/python/pants_test/build_graph/BUILD b/tests/python/pants_test/build_graph/BUILD index e0baa6de893..0fc9ec2f3f7 100644 --- a/tests/python/pants_test/build_graph/BUILD +++ b/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', ] ) diff --git a/tests/python/pants_test/build_graph/test_build_file_aliases.py b/tests/python/pants_test/build_graph/test_build_file_aliases.py index b63db6cbcc5..b5b59a3a0d5 100644 --- a/tests/python/pants_test/build_graph/test_build_file_aliases.py +++ b/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})