From 0610f239b022bd6158043cf5b01995280ffd1251 Mon Sep 17 00:00:00 2001 From: Grzegorz Lukasik Date: Tue, 22 Feb 2022 21:10:44 -0800 Subject: [PATCH 1/4] Intern strings for python zip runfiles --- .../build/lib/bazel/rules/python/BazelPythonSemantics.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index 55824a57150b19..5ffe1befc413d0 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -370,9 +370,10 @@ private static void createPythonZipAction( for (Artifact artifact : runfilesSupport.getRunfilesArtifacts().toList()) { if (!artifact.equals(executable) && !artifact.equals(zipFile)) { argv.addDynamicString( - getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName, legacyExternalRunfiles) - + "=" - + artifact.getExecPathString()); + (getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName, legacyExternalRunfiles) + + "=" + + artifact.getExecPathString()) + .intern()); inputsBuilder.add(artifact); } } From 1a987d10dea3fdd699837ce32731e8bfd0b7898a Mon Sep 17 00:00:00 2001 From: Grzegorz Lukasik Date: Tue, 29 Mar 2022 14:44:12 -0700 Subject: [PATCH 2/4] Revert "Intern strings for python zip runfiles" This reverts commit 0610f239b022bd6158043cf5b01995280ffd1251. --- .../build/lib/bazel/rules/python/BazelPythonSemantics.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index f6d35739302145..a76ba940ad72d9 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -370,10 +370,9 @@ private static void createPythonZipAction( for (Artifact artifact : runfilesSupport.getRunfilesArtifacts().toList()) { if (!artifact.equals(executable) && !artifact.equals(zipFile)) { argv.addDynamicString( - (getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName, legacyExternalRunfiles) - + "=" - + artifact.getExecPathString()) - .intern()); + getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName, legacyExternalRunfiles) + + "=" + + artifact.getExecPathString()); inputsBuilder.add(artifact); } } From 513a1030598c32de5de71825340e32b2dccf4341 Mon Sep 17 00:00:00 2001 From: Grzegorz Lukasik Date: Sat, 26 Mar 2022 19:30:57 -0700 Subject: [PATCH 3/4] Intorude CommandLineItem.DefinedParamsCacheMapFn --- .../build/lib/actions/CommandLineItem.java | 17 ++++++ .../build/lib/bazel/rules/python/BUILD | 2 + .../rules/python/BazelPythonSemantics.java | 59 +++++++++++++++++-- .../nestedset/NestedSetFingerprintCache.java | 8 ++- 4 files changed, 81 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java index 7b983799f54bfe..d72eb89c90e7ee 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.actions; +import java.util.UUID; import java.util.function.Consumer; /** An interface for an object that customizes how it is expanded into a command line. */ @@ -57,6 +58,22 @@ abstract class ParametrizedMapFn implements MapFn { public abstract int maxInstancesAllowed(); } + /** + * Map function which may have multiple instances, where each instance differs only by a set of + * defined parameters. + * + *

Each instance needs to return a fingerprint of parameters it depends on.

+ */ + interface DefinedParamsCacheMapFn extends MapFn { + // Returns UUID common to all instances of DefinedParamsCacheMapFn. + UUID getUUID(); + + // Returns bytes representing parameters of this instance. + // Note, it could be to replace this method to addToFingerprint, but this need to be done + // carefully to avoid cyclic dependencies. + byte[] getParamsBytes(); + } + /** Expands the object into the command line as a string. */ String expandToCommandLine(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BUILD index dbcd04e8b0687b..bab811a2be909c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BUILD @@ -22,6 +22,7 @@ java_library( deps = [ "//src/main/java/com/google/devtools/build/lib/actions", "//src/main/java/com/google/devtools/build/lib/actions:artifacts", + "//src/main/java/com/google/devtools/build/lib/actions:commandline_item", "//src/main/java/com/google/devtools/build/lib/analysis:actions/custom_command_line", "//src/main/java/com/google/devtools/build/lib/analysis:actions/launcher_file_write_action", "//src/main/java/com/google/devtools/build/lib/analysis:actions/substitution", @@ -45,6 +46,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/packages", "//src/main/java/com/google/devtools/build/lib/rules/cpp", "//src/main/java/com/google/devtools/build/lib/rules/python", + "//src/main/java/com/google/devtools/build/lib/util", "//src/main/java/com/google/devtools/build/lib/util:os", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", "//src/main/java/com/google/devtools/common/options", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java index a76ba940ad72d9..424a3f0202d97b 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPythonSemantics.java @@ -20,6 +20,7 @@ import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.actions.Artifact; +import com.google.devtools.build.lib.actions.CommandLineItem; import com.google.devtools.build.lib.actions.ParamFileInfo; import com.google.devtools.build.lib.actions.ParameterFile; import com.google.devtools.build.lib.analysis.AnalysisUtils; @@ -32,6 +33,7 @@ import com.google.devtools.build.lib.analysis.ShToolchain; import com.google.devtools.build.lib.analysis.TransitiveInfoCollection; import com.google.devtools.build.lib.analysis.actions.CustomCommandLine; +import com.google.devtools.build.lib.analysis.actions.CustomCommandLine.VectorArg; import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction; import com.google.devtools.build.lib.analysis.actions.LauncherFileWriteAction.LaunchInfo; import com.google.devtools.build.lib.analysis.actions.SpawnAction; @@ -48,12 +50,15 @@ import com.google.devtools.build.lib.rules.python.PythonSemantics; import com.google.devtools.build.lib.rules.python.PythonUtils; import com.google.devtools.build.lib.rules.python.PythonVersion; +import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.PathFragment; import java.io.Serializable; import java.util.ArrayList; import java.util.Collection; import java.util.List; +import java.util.UUID; +import java.util.function.Consumer; import java.util.function.Predicate; import javax.annotation.Nullable; @@ -342,6 +347,52 @@ private static String getZipRunfilesPath( return getZipRunfilesPath(PathFragment.create(path), workspaceName, legacyExternalRunfiles); } + private static class ZipPathArgMapper implements CommandLineItem.DefinedParamsCacheMapFn { + private static final UUID MAPPER_UUID = UUID.fromString("a27af967-a391-4214-9e9d-14fd548ce063"); + + private Artifact executable; + private Artifact zipFile; + private PathFragment workspaceName; + private boolean legacyExternalRunfiles; + + ZipPathArgMapper( + Artifact executable, + Artifact zipFile, + PathFragment workspaceName, + boolean legacyExternalRunfiles + ) { + this.executable = executable; + this.zipFile = zipFile; + this.workspaceName = workspaceName; + this.legacyExternalRunfiles = legacyExternalRunfiles; + } + + @Override + public void expandToCommandLine(Artifact artifact, Consumer args) { + if (!artifact.equals(executable) && !artifact.equals(zipFile)) { + args.accept( + getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName, legacyExternalRunfiles) + + "=" + + artifact.getExecPathString()); + } + } + + @Override + public UUID getUUID() { + return MAPPER_UUID; + } + + @Override + public byte[] getParamsBytes() { + Fingerprint fp = new Fingerprint(); + fp.addString(executable.getExecPathString()); + fp.addString(zipFile.getExecPathString()); + fp.addPath(workspaceName); + fp.addBoolean(legacyExternalRunfiles); + return fp.digestAndReset(); + } + } + private static void createPythonZipAction( RuleContext ruleContext, Artifact executable, @@ -367,12 +418,12 @@ private static void createPythonZipAction( // Read each runfile from execute path, add them into zip file at the right runfiles path. // Filter the executable file, cause we are building it. + argv.addAll(VectorArg.of(runfilesSupport.getRunfilesArtifacts()).mapped( + new ZipPathArgMapper(executable, zipFile, workspaceName, legacyExternalRunfiles))); + + // TODO: Consider adding all files to avoid expanding nested sets. for (Artifact artifact : runfilesSupport.getRunfilesArtifacts().toList()) { if (!artifact.equals(executable) && !artifact.equals(zipFile)) { - argv.addDynamicString( - getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName, legacyExternalRunfiles) - + "=" - + artifact.getExecPathString()); inputsBuilder.add(artifact); } } diff --git a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java index 1611817797ad85..1d448bfbf56011 100644 --- a/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java +++ b/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java @@ -49,7 +49,13 @@ public void addNestedSetToFingerprint( fingerprint.addInt(EMPTY_SET_DIGEST); return; } - DigestMap digestMap = mapFnToDigestMap.computeIfAbsent(mapFn, this::newDigestMap); + DigestMap digestMap; + if (mapFn instanceof CommandLineItem.DefinedParamsCacheMapFn) { + fingerprint.addUUID(((CommandLineItem.DefinedParamsCacheMapFn) mapFn).getUUID()); + fingerprint.addBytes(((CommandLineItem.DefinedParamsCacheMapFn) mapFn).getParamsBytes()); + mapFn = CommandLineItem.MapFn.DEFAULT; + } + digestMap = mapFnToDigestMap.computeIfAbsent(mapFn, this::newDigestMap); fingerprint.addInt(nestedSet.getOrder().ordinal()); Object children = nestedSet.getChildren(); addToFingerprint(mapFn, fingerprint, digestMap, children); From 9d8585f43ae1d8a9ba19b0ef4af22746dde8ed32 Mon Sep 17 00:00:00 2001 From: Grzegorz Lukasik Date: Tue, 29 Mar 2022 14:55:52 -0700 Subject: [PATCH 4/4] Wording --- .../com/google/devtools/build/lib/actions/CommandLineItem.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java index d72eb89c90e7ee..e44e62c22f77bc 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java +++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java @@ -65,7 +65,7 @@ abstract class ParametrizedMapFn implements MapFn { *

Each instance needs to return a fingerprint of parameters it depends on.

*/ interface DefinedParamsCacheMapFn extends MapFn { - // Returns UUID common to all instances of DefinedParamsCacheMapFn. + // Returns UUID common to all instances of a given implementation of DefinedParamsCacheMapFn. UUID getUUID(); // Returns bytes representing parameters of this instance.