Skip to content

turbine support worker mode #23

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ package(

java_binary(
name = "turbine_direct_binary",
main_class = "com.google.turbine.main.Main",
main_class = "com.google.devtools.build.java.turbine.TurbineWorkerWrapper",
srcs = ["TurbineWorkerWrapper.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/worker:work_request_handlers",
"//third_party:turbine",
],
runtime_deps = [
"//src/main/protobuf:deps_java_proto",
"//third_party:guava",
"//third_party:jsr305",
"//third_party:turbine",
],
)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package com.google.devtools.build.java.turbine;

import com.google.turbine.main.Main;
import com.google.devtools.build.lib.worker.ProtoWorkerMessageProcessor;
import com.google.devtools.build.lib.worker.WorkRequestHandler;

import java.io.IOException;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.time.Duration;
import java.util.List;

/**
* A Wrapper for Turbine to support multiplex worker
*/
public class TurbineWorkerWrapper {

public static void main(String[] args) throws IOException {
if (args.length == 1 && args[0].equals("--persistent_worker")) {
PrintStream realStdErr = System.err;
WorkRequestHandler workerHandler =
new WorkRequestHandler.WorkRequestHandlerBuilder(
new WorkRequestHandler.WorkRequestCallback(
(request, pw) ->
turbine(request.getArgumentsList(), pw)),
realStdErr,
new ProtoWorkerMessageProcessor(System.in, System.out))
.setCpuUsageBeforeGc(Duration.ofSeconds(10))
.build();
int exitCode = 1;
try {
workerHandler.processRequests();
exitCode = 0;
} catch (IOException e) {
realStdErr.println(e.getMessage());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider logging the full stack trace of the IOException for better debugging. Printing only the message might not be sufficient to diagnose the root cause.

Suggested change
realStdErr.println(e.getMessage());
realStdErr.println(e.getMessage());
e.printStackTrace(realStdErr); // Add this line

} finally {
// Prevent hanging threads from keeping the worker alive.
System.exit(exitCode);
}
} else {
Main.main(args);
}
}

private static int turbine(List<String> args, PrintWriter pw) {
try {
Main.compile(args.toArray(new String[0]));
} catch (Throwable e) {
pw.println(e.getMessage());
return 1;
}
return 0;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public static Builder newBuilder(RuleContext ruleContext) {
public static final class Builder {

private static final ParamFileInfo PARAM_FILE_INFO =
ParamFileInfo.builder(UNQUOTED).setCharset(ISO_8859_1).build();
ParamFileInfo.builder(UNQUOTED).setCharset(ISO_8859_1).setUseAlways(true).build();

private final RuleContext ruleContext;

Expand Down Expand Up @@ -449,14 +449,21 @@ public void build(JavaToolchainProvider javaToolchain) {
}
}

ImmutableMap<String, String> executionInfo =
TargetUtils.getExecutionInfo(ruleContext.getRule(), ruleContext.isAllowTagsPropagation());
ImmutableMap.Builder<String, String> executionInfoBuilder = ImmutableMap.builder();
executionInfoBuilder.putAll(
TargetUtils.getExecutionInfo(ruleContext.getRule(), ruleContext.isAllowTagsPropagation()));
if (javaConfiguration.inmemoryJdepsFiles()) {
executionInfo =
ImmutableMap.of(
ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS,
outputDepsProto.getExecPathString());
executionInfoBuilder.put(
ExecutionRequirements.REMOTE_EXECUTION_INLINE_OUTPUTS,
outputDepsProto.getExecPathString());
}
if (javaToolchain.getHeaderCompilerSupportsWorkers()) {
executionInfoBuilder.putAll(ExecutionRequirements.WORKER_MODE_ENABLED);
}
if (javaToolchain.getHeaderCompilerSupportsMultiplexWorkers()) {
executionInfoBuilder.putAll(ExecutionRequirements.WORKER_MULTIPLEX_MODE_ENABLED);
}
ImmutableMap<String, String> executionInfo = executionInfoBuilder.build();
if (useDirectClasspath) {
Comment on lines +452 to 467
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Possible duplicate-key crash when rule tags already enable worker mode

TargetUtils.getExecutionInfo() may already return the key contained in
ExecutionRequirements.WORKER_MODE_ENABLED (typically "supports-workers":"1").

Calling executionInfoBuilder.putAll() twice with the same key will raise an
IllegalArgumentException at build time because ImmutableMap.Builder forbids duplicates, even
when the value is identical.

-      if (javaToolchain.getHeaderCompilerSupportsWorkers()) {
-        executionInfoBuilder.putAll(ExecutionRequirements.WORKER_MODE_ENABLED);
-      }
+      if (javaToolchain.getHeaderCompilerSupportsWorkers()
+          && !executionInfoBuilder.buildOrThrow()
+              .containsKey(ExecutionRequirements.WORKER_MODE_ENABLED.keySet().iterator().next())) {
+        executionInfoBuilder.putAll(ExecutionRequirements.WORKER_MODE_ENABLED);
+      }

(The same guard is needed for the multiplex variant.)

Failing fast here would surface as an unexplained analysis exception to users.

Committable suggestion skipped: line range outside the PR's diff.

NestedSet<Artifact> classpath;
if (!directJars.isEmpty() || classpathEntries.isEmpty()) {
Expand Down Expand Up @@ -485,7 +492,7 @@ public void build(JavaToolchainProvider javaToolchain) {
ruleContext.registerAction(
new JavaHeaderCompileAction(
/* owner= */ ruleContext.getActionOwner(),
/* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
/* tools= */ headerCompiler.tool().getFilesToRun(),
/* inputs= */ allInputs,
/* outputs= */ outputs.build(),
/* resourceSetOrBuilder= */ AbstractAction.DEFAULT_RESOURCE_SET,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ public ConfiguredTarget create(RuleContext ruleContext)
ruleContext.attributes().get("javac_supports_multiplex_workers", Type.BOOLEAN);
boolean javacSupportsWorkerCancellation =
ruleContext.attributes().get("javac_supports_worker_cancellation", Type.BOOLEAN);
boolean headerCompilerSupportsWorkers =
ruleContext.attributes().get("header_compiler_supports_workers", Type.BOOLEAN);
boolean headerCompilerSupportsMultiplexWorkers =
ruleContext.attributes().get("header_compiler_supports_multiplex_workers", Type.BOOLEAN);
ImmutableSet<String> headerCompilerBuiltinProcessors =
ImmutableSet.copyOf(
ruleContext.attributes().get("header_compiler_builtin_processors", Type.STRING_LIST));
Expand Down Expand Up @@ -168,6 +172,8 @@ public ConfiguredTarget create(RuleContext ruleContext)
javacSupportsWorkers,
javacSupportsMultiplexWorkers,
javacSupportsWorkerCancellation,
headerCompilerSupportsWorkers,
headerCompilerSupportsMultiplexWorkers,
bootclasspath,
tools,
javabuilder,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ public static JavaToolchainProvider create(
boolean javacSupportsWorkers,
boolean javacSupportsMultiplexWorkers,
boolean javacSupportsWorkerCancellation,
boolean headerCompilerSupportsWorkers,
boolean headerCompilerSupportsMultiplexWorkers,
BootClassPathInfo bootclasspath,
NestedSet<Artifact> tools,
JavaToolchainTool javaBuilder,
Expand Down Expand Up @@ -149,6 +151,8 @@ public static JavaToolchainProvider create(
javacSupportsWorkers,
javacSupportsMultiplexWorkers,
javacSupportsWorkerCancellation,
headerCompilerSupportsWorkers,
headerCompilerSupportsMultiplexWorkers,
packageConfiguration,
jacocoRunner,
proguardAllowlister,
Expand Down Expand Up @@ -182,6 +186,8 @@ public static JavaToolchainProvider create(
private final boolean javacSupportsWorkers;
private final boolean javacSupportsMultiplexWorkers;
private final boolean javacSupportsWorkerCancellation;
private final boolean headerCompilerSupportsWorkers;
private final boolean headerCompilerSupportsMultiplexWorkers;
private final ImmutableList<JavaPackageConfigurationProvider> packageConfiguration;
private final FilesToRunProvider jacocoRunner;
private final FilesToRunProvider proguardAllowlister;
Expand Down Expand Up @@ -215,6 +221,8 @@ private JavaToolchainProvider(
boolean javacSupportsWorkers,
boolean javacSupportsMultiplexWorkers,
boolean javacSupportsWorkerCancellation,
boolean headerCompilerSupportsWorkers,
boolean headerCompilerSupportsMultiplexWorkers,
ImmutableList<JavaPackageConfigurationProvider> packageConfiguration,
FilesToRunProvider jacocoRunner,
FilesToRunProvider proguardAllowlister,
Expand Down Expand Up @@ -247,6 +255,8 @@ private JavaToolchainProvider(
this.javacSupportsWorkers = javacSupportsWorkers;
this.javacSupportsMultiplexWorkers = javacSupportsMultiplexWorkers;
this.javacSupportsWorkerCancellation = javacSupportsWorkerCancellation;
this.headerCompilerSupportsWorkers = headerCompilerSupportsWorkers;
this.headerCompilerSupportsMultiplexWorkers = headerCompilerSupportsMultiplexWorkers;
this.packageConfiguration = packageConfiguration;
this.jacocoRunner = jacocoRunner;
this.proguardAllowlister = proguardAllowlister;
Expand Down Expand Up @@ -419,6 +429,16 @@ public boolean getJavacSupportsMultiplexWorkers() {
return javacSupportsMultiplexWorkers;
}

/** Returns whether JavaHeaderCompiler supports running as a persistent worker or not. */
public boolean getHeaderCompilerSupportsWorkers() {
return headerCompilerSupportsWorkers;
}

/** Returns whether JavaHeaderCompiler supports running persistent workers in multiplex mode */
public boolean getHeaderCompilerSupportsMultiplexWorkers() {
return headerCompilerSupportsMultiplexWorkers;
}

/** Returns whether JavaBuilders supports running persistent workers with cancellation */
public boolean getJavacSupportsWorkerCancellation() {
return javacSupportsWorkerCancellation;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,14 @@ The Java target version (e.g., '6' or '7'). It specifies for which Java runtime
True if JavaBuilder supports cancellation of persistent workers, false if it doesn't.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr("javac_supports_worker_cancellation", BOOLEAN).value(true))
/* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(header_compiler_supports_workers) -->
True if JavaHeaderCompiler supports running as a persistent worker, false if it doesn't.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr("header_compiler_supports_workers", BOOLEAN).value(false))
/* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(header_compiler_supports_multiplex_workers) -->
True if JavaHeaderCompiler supports running as a multiplex persistent worker, false if it doesn't.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr("header_compiler_supports_multiplex_workers", BOOLEAN).value(false))
Comment on lines +135 to +139

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Consider setting the default value of header_compiler_supports_workers and header_compiler_supports_multiplex_workers to True to enable worker mode by default, if it's stable and recommended.

Suggested change
.add(attr("header_compiler_supports_workers", BOOLEAN).value(false))
/* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(header_compiler_supports_multiplex_workers) -->
True if JavaHeaderCompiler supports running as a multiplex persistent worker, false if it doesn't.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr("header_compiler_supports_multiplex_workers", BOOLEAN).value(false))
.add(attr("header_compiler_supports_workers", BOOLEAN).value(True))
/* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(header_compiler_supports_multiplex_workers) -->
True if JavaHeaderCompiler supports running as a multiplex persistent worker, false if it doesn't.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
.add(attr("header_compiler_supports_multiplex_workers", BOOLEAN).value(True))

/* <!-- #BLAZE_RULE(java_toolchain).ATTRIBUTE(tools) -->
Labels of tools available for label-expansion in jvm_opts.
<!-- #END_BLAZE_RULE.ATTRIBUTE --> */
Expand Down
Loading