Use nailgun for javac and import parser #12982

stuhood · 2021-09-22T05:40:20Z

Warm JVMs are necessary for reasonable performance in common tasks like compilation and import parsing. Although there are other options to achieve them (Bazel's gRPC-based worker protocol in particular), we have had good experiences using nailgun, and already have a client and nascent support.

This change refreshes the process_execution crate's support for nailgun, and then enables it by default. Individual processes opt-in to using nailgun by setting a second digest on Process which includes all inputs for the nailgun server.

Moves from a is_nailgunnable: bool to a use_nailgun: Digest field, which is used as the inputs-to and fingerprint-for the server. This hands the responsibility for computing the server-relevant subset of the entire input_digest to the Process author.
Moves the NailgunPool to a fingerprint-aware LRU, where we first attempt to find an idle server with a matching fingerprint, and then kill the least recently used idle server without a matching fingerprint. As mentioned in comments, the NailgunPool assumes it is used under a Semaphore (the BoundedCommandRunner), and will error if the pool is full and none are idle.
Adds a BorrowedNailgunProcess drop guard that ensures that the working directory of the server is cleaned up between runs, and kills the server on cancellation. Moves the workdir creation which used to be implemented by the CapturedWorkdir trait out into the local::CommandRunner, because the nailgun::CommandRunner must reuse the server's workdir, clearing it between runs.
Refactors JdkSetup to (mostly) encapsulate the selection of the JDK, and to add the nailgun classpath to args.

Fixes #12810, fixes #8481, and fixes #8527. Although nailgun servers will be torn down via Drop in most cases, ensuring that they die when pantsd is killed by SIGKILL is future work covered by #12996.

…er`. (#12990) To prepare for #12982, swap the `process_execution::nailgun` module to async/await. [ci skip-build-wheels]

…rprint-aware LRU. [ci skip-build-wheels]

[ci skip-build-wheels]

…nner needs to use the nailgun directory. [ci skip-build-wheels]

…dentify the JVM to use. [ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

stuhood · 2021-09-24T04:47:04Z

Commits are overlapping, but somewhat useful to review independently.

Apologies for the larger review: the clearest way to provide test coverage for this code is to expose and use it in Python tests, so while landing the rust-only portion with only minimal testing would be an option, I thought that it made more sense to land the coverage here.

jsirois · 2021-09-24T15:02:37Z

src/rust/engine/process_execution/src/lib.rs

+  /// TODO: Currently this Digest must be a subset of the `input_digest`, but we should consider
+  /// making it disjoint, and then automatically merging it.
+  ///
+  pub use_nailgun: Digest,


Is there some trumping good ergonomic that outweighs the awkward ergonomic of having to pass EMPTY_DIGEST instead of None? An option seems more clear than the EMPTY_DIGEST ~magic value.

On the Python side this is a default argument, so users mostly don't need to worry about it. But we have in general used the "null object" pattern (with EMPTY_DIGEST as null) for digests, rather than making them optional on Process: see input_files, for example.

src/rust/engine/process_executor/src/main.rs

src/rust/engine/process_execution/src/nailgun/nailgun_pool.rs

jsirois · 2021-09-24T15:51:10Z

src/rust/engine/process_execution/src/nailgun/nailgun_pool.rs

+    if processes.len() >= self.size {
+      // Find the oldest idle non-matching process and remove it.
+      let idx = Self::find_lru_idle(&mut *processes)?.ok_or_else(|| {
+        // NB: See the method docs: the pool assumes that it is running under a semaphore, so this


Is there any harm in in-fact using another semaphore that has size slots? I'm guessing its not a measurable perf hit but maybe it is? Even though this is not needed globally, having it locally would make the code make sense without comment.

Not really... my primary concern was really just that there are already two mutexes in here, and adding a third primitive would make things a little bit unwieldy. I think that if the consequence of forgetting to do this was a panic or silently incorrect behavior, then it would be necessary to include it here. But given that we can error instead, and we know how we are called in general, this seemed slightly easier.

[ci skip-build-wheels]

Eric-Arellano · 2021-09-24T19:37:25Z

enabled auto-merge (squash)

This is a pretty big PR - might be better to not auto-merge in case people don't realize it when hitting accept.

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood · 2021-09-24T19:47:28Z

enabled auto-merge (squash)

This is a pretty big PR - might be better to not auto-merge in case people don't realize it when hitting accept.

John already accepted, so this is just waiting on CI to go green.

tdyas · 2021-09-24T20:01:04Z

src/python/pants/backend/java/util_rules.py

-            version_result.stderr.decode("utf-8"),
-            version_result.stdout.decode("utf-8"),
-        ]
+        /bin/ln -s "$({java_home_command})" "{JdkSetup.java_home}"


Why is this symlinking JAVA_HOME within the input root?

To allow callers to consume it as a relative path: https://github.com/pantsbuild/pants/pull/12982/files#diff-3dd6edcc8f9a9ef9521eb1b9cec604222276eefafa48d966fc5378e4dce20148R246-R247

tdyas · 2021-09-24T20:03:49Z

src/python/pants/backend/java/util_rules.py

-    fingerprint_comment: str
+    nailgun_jar: str
+    jdk_preparation_script: ClassVar[str] = "__jdk.sh"
+    java_home: ClassVar[str] = "__java_home"


Why is this not a constant?

ClassVars are effectively constant unless we expect this to be subclassed. But the real answer is that I was just following the existing pattern in the JavacBinary class.

tdyas · 2021-09-24T20:12:49Z

src/python/pants/backend/java/dependency_inference/java_parser_launcher.py

@@ -86,22 +90,23 @@ async def build_processors(bash: BashBinary, javac: JavacBinary) -> JavaParserCo
        ProcessResult,
        Process(
            argv=[
-                bash.path,
-                javac.javac_wrapper_script,
+                *jdk_setup.args(bash, [f"{jdk_setup.java_home}/lib/tools.jar"]),


Shouldn't we be invoking javac and not assuming existence of non-java. packages? I.e., what is someone uses a JVM that does not have com.sun.tools.javac.Main as an entrypoint?

Nailgun is not aware of javac in particular, and requires the main class as the first argument to the server. And if this class isn't present, then it's likely because coursier has selected something that is only a JVM and not a JDK.

This class is a de-factor public API... I'm sure it will eventually be deprecated, but none of these packages have moved post-Oracle.

tdyas · 2021-09-24T20:13:58Z

src/python/pants/backend/java/util_rules.py

+    nailgun = await Get(
+        ResolvedClasspathEntry,
+        CoursierLockfileEntry(
+            coord=Coordinate.from_coord_str("com.martiansoftware:nailgun-server:0.9.1"),


Maybe use the Coordinate constructor directly instead of from_coord_str?

This was referenced Sep 22, 2021

jvm: execute build tasks with warm JVMs #12810

Closed

[internal] jvm/java: ensure JDK downloaded in one process #12972

Merged

stuhood force-pushed the stuhood/nailgun-for-javac branch from e3cfe53 to a1eb359 Compare September 23, 2021 05:50

stuhood mentioned this pull request Sep 23, 2021

[internal] Async-ify NailgunPool::connect and nailgun::CommandRunner. #12990

Merged

stuhood force-pushed the stuhood/nailgun-for-javac branch from a1eb359 to a6c1660 Compare September 23, 2021 06:09

stuhood added a commit that referenced this pull request Sep 23, 2021

[internal] Async-ify NailgunPool::connect and `nailgun::CommandRunn…

f3b714d

…er`. (#12990) To prepare for #12982, swap the `process_execution::nailgun` module to async/await. [ci skip-build-wheels]

stuhood force-pushed the stuhood/nailgun-for-javac branch from a6c1660 to 900c614 Compare September 24, 2021 03:52

stuhood added 5 commits September 23, 2021 20:57

Hold an async Mutex while using a NailgunProcess, and implement finge…

38d7ec1

…rprint-aware LRU. [ci skip-build-wheels]

is_nailgunnable: bool -> use_nailgun: Digest

02531bf

[ci skip-build-wheels]

Move workdir creation out into the local runner, since the nailgun ru…

00dfe52

…nner needs to use the nailgun directory. [ci skip-build-wheels]

Move to invoking via a classname so that native nailgun support can i…

c336fd8

…dentify the JVM to use. [ci skip-build-wheels]

Adjust javac and import parser to use nailgun.

918eab6

[ci skip-rust] [ci skip-build-wheels]

stuhood force-pushed the stuhood/nailgun-for-javac branch from 900c614 to 918eab6 Compare September 24, 2021 04:08

stuhood mentioned this pull request Sep 24, 2021

Implement pantsd watchdog process to clean up child processes even after SIGKILL #12996

Open

stuhood requested review from tdyas, illicitonion and jsirois September 24, 2021 04:39

stuhood marked this pull request as ready for review September 24, 2021 04:39

jsirois approved these changes Sep 24, 2021

View reviewed changes

Review feedback.

04f18e0

[ci skip-build-wheels]

stuhood force-pushed the stuhood/nailgun-for-javac branch from e934eae to 04f18e0 Compare September 24, 2021 18:19

stuhood enabled auto-merge (squash) September 24, 2021 18:19

Fixup tests.

dc1660d

# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

tdyas reviewed Sep 24, 2021

View reviewed changes

stuhood merged commit ee9a43b into pantsbuild:main Sep 24, 2021

stuhood deleted the stuhood/nailgun-for-javac branch September 24, 2021 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use nailgun for javac and import parser #12982

Use nailgun for javac and import parser #12982

stuhood commented Sep 22, 2021 •

edited

Loading

stuhood commented Sep 24, 2021 •

edited

Loading

jsirois Sep 24, 2021

stuhood Sep 24, 2021 •

edited

Loading

jsirois Sep 24, 2021

stuhood Sep 24, 2021

Eric-Arellano commented Sep 24, 2021

stuhood commented Sep 24, 2021 •

edited

Loading

tdyas Sep 24, 2021

stuhood Sep 24, 2021

tdyas Sep 24, 2021

stuhood Sep 24, 2021

tdyas Sep 24, 2021

stuhood Sep 24, 2021 •

edited

Loading

tdyas Sep 24, 2021

Use nailgun for javac and import parser #12982

Use nailgun for javac and import parser #12982

Conversation

stuhood commented Sep 22, 2021 • edited Loading

stuhood commented Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

stuhood Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eric-Arellano commented Sep 24, 2021

stuhood commented Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Sep 22, 2021 •

edited

Loading

stuhood commented Sep 24, 2021 •

edited

Loading

stuhood Sep 24, 2021 •

edited

Loading

stuhood commented Sep 24, 2021 •

edited

Loading

stuhood Sep 24, 2021 •

edited

Loading