Intern strings for python zip runfiles

[glukasiknuro]

Provides substantial memory savings for repos using heavily python dependencies.

Possibly even better change could be created that avoids creating those strings, but this small change by itself provides most of the benefits without to much risk.

See #14890

[comius]

You should use BlazeInterners.weakInterner for this, like the rest of the codebase. https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/concurrent/BlazeInterners.java;l=38;drc=ec8fb7969614c6b3752392089e2310708d103225

Interning is not available in Starlark, so this will have to be optimised in a different way.

Even better would be if you replaced call argv.addDynamicString with one of the calls that accepts artifact.getExecPath() as an argument and set format to getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName, legacyExternalRunfiles) + "=".

Can you also provide some benchmarking results? baseline vs the change?
I'm mostly interested in used-heap-size-after-gc displayed by bazel info after building the project. (You should do 1 warm-up run before collecting the results).

Unfortunately I don't have appropriate project to run benchmarks for you.

[glukasiknuro]

Benchmarks are in #14890, see especially #14890 (comment)

The used-heap-size-after-gc decreased for part of tensorflow from 669MB to 335MB

[comius]

Nice results!

Those can be better + Starlarkifiable. Using lazy argument construction, Bazel wouldn't retain any of those strings.

Change the whole for loop to something like:
argv.addAll(VectorArg.of(runfilesSupport.getRunfilesArtifacts()).mapped(MAP_FN))

where

MAPFN = (artifact, args) ->
{ 
if (!artifact.equals(executable) && !artifact.equals(zipFile)) {
          args.accept(getZipRunfilesPath(artifact.getRunfilesPath(), workspaceName, legacyExternalRunfiles)
                    + "="
                    + artifact.getExecPathString());
     }
}

Example:
https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/java/OneVersionCheckActionBuilder.java;l=121-128;drc=103ea9d3f3a0d85a0d7c1ad98fd8dbb422505d95

[glukasiknuro]

Simply using mapfn ends up with CommandLineItem.MapFn instances must be singletons, from here:

bazel/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java

Line 110 in 1799842

"Illegal mapFn implementation: '%s'. "

Using ParametrizedMapFn requires specifying INT_MAX in maxInstancesAllowed

bazel/src/main/java/com/google/devtools/build/lib/actions/CommandLineItem.java

Line 57 in 7fcc581

public abstract int maxInstancesAllowed();

which I hesitate for now to use before understanding consequences.

Right now I think the main problem with using big number of instances is that separate cache is created for each instance of map function here:

bazel/src/main/java/com/google/devtools/build/lib/collect/nestedset/NestedSetFingerprintCache.java

Line 52 in 1799842

DigestMap digestMap = mapFnToDigestMap.computeIfAbsent(mapFn, this::newDigestMap);

Which unnecessary caches fingerprints for children nested sets. Potentially I see two solutions for time being:

• avoid creating cache at all for certain map functions
• change the fingerprint function so that it will return: fingerprint(map_fn_id + map_fn_params) + vanilla fingerprint(nested set)

[glukasiknuro]

Implemented second solution by adding CommandLineItem.DefinedParamsCacheMapFn , PTAL

[glukasiknuro]

Also, CC @alex-torok that created #15037 using CapturingMapFn that was removed from master branch.

[comius]

I removed CapturingMapFn, because it was unused. I think the solution using it would be nice. Let me rollback this removal.

[comius]

A sec, before I rollback removal of CapturingMapFn.

Using ParametrizedMapFn requires specifying INT_MAX in maxInstancesAllowed

When implementing this in Starlark it uses ParametrizedMapFn with maxInstancesAllowed set to Integer.MAX_VALUE. See https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkCustomCommandLine.java;l=893-976;drc=8f0034ce1e7854521e259a5eaf71859b1e6f95bc

The requirement in ParametrizedMapFn is:

  * <p>The user promises that the number of distinct instances constructed is closer to O(rule
   * class count) than O(rule count).

In any case, I'd prefer if we don't add new things to CommandLineItem and NestedSetFingerprintCache

[comius]

Rolled back removal of CapturingMapFn in c70e072

[comius]

@comius friendly ping. Do you think we can proceed with this PR?

There's a second PR that handles this with CapturingMapFn: #15037

[comius]

Friendly ping.

[glukasiknuro]

@comius friendly ping. Do you think we can proceed with this PR?

[comius]

Since the second PR (#15037) has been merged I'll close this one.