Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP, pending performance investigations] remote hermetic experiments revisited #7231

Closed
wants to merge 23 commits into from

Conversation

cosmicexplorer
Copy link
Contributor

This is based off of #7227: see https://github.com/cosmicexplorer/pants/compare/cosmicexplorer:mixed-rsc-zinc-compiles...cosmicexplorer:remoting-FINAL?expand=1. The previous commits will be removed when that PR is merged.

The scala edits in this commit should be unchanged from twitter@3e798c1.

Problem

One interesting possibility for distributed rsc compilation is to have individual remote execution workers maintain a nailgun instance which individual process executions connect to. This removes JVM startup time and allows the JIT to warm up (vaguely). This is pretty difficult to set up (as described below) but if the relative performance is convincing, may deserve more investigation.

(explain the context of the problem and why you're making this change. include
references to all relevant github issues.
)

Solution

  • Edit the pants zinc wrapper to convert non-absolute paths on the provided classpath to be relative to the working directory provided by the nailgun connection, as opposed to the directory where the nailgun server was invoked in.
  • Introduce hermetic-with-nailgun execution strategy to connect to a remote execution server which is running its own nailgun server in the background.
    • This nailgun server should have the jar produced by ./pants binary src/scala/org/pantsbuild/zinc/compiler:rsc-and-zinc on its classpath.
    • The remote execution worker is expected to set NAILGUN_PORT in its execution sandbox -- this is difficult to set up and likely insecure, along with other concerns I am less familiar with.

Please look at the --ng-client-input-digest and --ng-client-input-digest-length option descriptions to understand the requirements for the ng command-line client used to perform this connection: https://github.com/cosmicexplorer/pants/blob/a33b43dc15898a71c7f8e8f41478da5564a0fbc2/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py#L178-L188

(describe the modifications you've made.)

Result

(describe how your changes affect the end-user behavior of the system. this section is
optional, and should generally be summarized in the title of the pull request.
)

@cosmicexplorer cosmicexplorer changed the title [WIP] remote hermetic experiments revisited [WIP, pending performance investigations] remote hermetic experiments revisited Feb 9, 2019
@cosmicexplorer cosmicexplorer force-pushed the remoting-FINAL branch 4 times, most recently from dc52e7f to 4c0b5b3 Compare February 11, 2019 08:29
cosmicexplorer and others added 23 commits February 11, 2019 22:09
…exceptions

- this also requires none_is_default=True to be set in .create() calls to coerce None
…gration.py

(this makes addressing pantsbuild#6866 later a little easier)
…exceptions

- this also requires none_is_default=True to be set in .create() calls to coerce None
add experimental hermetic-with-nailgun jvm execution strategy

upstream all the jvm task changes

make rsc jobs populate the runtime_classpath for subsequent zinc jobs!

mixed mode compilation 100% works

mjar -> jar

start outling compiling java with rsc

revert hermetic with nailgun changes

[BROKEN] revert enum changes and process stringification

Revert "add experimental hermetic-with-nailgun jvm execution strategy"

This reverts commit 6fd30defa11622ac13b92768eabe15d1b94df556.

extraneous edits to support the new enum

fix lint
…exceptions

- this also requires none_is_default=True to be set in .create() calls to coerce None
@cosmicexplorer
Copy link
Contributor Author

Closed for now, the commits should be easily cherry-pickable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants