Path map command lines lazily during expansion #25368
Conversation
fmeum
force-pushed
the
lazy-path-mapping
branch
from
ae62071 to
f905c9a
Compare
fmeum
force-pushed
the
lazy-path-mapping
branch
from
3722f1f to
10d042e
Compare
src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapper.java
Show resolved
Hide resolved
github-actions
bot
added
team-Performance
|
@gregestren @aranguyen are there still plans to enable path mapping in google? Trying to decide how closely I should look at this PR. |
A measurement team just added it as a candidate to estimate value. If that plays out with positive results, then yes. See b/399115992. |
|
@fmeum what is the bazel motivation for this change? It's a lot of complexity for a feature that we're not sure whether we are going to enable for blaze. I'm leaning toward deferring the review until after we make a go/no-go decision. |
|
@justinhorvitz While path mapping hasn't been enabled inside Google yet, it's grown pretty popular (for an experimental feature) in Bazel, especially since I added support for C++ actions. #22658 has a number of user stories. The concrete motivation for this PR is that I had a large scale user reach out to me about path mapping. They encountered OOMs with path mapping enabled that weren't showing up without it. As far as I can tell, this PR should resolve the last remaining memory inefficiency of path mapping, which should help them adopt it. If you encounter any issues in this part of the code in the future, I'm always open to being assigned to them and preparing fixes that you would then only need to review. Let me know if you need further information on the state of path mapping in Bazel. |
|
I agree this feature has already proven value for Bazel and is getting more popular. I can bear more weight for that part of the review but I don't think we should discourage Bazel adoption. |
|
Thanks for the context. I'll look for time to review this soon. |
| /** | ||
| * Returns the length of the mapped path minus the length of the unmapped path. | ||
| * | ||
| * <p>Implementations should provide a more efficient implementation that avoids allocations. |
There was a problem hiding this comment.
Is there anything that uses this default impl? Maybe just test implementations?
| * | ||
| * <p>Implementations should provide a more efficient implementation that avoids allocations. | ||
| */ | ||
| default int computeExecPathLengthDiff(Artifact.DerivedArtifact artifact) { |
There was a problem hiding this comment.
Import DerivedArtifact instead of qualifying it. A qualifier is not needed if the inner type is unambiguous.
| stringValues.set(addIndex++, val); | ||
| Object /* String | DerivedArtifact */ val = values.get(i); | ||
| // If the path mapper is a no-op, an artifact behaves just like its (trivially mapped) | ||
| // exec path string.If the path mapper is not a no-op, mapped paths are always distinct |
There was a problem hiding this comment.
Missing space before "If"
| // exec path string.If the path mapper is not a no-op, mapped paths are always distinct | ||
| // from unmapped paths. We can thus uniquify based on the mapped exec path string in each | ||
| // case. | ||
| String valToDeduplicateBy = |
There was a problem hiding this comment.
Why deduplicate based on mapped strings instead of unmapped exec paths? Are you just trying to preserve existing behavior, or is it ever expected that two unique artifacts map to the same path (or even weirder, an artifact maps to a path that matches a raw string argument)?
| @@ -1423,8 +1432,13 @@ void addPreprocessedArg(PreprocessedArg arg) { | |||
| numArgs += arg.numArgs(); | |||
| } | |||
|
|
|||
| void addString(String arg) { | |||
There was a problem hiding this comment.
Can you keep this method? Some callers still have a String reference, for which it helps performance to avoid the type check, and enhances readability that it's definitely a string.
| String stringValue = | ||
| StarlarkCustomCommandLine.expandToCommandLine(object, pathMapper, mainRepoMapping); | ||
| builder.addPreprocessedArg(new PreprocessedSingleFormattedArg(formatStr, stringValue)); | ||
| switch (StarlarkCustomCommandLine.expandToCommandLine(object, mainRepoMapping)) { |
There was a problem hiding this comment.
Unnecessary qualifier to the enclosing class.
| @@ -105,6 +105,16 @@ default String getMappedExecPathString(ActionInput artifact) { | |||
| return map(artifact.getExecPath()).getPathString(); | |||
| } | |||
|
|
|||
| /** | |||
| * Returns the length of the mapped path minus the length of the unmapped path. | |||
There was a problem hiding this comment.
I got confused - this is actually supposed to return a negative number when the mapped path is shorter (what we expect). I think it makes more sense to deal with positive numbers. What do you think about making this return |unmapped| - |mapped| instead? Either way, please make it super clear in the documentation.
| @@ -145,7 +145,9 @@ static Optional<PathMapper> tryCreate(AbstractAction action, boolean isStarlarkA | |||
|
|
|||
| @Override | |||
| public String getMappedExecPathString(ActionInput artifact) { | |||
| if (isSupportedInputType(artifact) && isOutputPath(artifact, outputRoot)) { | |||
| if (artifact instanceof DerivedArtifact | |||
There was a problem hiding this comment.
I found this easier to read with the helper method. Perhaps keep an isSupported(artifact) that does both the type check and calls isOutputPath.
This removes a peak memory inefficiency of path mapping compared to regular execution.