Skip to content

Support multiplex workers in ResourceProcessorBusyBox #14428

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.

Sign up for GitHub

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

Closed
Bencodes wants to merge 1 commit into from
Closed

Support multiplex workers in ResourceProcessorBusyBox #14428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions src/main/java/com/google/devtools/build/lib/actions/ExecutionRequirements.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,9 @@ public enum WorkerProtocolFormat {
public static final ImmutableMap<String, String> WORKER_MODE_ENABLED =
ImmutableMap.of(SUPPORTS_WORKERS, "1");

public static final ImmutableMap<String, String> WORKER_MULTIPLEX_MODE_ENABLED =
ImmutableMap.of(SUPPORTS_MULTIPLEX_WORKERS, "1");

/**
* Requires local execution without sandboxing for a spawn.
*
Expand Down
19 changes: 19 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/android/AndroidConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,6 +881,17 @@ public static class Options extends FragmentOptions {
help = "Tracking flag for when busybox workers are enabled.")
public boolean persistentBusyboxTools;

@Option(
name = "experimental_persistent_multiplex_busybox_tools",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {
OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS,
OptionEffectTag.EXECUTION,
},
defaultValue = "false",
help = "Tracking flag for when multiplex busybox workers are enabled.")
public boolean experimentalPersistentMultiplexBusyboxTools;

@Option(
name = "experimental_remove_r_classes_from_instrumentation_test_jar",
defaultValue = "true",
Expand Down Expand Up @@ -994,6 +1005,7 @@ public FragmentOptions getHost() {
host.oneVersionEnforcementUseTransitiveJarsForBinaryUnderTest =
oneVersionEnforcementUseTransitiveJarsForBinaryUnderTest;
host.persistentBusyboxTools = persistentBusyboxTools;
host.experimentalPersistentMultiplexBusyboxTools = experimentalPersistentMultiplexBusyboxTools;

// Unless the build was started from an Android device, host means MAIN.
host.configurationDistinguisher = ConfigurationDistinguisher.MAIN;
Expand Down Expand Up @@ -1039,6 +1051,7 @@ public FragmentOptions getHost() {
private final boolean dataBindingUpdatedArgs;
private final boolean dataBindingAndroidX;
private final boolean persistentBusyboxTools;
private final boolean experimentalPersistentMultiplexBusyboxTools;
private final boolean filterRJarsFromAndroidTest;
private final boolean removeRClassesFromInstrumentationTestJar;
private final boolean alwaysFilterDuplicateClassesFromAndroidTest;
Expand Down Expand Up @@ -1098,6 +1111,7 @@ public AndroidConfiguration(BuildOptions buildOptions) throws InvalidConfigurati
this.dataBindingUpdatedArgs = options.dataBindingUpdatedArgs;
this.dataBindingAndroidX = options.dataBindingAndroidX;
this.persistentBusyboxTools = options.persistentBusyboxTools;
this.experimentalPersistentMultiplexBusyboxTools = options.experimentalPersistentMultiplexBusyboxTools;
this.filterRJarsFromAndroidTest = options.filterRJarsFromAndroidTest;
this.removeRClassesFromInstrumentationTestJar =
options.removeRClassesFromInstrumentationTestJar;
Expand Down Expand Up @@ -1348,6 +1362,11 @@ public boolean persistentBusyboxTools() {
return persistentBusyboxTools;
}

@Override
public boolean persistentMultiplexBusyboxTools() {
return experimentalPersistentMultiplexBusyboxTools;
}

@Override
public boolean incompatibleUseToolchainResolution() {
return incompatibleUseToolchainResolution;
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/android/AndroidDataContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public class AndroidDataContext implements AndroidDataContextApi {
private final FilesToRunProvider busybox;
private final AndroidSdkProvider sdk;
private final boolean persistentBusyboxToolsEnabled;
private final boolean persistentMultiplexBusyboxTools;
private final boolean optOutOfResourcePathShortening;
private final boolean optOutOfResourceNameObfuscation;
private final boolean throwOnShrinkResources;
Expand All @@ -89,6 +90,7 @@ public static AndroidDataContext makeContext(RuleContext ruleContext) {
ruleContext,
ruleContext.getExecutablePrerequisite("$android_resources_busybox"),
androidConfig.persistentBusyboxTools(),
androidConfig.persistentMultiplexBusyboxTools(),
AndroidSdkProvider.fromRuleContext(ruleContext),
hasExemption(ruleContext, "allow_raw_access_to_resource_paths", false),
hasExemption(ruleContext, "allow_resource_name_obfuscation_opt_out", false),
Expand All @@ -113,6 +115,7 @@ protected AndroidDataContext(
RuleContext ruleContext,
FilesToRunProvider busybox,
boolean persistentBusyboxToolsEnabled,
boolean persistentMultiplexBusyboxTools,
AndroidSdkProvider sdk,
boolean optOutOfResourcePathShortening,
boolean optOutOfResourceNameObfuscation,
Expand All @@ -125,6 +128,7 @@ protected AndroidDataContext(
boolean includeProguardLocationReferences,
ImmutableMap<String, String> executionInfo) {
this.persistentBusyboxToolsEnabled = persistentBusyboxToolsEnabled;
this.persistentMultiplexBusyboxTools = persistentMultiplexBusyboxTools;
this.ruleContext = ruleContext;
this.busybox = busybox;
this.sdk = sdk;
Expand Down Expand Up @@ -216,6 +220,10 @@ public boolean isPersistentBusyboxToolsEnabled() {
return persistentBusyboxToolsEnabled;
}

public boolean isPersistentMultiplexBusyboxTools() {
return persistentMultiplexBusyboxTools;
}

public boolean optOutOfResourcePathShortening() {
return optOutOfResourcePathShortening;
}
Expand Down
8 changes: 5 additions & 3 deletions src/main/java/com/google/devtools/build/lib/rules/android/BusyBoxActionBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,10 +344,12 @@ public void buildAndRegister(String message, String mnemonic) {

if (dataContext.isPersistentBusyboxToolsEnabled()) {
commandLine.add("--logWarnings=false");
spawnActionBuilder
.addCommandLine(commandLine.build(), WORKERS_FORCED_PARAM_FILE_INFO);

spawnActionBuilder.addCommandLine(commandLine.build(), WORKERS_FORCED_PARAM_FILE_INFO);
executionInfo.putAll(ExecutionRequirements.WORKER_MODE_ENABLED);

if (dataContext.isPersistentMultiplexBusyboxTools()) {
executionInfo.putAll(ExecutionRequirements.WORKER_MULTIPLEX_MODE_ENABLED);
Copy link
Contributor

@larsrc-google larsrc-google Jan 10, 2022

Choose a reason for hiding this comment

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

So this flag would override --noexperimental_worker_multiplex?

Copy link
Contributor Author

@Bencodes Bencodes Jan 10, 2022

Choose a reason for hiding this comment

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

Passing --noexperimental_worker_multiplex would disable multiplex workers entirely while isPersistentMultiplexBusyboxTools just instructs BusyBoxActionBuilder to register it's actions with with supports-multiplex-worker specified as part of it's execution info.

Copy link
Contributor

@larsrc-google larsrc-google Jan 12, 2022

Choose a reason for hiding this comment

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

D'uh. You're right.

}
} else {
spawnActionBuilder.addCommandLine(commandLine.build(), FORCED_PARAM_FILE_INFO);
}
Expand Down
7 changes: 7 additions & 0 deletions .../java/com/google/devtools/build/lib/starlarkbuildapi/android/AndroidConfigurationApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,13 @@ public interface AndroidConfigurationApi extends StarlarkValue {
documented = false)
boolean persistentBusyboxTools();

@StarlarkMethod(
name = "experimental_persistent_multiplex_busybox_tools",
structField = true,
doc = "",
documented = false)
boolean persistentMultiplexBusyboxTools();

@StarlarkMethod(
name = "get_output_directory_name",
structField = true,
Expand Down
2 changes: 2 additions & 0 deletions src/tools/android/java/com/google/devtools/build/android/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ java_library(
":dependency_info",
"//src/java_tools/singlejar/java/com/google/devtools/build/singlejar:libSingleJar",
"//src/java_tools/singlejar/java/com/google/devtools/build/zip",
"//src/main/java/com/google/devtools/build/lib/worker",
"//src/main/java/com/google/devtools/build/lib/worker:work_request_handlers",
"//src/main/java/com/google/devtools/common/options",
"//src/main/protobuf:worker_protocol_java_proto",
"//src/tools/android/java/com/google/devtools/build/android/junctions",
Expand Down
78 changes: 47 additions & 31 deletions src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

import com.google.devtools.build.android.aapt2.Aapt2Exception;
import com.google.devtools.build.android.resources.JavaIdentifierValidator.InvalidJavaIdentifier;
import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest;
import com.google.devtools.build.lib.worker.WorkerProtocol.WorkResponse;
import com.google.devtools.build.lib.worker.ProtoWorkerMessageProcessor;
import com.google.devtools.build.lib.worker.WorkRequestHandler;
import com.google.devtools.common.options.EnumConverter;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionDocumentationCategory;
Expand All @@ -26,11 +26,14 @@
import com.google.devtools.common.options.OptionsParser;
import com.google.devtools.common.options.OptionsParsingException;
import com.google.devtools.common.options.ShellQuotedParamsFilePreProcessor;

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.nio.file.FileSystems;
import java.time.Duration;
import java.util.Arrays;
import java.util.List;
import java.util.Properties;
Expand Down Expand Up @@ -183,43 +186,56 @@ private static int runPersistentWorker() throws Exception {
PrintStream ps = new PrintStream(buf, true);
PrintStream realStdOut = System.out;
PrintStream realStdErr = System.err;
try {
// Redirect all stdout and stderr output for logging.
System.setOut(ps);
System.setErr(ps);

while (true) {
try {
WorkRequest request = WorkRequest.parseDelimitedFrom(System.in);
if (request == null) {
break;
}

int exitCode = processRequest(request.getArgumentsList());
ps.flush();

WorkResponse.newBuilder()
.setExitCode(exitCode)
.setRequestId(request.getRequestId())
.setOutput(buf.toString())
.build()
.writeDelimitedTo(realStdOut);

realStdOut.flush();
buf.reset();
} catch (IOException e) {
logger.severe(e.getMessage());
e.printStackTrace(realStdErr);
return 1;
}
}
// Redirect all stdout and stderr output for logging.
System.setOut(ps);
System.setErr(ps);
try {
WorkRequestHandler workerHandler =
new WorkRequestHandler.WorkRequestHandlerBuilder(
new WorkRequestHandler.WorkRequestCallback(
(request, pw) -> processRequest(request.getArgumentsList(), pw, buf)),
realStdErr,
new ProtoWorkerMessageProcessor(System.in, realStdOut))
.setCpuUsageBeforeGc(Duration.ofSeconds(10))
.build();
workerHandler.processRequests();
} catch (IOException e) {
logger.severe(e.getMessage());
e.printStackTrace(realStdErr);
return 1;
} finally {
System.setOut(realStdOut);
System.setErr(realStdErr);
}
return 0;
}

/**
* Processes the request for the given args and writes the captured byte array buffer to the
* WorkRequestHandler print writer.
*/
private static int processRequest(List<String> args, PrintWriter pw, ByteArrayOutputStream buf) {
int exitCode;
try {
// Process the actual request and grab the exit code
exitCode = processRequest(args);
} catch (Exception e) {
e.printStackTrace(pw);
exitCode = 1;
} finally {
// Write the captured buffer to the work response. We synchronize to avoid race conditions
// while reading from and calling reset on the shared ByteArrayOutputStream.
synchronized (buf) {
String captured = buf.toString().trim();
buf.reset();
pw.print(captured);
Copy link
Contributor Author

@Bencodes Bencodes Jan 24, 2022

Choose a reason for hiding this comment

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

This preserves the previous logging behavior seen here https://github.com/bazelbuild/bazel/pull/14428/files#diff-6bbb0aaf4cf2d425734021815617e2b6203223abe7dc76ca8030a90e9f043873L204

}
}

return exitCode;
}

private static int processRequest(List<String> args) throws Exception {
OptionsParser optionsParser =
OptionsParser.builder()
Expand Down