[group_task][jvm] Precompute compile contexts for all targets

PROBLEM:
Zinc learns about analysis files when they are passed in analysis-map argument. We always supply the analysis files for all targets in the build graph every time we invoke Zinc. The problem is that the paths to the analysis- and class files are computed separately by ScalaCompile and JavaZincCompile. This happens in compile_context() for isolated strategy, where a path is computed by concatenating target.id to the task's workdir. Basically compile_context() is only suitable for computing the context for the task's own targets, but we're (ab)using it for all targets in the build graph, when computing the value for analysis-map argument. This means some of the paths are incorrect and non-existent: e.g. path to java target analysis is computed by ScalaCompile as compile/jvm/scala/isolated-analysis/storage.clients.manhattan.client.src.main.java.java, while it should be compile/jvm/zinc-java/isolated-analysis/storage.clients.manhattan.client.src.main.java.java. Such incorrect paths are not matched with classpath entries in _upstream_analysis(), and therefore the corresponding analysis files are not included in the analysis-map argument. This causes Zinc to consider cross-language dependencies as binary dependencies and compare timestamps, which is suboptimal.

SOLUTION:
Compute the paths to analysis files more intelligently. The CompileContexts are computed in prepare_compile() (for both strategies) and saved in a global (target -> compile_context) map belonging to GroupTask. Then in compile_chunk(), this global map is used instead of calling _create_compile_contexts_for_targets(). This also possibly solves the boundary between tasks using global strategy (I believe they all map to same analysis and classdir) and the ones using isolated strategy.

DOWNSIDE:
GroupTask is now aware of the fact that it's in fact a compilation task, as does GroupMember. This was bound to happen anyway since GroupTask is going away.

Testing Done:
Green CI: https://travis-ci.org/pantsbuild/pants/builds/70450002

Also, ran incremental compilation with -ldebug and observed no "Missing upstream analysis" messages where they used to be.

Bugs closed: 1788

Reviewed at https://rbcommons.com/s/twitter/r/2452/

src/python/pants/backend/core/tasks/group_task.py

@@ -7,7 +7,7 @@
 
 import os
 from abc import abstractmethod, abstractproperty
-from collections import defaultdict, deque
+from collections import OrderedDict, defaultdict, deque
 
 from pants.backend.core.tasks.task import Task, TaskBase
 from pants.base.build_graph import invert_dependencies
@@ -318,7 +318,11 @@ def add_member(cls, group_member):
   def __init__(self, *args, **kwargs):
     super(GroupTask, self).__init__(*args, **kwargs)
 
