[jvm] Split nailgun digest from input file digests #13813
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ci skip-build-wheels]
…rule`s. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
|
Commits are useful to review independently. |
chrisjrn
reviewed
Dec 6, 2021
| // We set the nailgun pool size to twice the number that will ever be active in order to | ||
| // keep warm but idle processes for task fingerprints which are not currently busy (e.g. | ||
| // while busy running scalac, keeping some javac processes idle). | ||
| // TODO: The nailgun pool size should be configurable independent of concurrency, along |
There was a problem hiding this comment.
Ticket for this task as well?
There was a problem hiding this comment.
It's all the same ticket I think... you cannot adjust the total number of instances without accounting for memory. I'll expand the description there if need be.
There was a problem hiding this comment.
Yea, it's included on #13067: the idea as it stands is to statically compute the count of JVMs from the total memory limit and per-instance settings.
chrisjrn
reviewed
Dec 6, 2021
tdyas
approved these changes
Dec 6, 2021
| /// | ||
| /// If non-empty, the Digest of a nailgun server to use to attempt to spawn the Process. | ||
| /// | ||
| pub use_nailgun: Digest, |
There was a problem hiding this comment.
Should this just be called tool_files or tool_digest? Or would such a renaming be better done in its own PR?
There was a problem hiding this comment.
(This also relates to the "reusable input digests" PR, maybe we should just call these kinds of digests "tool" since they are associated with the "tool" being run.
There was a problem hiding this comment.
Should this just be called
tool_filesortool_digest? Or would such a renaming be better done in its own PR?
Maybe? But right now this being non-EMPTY_DIGEST is also treated as the trigger to attempt to spawn as nailgun, and I have been assuming that we would have a separate field for the immutable_input_*s, which don't need to differentiate between being for the tool or not. #12716 will be coming soon regardless, and can change the naming if that ends up looking right.
I think that Bazel calling them "tool_*" has to do with the fact that the worker protocol is not JVM specific (...neither is nailgun, really. but in practice).
Merged
illicitonion
added a commit
that referenced
this pull request
Dec 7, 2021
Internal changes: * [internal] Remove superfluous f-string specifiers ([#13821](#13821)) * [internal] scala: extract annotations as consumed types ([#13810](#13810)) * [jvm] Split nailgun digest from input file digests ([#13813](#13813)) * [internal] jvm: add jre_major_version and use stderr to properly extract version ([#13812](#13812)) * [internal] Clean up Go `embed` support's handling of dependencies ([#13801](#13801)) * [internal] scala: handle package object syntax in parser ([#13809](#13809)) * [internal] java: fix junit sentinel rule setup ([#13815](#13815)) * [internal] upgrade to rust v1.57.0 ([#13807](#13807)) * [internal] add failing test for FrozenDict equality issue ([#13389](#13389)) * [internal] Use `PyObject` instead of `Value` in more places ([#13802](#13802)) * Remove MultiPlatform Process abstractions ([#12725](#12725)) * [internal] add `JvmToolBase` for lockfile handling for JVM tools ([#13777](#13777)) * [internal] Port `MergeDigests` to Rust ([#13773](#13773)) * [jvm] Spawn nailgun servers outside the pool lock ([#13796](#13796)) * [internal] DRY loading internal Go binaries ([#13800](#13800)) * [internal] Convert unit tests to use pytest ([#13798](#13798)) * [internal] remove dead code - socket util. ([#13797](#13797)) * [internal] Reorganize Go parser scripts ([#13791](#13791)) * Adds Jackson core/datatype to `JVM_ARTIFACT_MAPPINGS` ([#13792](#13792)) * [internal] go: initial support for embedding resources ([#13743](#13743)) * [internal] Refer to `go.mod` path when downloading packages ([#13786](#13786)) * [internal] More robust Go dependency inference tests ([#13785](#13785)) * [internal] `tailor` doesn't add `go_package` for `testdata` folder ([#13783](#13783)) * [internal] Add Scala backend to dryrun for wheel builds. ([#13772](#13772)) * [internal] Unify JVM thirdparty resolves ([#13771](#13771)) * [internal] scala: infer dependencies from consumed symbols and intermediate scopes ([#13758](#13758)) * [internal] java: infer scala encoded symbols ([#13739](#13739)) * [internal] scala: parse and report package scopes ([#13738](#13738)) * [internal] go: configure included env vars for `GoSdkProcess` ([#13734](#13734)) * Fix some `./cargo audit` vulnerabilities. ([#13728](#13728)) * [internal] fix non-empty __init__.py ([#13730](#13730)) * Compute RepositoryPex directly from addresses. ([#13716](#13716)) * Upgrade to cargo-audit 0.16.0. ([#13729](#13729)) * Simplify `NativeHandler`. ([#13727](#13727)) * [internal] Switch to a maintained fork of the `fuse` crate for `brfs`. ([#13679](#13679)) * [internal] Add infrastructure to support deprecating field names ([#13666](#13666)) * [internal] Introduce OptionalPex/OptionalPexRequest. ([#13712](#13712)) * [internal] tailor adds go_package targets ([#13703](#13703)) * [internal] Remove unused testproject for pants-plugin ([#13704](#13704)) * [internal] Rename ambiguous `subpath` variable for Go code ([#13701](#13701)) * [internal] scala: generate the JVM names seen by Java code for Scala code ([#13696](#13696)) * Use RequirementsPexRequest in run_pex_binary.py. ([#13693](#13693)) * [internal] Refactor finding owning `go_mod` for first-party packages ([#13695](#13695)) * [internal] repl: add append_only_caches / run_in_workspace attributes to ReplRequest ([#13599](#13599)) * [internal] switch back to official `cargo-ensure-prefix` crate ([#13692](#13692)) * [internal] scala: extract type names from all Type.* AST nodes ([#13685](#13685)) * [internal] Convert unit tests to use pytest ([#13652](#13652)) * Unblock codegen support for java export analysis (#13645) ([#13675](#13675)) * [internal] upgrade to Rust 2021 Edition ([#13644](#13644)) * [internal] Don't store derived values on `go_first_party_package` targets ([#13676](#13676)) * Upgrade to py03 0.15.1. ([#13725](#13725)) * Add PyPDF2 to module mapping ([#13717](#13717)) * Upgrade console and indacatif. ([#13726](#13726))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #13787, the
input_filesdigest must currently include theuse_nailgundigest, and that means that the input files for the server are materialized for every run.InputDigestsstruct to encapsulate managing the collection of input digests for aProcess(which will soon also include the immutable digests from immutable cache design #12716), and split theuse_nailgundigest from theinput_filesdigest.std::processandstd::fs, and so is synchronous) onto theExecutor.javacprocesses whilescalacprocesses are active).Collectively, these changes make compilation 40% faster.
Fixes #13787.