remote: re-implement merkle tree buildling

[buchgr]

The TreeNodeRepository served us well, but has become a bit dated over
time and it's increasingly hard to understand and to fix bugs. Additionally,
it was written with the initial intend of incrementally updating the merkle
tree between actions, however internal performance analysis has shown
that doing so is 1) hard to implement reliably and 2) would increase
memory consumption multifold.

This is a rewrite of the code that also fixes bugs like #4663. The
merkle tree construction is split into two:

• InputTree is an intermediate tree representation of a (sorted) list
  of inputs.
• MerkleTree implements a visitor pattern over an InputTree and is
  responsible for serializing the InputTree to the protobuf merkle
  tree that's used by remote caching / execution.

Benchmarks on my local machine have shown this implementation to
consumes between 2-10x less CPU time than the current implementation.
Tests by users have shown equally encouraging speedups [1].

[1] https://groups.google.com/d/msg/bazel-discuss/wPHrqm2z8lU/mzoRF236GQAJ

[ulfjack]

s/MerkleTree.//?

[buchgr]

done

[ulfjack]

I wonder if there's a way to just take a plain list of ActionInputs and efficiently build the merkle tree directly from that. This is using a map, not a list, and the DirectoryNode instances seem to contain all nested elements.

[buchgr]

I believe there is for a lexicographically sorted list and it should be fairly easy to implement. However, what InputTree mostly is for is to expand directory inputs. I think after this change we could move the directory expansion to the SpawnInputExpander and after that we can build a merkle tree directly from a list of ActionInputs.

[buchgr]

@benjaminp iirc you mentioned that you are not using the TreeNodeRepository for performance reasons. If so, I'd love to get your feedback about this PR.

[benjaminp]

Yeah, we replace the merkle tree hashing with the following:

    Hasher hasher = Hashing.sha256().newHasher();
    Iterable<? extends ActionInput> spawnInputs = spawn.getInputFiles();
    for (ActionInput input : spawnInputs) {
      hasher.putBytes(input.getExecPathString().getBytes(StandardCharsets.UTF_8));
      if (input instanceof VirtualActionInput) {
        ((VirtualActionInput) input).writeTo(Funnels.asOutputStream(hasher));
        continue;
      }
      hasher.putBytes(getMetadataProvider().getMetadata(input).getDigest());
    }

A subtle but important property of the above is that it exploits input aggregation that Bazel has already done. In particular, since runfiles trees are behind a middlemen artifact, you don't end up rehashing every runfile for every action a particular runfiles tree is in. This optimization greatly helps static asset pipelines, which often involve thousands of actions consisting of one 40,000 file tool that converts one input file to one output file. (See #3217 for a related example of how the wonderful world of web static asset processing can stress Bazel.)

It seems it would be hard to easily do better than our implementation in pure hashing efficiency given that it's a straightforward for loop over the inputs. That said, if the performance improvements you mention in the summary hold for us, this PR may be good enough. I do love to drop local patches.

[buchgr]

@benjaminp thanks! I suppose we could also use the code snippet you posted for remote caching (which I understand you use), as technically it's really only remote execution that requires a merkle tree. I'd be happy to review a patch :-).