-    self._group_members = [member_type(self.context, os.path.join(self.workdir, member_type.name()))
+    self._all_compile_contexts = OrderedDict()
+
+    self._group_members = [member_type(context=self.context,
+                                       workdir=os.path.join(self.workdir, member_type.name()),
+                                       all_compile_contexts=self._all_compile_contexts)
                            for member_type in self._member_types()]
 
   @abstractmethod

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py

@@ -176,7 +176,7 @@ def extra_products(self, target):
     """
     return []
 
-  def __init__(self, *args, **kwargs):
+  def __init__(self, all_compile_contexts=None, *args, **kwargs):
     super(JvmCompile, self).__init__(*args, **kwargs)
 
     # JVM options for running the compiler.
@@ -200,6 +200,7 @@ def __init__(self, *args, **kwargs):
     self._strategy = strategy_constructor(self.context,
                                           self.get_options(),
                                           self.workdir,
+                                          all_compile_contexts,
                                           self.create_analysis_tools(),
                                           self._language,
                                           lambda s: s.endswith(self._file_suffix))

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile_global_strategy.py

@@ -57,9 +57,9 @@ def register_options(cls, register, language, supports_concurrent_execution):
              help='If non-zero, and we have fewer than this number of locally-changed targets, '
                   'partition them separately, to preserve stability when compiling repeatedly.')
 
-  def __init__(self, context, options, workdir, analysis_tools, language, sources_predicate):
-    super(JvmCompileGlobalStrategy, self).__init__(context, options, workdir, analysis_tools,
-                                                   language, sources_predicate)
+  def __init__(self, context, options, workdir, all_compile_contexts, analysis_tools, language, sources_predicate):
+    super(JvmCompileGlobalStrategy, self).__init__(context, options, workdir, all_compile_contexts,
+                                                   analysis_tools, language, sources_predicate)
 
     # Various working directories.
     # NB: These are grandfathered in with non-strategy-specific names, but to prevent
@@ -110,7 +110,7 @@ def munge_flag(flag):
   def name(self):
     return 'global'
 
-  def compile_context(self, target):
+  def _compute_compile_context(self, target):
     """Returns the default/stable compile context for the given target.
 
     Temporary compile contexts are private to the strategy.

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile_isolated_strategy.py

@@ -7,7 +7,7 @@
 
 import os
 import shutil
-from collections import OrderedDict, defaultdict
+from collections import defaultdict
 
 from pants.backend.jvm.tasks.classpath_util import ClasspathUtil
 from pants.backend.jvm.tasks.jvm_compile.execution_graph import (ExecutionFailure, ExecutionGraph,
@@ -33,9 +33,9 @@ def register_options(cls, register, language, supports_concurrent_execution):
     register('--capture-log', action='store_true', default=False, advanced=True,
             help='Capture compilation output to per-target logs.')
 
-  def __init__(self, context, options, workdir, analysis_tools, language, sources_predicate):
-    super(JvmCompileIsolatedStrategy, self).__init__(context, options, workdir, analysis_tools,
-                                                     language, sources_predicate)
+  def __init__(self, context, options, workdir, all_compile_contexts, analysis_tools, language, sources_predicate):
+    super(JvmCompileIsolatedStrategy, self).__init__(context, options, workdir, all_compile_contexts,
+                                                     analysis_tools, language, sources_predicate)
 
     # Various working directories.
     self._analysis_dir = os.path.join(workdir, 'isolated-analysis')
@@ -56,21 +56,14 @@ def __init__(self, context, options, workdir, analysis_tools, language, sources_
   def name(self):
     return 'isolated'
 
-  def compile_context(self, target):
+  def _compute_compile_context(self, target):
     analysis_file = JvmCompileStrategy._analysis_for_target(self._analysis_dir, target)
     classes_dir = os.path.join(self._classes_dir, target.id)
     return self.CompileContext(target,
                                analysis_file,
                                classes_dir,
                                self._sources_for_target(target))
 
-  def _create_compile_contexts_for_targets(self, targets):
-    compile_contexts = OrderedDict()
-    for target in targets:
-      compile_context = self.compile_context(target)
-      compile_contexts[target] = compile_context
-    return compile_contexts
-
   def pre_compile(self):
     super(JvmCompileIsolatedStrategy, self).pre_compile()
     safe_mkdir(self._analysis_dir)
@@ -243,11 +236,9 @@ def compile_chunk(self,
     extra_compile_time_classpath = self._compute_extra_classpath(
         extra_compile_time_classpath_elements)
 
-    compile_contexts = self._create_compile_contexts_for_targets(all_targets)
-
     # Now create compile jobs for each invalid target one by one.
     jobs = self._create_compile_jobs(compile_classpaths,
-                                     compile_contexts,
+                                     self._all_compile_contexts,
                                      extra_compile_time_classpath,
                                      invalid_targets,
                                      invalidation_check.invalid_vts_partitioned,

src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile_strategy.py

@@ -61,7 +61,7 @@ def register_options(cls, register, language, supports_concurrent_execution):
     The abstract base class does not register any options itself: those are left to JvmCompile.
     """
 
-  def __init__(self, context, options, workdir, analysis_tools, language, sources_predicate):
+  def __init__(self, context, options, workdir, all_compile_contexts, analysis_tools, language, sources_predicate):
     self._language = language
     self.context = context
     self._analysis_tools = analysis_tools
@@ -73,6 +73,8 @@ def __init__(self, context, options, workdir, analysis_tools, language, sources_
     self._sources_by_target = None
     self._sources_predicate = sources_predicate
 
+    self._all_compile_contexts = all_compile_contexts
+
     # The ivy confs for which we're building.
     self._confs = options.confs
     self._clear_invalid_analysis = options.clear_invalid_analysis
@@ -86,12 +88,16 @@ def invalidation_hints(self, relevant_targets):
     """A tuple of partition_size_hint and locally_changed targets for the given inputs."""
 
   @abstractmethod
-  def compile_context(self, target):
-    """Returns the default/stable compile context for the given target.
+  def _compute_compile_context(self, target):
+    """Computes the default/stable compile context for the given target.
 
     Temporary compile contexts are private to the strategy.
     """
 
+  def compile_context(self, target):
+    """Returns the default/stable compile context for the given target."""
+    return self._all_compile_contexts[target]
+
   @abstractmethod
   def compute_classes_by_source(self, compile_contexts):
     """Compute a map of (context->(src->classes)) for the given compile_contexts.
@@ -149,12 +155,16 @@ def validate_analysis(self, path):
   def prepare_compile(self, cache_manager, all_targets, relevant_targets):
     """Prepares to compile the given set of targets.
 
-    Has the side effects of pruning old analysis, and computing deleted sources.
+    Has the side effects of pruning old analysis, computing deleted sources, and computing compile contexts.
     """
     # Target -> sources (relative to buildroot).
     # TODO(benjy): Should sources_by_target be available in all Tasks?
     self._sources_by_target = self._compute_sources_by_target(relevant_targets)
 
+    # Compute compile contexts for targets in the current chunk.
+    for target in relevant_targets:
+      self._all_compile_contexts[target] = self._compute_compile_context(target)
+
   def class_name_for_class_file(self, compile_context, class_file_name):
     if not class_file_name.endswith(".class"):
       return None

tests/python/pants_test/tasks/test_group_task.py

@@ -148,7 +148,7 @@ class RecordingGroupMember(GroupMember):
       def prepare(cls, options, round_manager):
         self.recorded_actions.append(self.prepare_action(name))
 
-      def __init__(me, *args, **kwargs):
+      def __init__(me, all_compile_contexts=None, *args, **kwargs):
         super(RecordingGroupMember, me).__init__(*args, **kwargs)
         self.recorded_actions.append(self.construct_action(name))