Performance fix for consolidated classpath

Review Request #4184 — Created Aug. 23, 2016 and submitted

mateor, nhoward_tw, patricklaw, stuhood, zundel
Previously we fetched the classpath entries for each target which was very expensive.  This change fetches them upfront.

Prior to change:

23.419 main:consolidate-classpath
23.414 main:consolidate-classpath:consolidate-classpath

After change:

1.344 main:consolidate-classpath
1.340 main:consolidate-classpath:consolidate-classpath

CI pending:

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  2. This set of targets will no longer be filtered to those that have relevant classpath entries.

    Should this do something like use the keys of a filtered dict generated by _find_consolidate_classpath_candidates?

    1. The find_consolidated_classpath_candidates wasn't buying us anyting since we did the exact same checks in consolidate_classpath. We just did them twice before.

  3. This method is unused now?

    1. Yes, I'll remove this.

  1. Ship It!
  1. Ship It!
  2. So this passes all targets types to the consolidate method, including unexpected ones like PythonLibrary etc. Which is likely correct, my understanding of the classpath is that it should take all non-jvm files and bundle them into the runtime classpath as jars.

    But that should only need to happen for targets that are depended upon by a jvm target.

    A future optimization could be to only include the closure of jvm_targets, maybe even the closure of jvm_library targets?

    Also, some target types we know are not needed to consolidate - specifically the JarLibrary, JarDependency and ArtifactClasspathEntry. Do all those pass an is_jar check?

    I guess it depends how much of a perf hit this task still has whether you think this is useful.

  3. Could you restore the comment?

    It's possible that there are still perf wins to be found here, including having this task output the consolidated classpath as a synthetic jar.

    Such a jar could be cached and then this task would only need to process invalid vts.

    1. I didn't mean to open an issue for this - I marked it fixed to unblock you. But if you do another revision, keep it in mind for me : )

Review request changed

Status: Closed (submitted)

Change Summary:

commit 46a4b43f02d9aa4558ed225750017c1d4ea20ea3