Add BLAKE3 hasher to vfs #18784

tylerwilliams · 2023-06-26T23:42:04Z

Add BLAKE3 hasher to vfs

This PR adds the Blake3Hasher and Blake3HashFunction classes to vfs and
makes them available under the flag --digest_function=BLAKE3.

I spent a little time trying to optimize performance more here, and am happy to
chat about different approaches (ByteBuffer vs byte[] vs unsafe vs mmap). In the
end this CL implements something pretty conservative, but there is room to get
more performance out of this with a different approach.

This is a partial commit for #18658.

tylerwilliams · 2023-06-27T00:37:14Z

@coeuvre -- Could use your advice on how to fix:

//src/test/shell/bazel:bazel_bootstrap_distfile_test is failing, how is this supposed to interact with dist_http_archive? It seemed like that test should be able to run w/o network enabled -- how do I make it aware of the dist_http_archive sources?
Clang on :ubuntu: 18.04 LTS (OpenJDK 11, gcc 7.5.0) -- If we can disable Ubuntu 18 (it's already EOLd), this should just go away. If not, I think possibly a repository_rule could work here to disable including AVX512 on the old ubuntu18 / gcc versions, but this is more complicated. This code is linked in but only used if the CPU supports it -- it's just that this old OS + compiler do not even support that flag afaik.

Thanks!

coeuvre · 2023-06-27T08:50:36Z

For 1, yes the test is to verify Bazel can be built without network. You need to add blake3 to distdir_deps.bzl.

@meteorcloudy what's your opinion on 2?

coeuvre · 2023-06-27T10:12:31Z

src/main/java/com/google/devtools/build/lib/vfs/DigestHashFunction.java

@@ -82,8 +73,6 @@ private DigestHashFunction(
    checkArgument(!names.isEmpty());
    this.name = names.get(0);
    this.names = names;
-    this.messageDigestPrototype = getMessageDigestInstance();
-    this.messageDigestPrototypeSupportsClone = supportsClone(messageDigestPrototype);
  }

  public static DigestHashFunction register(


We cannot change how the registration works here because we rely on this internally. You could, however, glue BLAKE3 into this with following steps:

Write a class that implements java.security.MessageDigest for BLAKE3. This is the class that interact with JNI and call into c code.

Register the MessageDigest from step 1 by implementing a java.security.Provider and add it with java.security.Security.addProvider(...). So that when you call MessageDigest.getInstance("BLAKE3"), you can get back the instance from the provider.

Adapt DigestMessage for BLAKE3 into HashFunction by calling MessageDigestHashFunction.

Create a DigestHashFunction by calling DigestHashFunction.register(...) using the HashFunction from step 3.

I know this is verbose and not fun but we have to write it in this way.

meteorcloudy · 2023-06-27T12:34:49Z

For the bazel_bootstrap_distfile_test, the issue is subtle:

bazel/distdir_deps.bzl

Lines 276 to 281 in 75b0221

    
           "blake3": { 
        
               "archive": "v1.3.3.zip", 
        
               "sha256": "bb529ba133c0256df49139bd403c17835edbf60d2ecd6463549c6a5fe279364d", 
        
               "urls": [ 
        
                   "https://github.com/BLAKE3-team/BLAKE3/archive/refs/tags/1.3.3.zip", 
        
               ],

"archive": "v1.3.3.zip", should be "archive": "1.3.3.zip", to match the URL so that it actually got used during the bootstrap.

meteorcloudy · 2023-06-27T12:38:45Z

For "Clang on :ubuntu: 18.04 LTS (OpenJDK 11, gcc 7.5.0)", we do plan to phase out ubuntu 1804 soon. But we need to schedule the work and ideally we do that for Bazel 7 not in the middle of a LTS release since we promises backwards compatibility. So if there is other ways to workaround it, that will be preferred.

tylerwilliams · 2023-06-30T21:35:34Z

PTAL

@coeuvre -- I've made the changes you suggested and made the implementation a MessageDigest as well as reverting the changes to Fingerprint etc. Thank you for the detailed pointers there.

Re failing tests, I have merged bazelbuild/bazel-central-registry#698 to fix the bzlmod tests but now they are failing in a new way with:

(17:32:13) ERROR: Error computing the main repository mapping: Yanked version detected in your resolved dependency graph: rules_go@0.34.0, for the reason: Obsolete experimental version that emits debug prints. Update to 0.38.1 or higher..

Any ideas on how to fix this?

…b3hasher2

coeuvre · 2023-07-05T09:12:12Z

src/main/java/com/google/devtools/build/lib/vfs/Blake3Provider.java

+public final class Blake3Provider extends Provider {
+  public Blake3Provider() {
+    super("BLAKE3Provider", "1.0", "A BLAKE3 digest provider");
+    put("MessageDigest.BLAKE3", "com.google.devtools.build.lib.vfs.Blake3MessageDigest");


nit: Use Blake3MessageDigest.class.getName() to make refactor easier in the future.

coeuvre · 2023-07-05T09:17:39Z

src/main/java/com/google/devtools/build/lib/vfs/DigestHashFunction.java

@@ -67,6 +68,7 @@ public int getDigestMaximumLength() {

  public static final DigestHashFunction SHA1 = register(Hashing.sha1(), "SHA-1", "SHA1");
  public static final DigestHashFunction SHA256 = register(Hashing.sha256(), "SHA-256", "SHA256");
+  public static final DigestHashFunction BLAKE3 = register(new Blake3HashFunction(), "BLAKE3");


I think you can just use new MessageDigestHashFunction("BLAKE3") (doc) instead of implementing HashFunction by yourself in Blake3HashFunction.

As mentioned above, we don't want put BLAKE3 code in the common code path. Neither the registration here nor the provider below.

I suggest we have a utility class BazelHashFunctions which installs the provider and register the hash function, e.g.:

class BazelHashFunctions { static { addProvider(...); } public static final DigestHashFunction BLAKE3 = DigestHashFunction.register(...); }

In a following PR when we want to create a file system based on BLAKE3, we could do that in BazelFileSystemModule with code like:

new UnixFileSystem(BazelHashFunctions.BLAKE3, ...);

I'm trying to read between the lines here and figure out where you want stuff / how you want it to be injected -- let me know what I got wrong.

I moved blake3 stuff into a bazel subdir of vfs. The provider is statically registered when BazelHashFunctions is loaded by the classloader in src/main/java/com/google/devtools/build/lib/bazel/Bazel.java. Re the MessageDigestHashFunction -- addressed above, but that's not public afaict.

Yes, we want the provider to be statically registered by the classloader when it is loading class BazelHashFunctions. But we don't want to have it implementing BlazeModule and put it in src/main/java/com/google/devtools/build/lib/bazel/Bazel.java.

What I proposed above is we could add the code

new UnixFileSystem(BazelHashFunctions.BLAKE3, ...);

to somewhere like here so that when we want to use BLAKE3, it registers it.

We don't need to do it in this CL as we haven't focused on integrating BLAKE3 into Bazel's VFS. Let's limit the scope of this CL to only adapt BLAKE3 as a DigestHashFunction.

Can you give a little more context on what else is required to add the digest function to vfs? I'm a little unclear on that part and it will help me for the next cl. Thanks!

Well, I think I overcomplicated that. The only necessary part is we want to register BLAKE3 for Bazel only. We can just force loading BazelHashFunctions by adding following code block in BazelFilesystemModule

class BazelFilesystemmodule { static { BazelHashFunctions.ensureRegistered(); } }

where BazelHashFunctions.ensureRegistered is just an empty static function whose sole purpose is to let JVM load class BazelHashFunctions and run the static block there.

Thanks, this is just what I needed. Done.

coeuvre · 2023-07-05T09:21:28Z

src/main/java/com/google/devtools/build/lib/vfs/Blake3MessageDigest.java

+import java.security.DigestException;
+import java.security.MessageDigest;
+
+public final class Blake3MessageDigest extends MessageDigest implements Hasher {


If you use MessageDigestHashFunction as suggested below, you can remove implements Hasher.

MessageDigestHashFunction is not public, and copying it in is prohibitive because it brings like 5 other classes.

Sorry, I missed that. Can we then create a separate class to implement Hasher? I found it is confusing to combine these two especially when we have update, engineUpdate and share the underlying buffer.

src/main/java/com/google/devtools/build/lib/vfs/Blake3JNI.java

src/main/java/com/google/devtools/build/lib/vfs/Blake3MessageDigest.java

…b3hasher2

src/main/native/blake3_jni.cc

coeuvre · 2023-07-06T09:21:51Z

src/main/java/com/google/devtools/build/lib/vfs/bazel/Blake3MessageDigest.java

+  }
+
+  public void engineUpdate(byte[] data, int offset, int length) {
+    while (length > 0) {


One optimization here is we could avoid copying if the size of data is larger than ONESHOT_THRESHOLD. But this is totally optional and we could leave that for a separate PR.

Yeah I was trying to keep this simple for now -- enough complexity already :) Agree on deferring this for a followup (where we measure perf).

coeuvre · 2023-07-06T09:23:19Z

src/main/java/com/google/devtools/build/lib/vfs/bazel/Blake3MessageDigest.java

+  }
+
+  public void engineUpdate(byte b) {
+    engineUpdate(new byte[] {b});


Optional: avoid creating a new object, put the byte into buffer directly.

coeuvre · 2023-07-06T09:28:24Z

src/main/java/com/google/devtools/build/lib/vfs/bazel/Blake3MessageDigest.java

+
+  @CanIgnoreReturnValue
+  public Hasher putBytes(ByteBuffer b) {
+    buffer = b;


I think we need to copy/use the argument b now instead of saving it for use later. I believe the contract of the interface is the caller is free to use the passed buffer after the function is returned.

Right you are.

coeuvre

Thanks, this PR is pretty good now! Just a few more comments and we are ready to go!

coeuvre · 2023-07-10T09:04:25Z

src/main/native/BUILD

 cc_binary(
    name = "libunix_jni.so",
    srcs = [
+        "blake3_jni.cc",


Do we need this line since blake3_jni is already in the deps?

coeuvre · 2023-07-10T09:05:59Z

src/test/java/com/google/devtools/build/lib/vfs/bazel/BUILD

+java_test(
+    name = "BazelTests",
+    size = "small",
+    tags = ["no_windows"],


I see BALKE3 code can be compiled on Windows. Are there any blockers for running the tests on windows?

fmeum · 2023-07-18T06:36:14Z

third_party/blake3/blake3.BUILD

-            "c/blake3_avx512_x86-64_unix.S",
+            # Disable to appease bazel-ci which uses ubuntu-18 (EOL) and GCC 7
+            # lacking the headers to compile AVX512.
+            # "c/blake3_avx512_x86-64_unix.S",


@meteorcloudy FYI, this is an unfortunate consequence of the old versions running in CI.

Related: #18784 (comment)

fmeum · 2023-07-18T06:40:31Z

src/main/native/blake3_jni.cc

+  return env->ReleaseByteArrayElements(array, addr, 0);
+}
+
+extern "C" JNIEXPORT long JNICALL


This returns long while the corresponding Java method returns int. It should also the JNIEnv * and jobject parameters.

fmeum · 2023-07-18T06:43:18Z

src/main/java/com/google/devtools/build/lib/vfs/bazel/Blake3MessageDigest.java

+  }
+
+  @Override
+  protected void finalize() throws Throwable {


That usage is fine as it comes after the loadJni call. The failing test bootstraps Bazel and may not honor BUILD files, maybe the new JNI sources need to be listed somewhere else?

tylerwilliams · 2023-07-20T04:58:12Z

@coeuvre I believe this is ready for another look / attempt at merging. Thanks

coeuvre · 2023-07-20T09:44:38Z

Thanks, I am importing now!

…b3hasher2

coeuvre · 2023-07-21T13:23:28Z

Everything went well. Currently waiting for internal review.

tylerwilliams · 2023-07-21T14:08:37Z

Everything went well. Currently waiting for internal review.

@coeuvre sweet, thanks! Btw did you see my comment on the gerrit repo? https://bazel-review.googlesource.com/c/bazel/+/225070

Hoping we can make that last minute change before it goes in. Thanks again!

coeuvre · 2023-07-21T15:01:41Z

Sorry, I didn't see that because there wasn't any notification :(.

Np, I can make the change now.

brentleyjones · 2023-07-24T12:42:15Z

Thanks @coeuvre!

tylerwilliams · 2023-07-24T16:06:06Z

@coeuvre thank you again for all your help shepherding this through the process

Currently, it is included in runtime library with glob which is shared between bazel and blaze. Moving it to bazel package to prevent from packaging it when building blaze. An upcoming change (in bazelbuild#18784) will package BLAKE3 to this module and we don't want it in blaze. PiperOrigin-RevId: 547516042 Change-Id: I673fa3d6824d56c85e85b02d13b4eb56571edda9 (cherry picked from commit 6d6bbdc)

Currently, it is included in runtime library with glob which is shared between bazel and blaze. Moving it to bazel package to prevent from packaging it when building blaze. An upcoming change (in #18784) will package BLAKE3 to this module and we don't want it in blaze. PiperOrigin-RevId: 547516042 Change-Id: I673fa3d6824d56c85e85b02d13b4eb56571edda9 (cherry picked from commit 6d6bbdc)

Currently, it is included in runtime library with glob which is shared between bazel and blaze. Moving it to bazel package to prevent from packaging it when building blaze. An upcoming change (in bazelbuild#18784) will package BLAKE3 to this module and we don't want it in blaze. PiperOrigin-RevId: 547516042 Change-Id: I673fa3d6824d56c85e85b02d13b4eb56571edda9 (cherry picked from commit 6d6bbdc)

Currently, it is included in runtime library with glob which is shared between bazel and blaze. Moving it to bazel package to prevent from packaging it when building blaze. An upcoming change (in #18784) will package BLAKE3 to this module and we don't want it in blaze. PiperOrigin-RevId: 547516042 Change-Id: I673fa3d6824d56c85e85b02d13b4eb56571edda9 (cherry picked from commit 6d6bbdc)

Currently, it is included in runtime library with glob which is shared between bazel and blaze. Moving it to bazel package to prevent from packaging it when building blaze. An upcoming change (in #18784) will package BLAKE3 to this module and we don't want it in blaze. PiperOrigin-RevId: 547516042 Change-Id: I673fa3d6824d56c85e85b02d13b4eb56571edda9 (cherry picked from commit 6d6bbdc) Co-authored-by: Googler <chiwang@google.com>

iancha1992 · 2023-09-21T20:51:02Z

The changes in this PR have been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

Add BLAKE3 hasher to vfs

bfa90b2

tylerwilliams force-pushed the b3hasher2 branch from d355ccd to e1beb94 Compare June 27, 2023 00:32

tylerwilliams marked this pull request as ready for review June 27, 2023 00:37

github-actions bot added the awaiting-review PR is awaiting review from an assigned reviewer label Jun 27, 2023

sgowroji added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Jun 27, 2023

coeuvre self-requested a review June 27, 2023 08:51

coeuvre reviewed Jun 27, 2023

View reviewed changes

tylerwilliams force-pushed the b3hasher2 branch 3 times, most recently from a904ba6 to 8e26aa5 Compare June 30, 2023 19:55

tylerwilliams force-pushed the b3hasher2 branch from 8e26aa5 to c9c3e42 Compare July 1, 2023 04:43

Merge branch 'master' of https://github.com/tylerwilliams/bazel into …

d6cc1a5

…b3hasher2

tylerwilliams force-pushed the b3hasher2 branch from c9c3e42 to d6cc1a5 Compare July 1, 2023 17:30

coeuvre requested changes Jul 5, 2023

View reviewed changes

tylerwilliams force-pushed the b3hasher2 branch from 3433d0f to f285a96 Compare July 6, 2023 04:52

Merge branch 'master' of https://github.com/tylerwilliams/bazel into …

f32259d

…b3hasher2

tylerwilliams force-pushed the b3hasher2 branch from f285a96 to f32259d Compare July 6, 2023 05:10

tylerwilliams commented Jul 6, 2023

View reviewed changes

src/main/native/blake3_jni.cc Outdated Show resolved Hide resolved

tylerwilliams force-pushed the b3hasher2 branch 2 times, most recently from 07fbd47 to c92ed72 Compare July 6, 2023 08:26

coeuvre reviewed Jul 6, 2023

View reviewed changes

tylerwilliams force-pushed the b3hasher2 branch 2 times, most recently from 339d81d to 3d6b67e Compare July 8, 2023 05:30

coeuvre reviewed Jul 10, 2023

View reviewed changes

tylerwilliams force-pushed the b3hasher2 branch from 3d6b67e to bfdd92c Compare July 10, 2023 17:09

tylerwilliams force-pushed the b3hasher2 branch from 71ca701 to 3a18d06 Compare July 18, 2023 05:56

fmeum reviewed Jul 18, 2023

View reviewed changes

tylerwilliams force-pushed the b3hasher2 branch 7 times, most recently from 81ded64 to 890327f Compare July 19, 2023 05:14

Merge branch 'master' of https://github.com/tylerwilliams/bazel into …

e848e90

…b3hasher2

tylerwilliams force-pushed the b3hasher2 branch from 890327f to e848e90 Compare July 20, 2023 18:56

copybara-service bot closed this in cc49d68 Jul 24, 2023

github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jul 24, 2023

brentleyjones mentioned this pull request Jul 25, 2023

[6.4.0] Move BazelFileSystemModule into bazel package #19043

Merged

brentleyjones mentioned this pull request Aug 7, 2023

[6.4.0] Add support for the BLAKE3 digest function #19191

Merged

Add BLAKE3 hasher to vfs #18784

Add BLAKE3 hasher to vfs #18784

Conversation

tylerwilliams commented Jun 26, 2023

tylerwilliams commented Jun 27, 2023 • edited

coeuvre commented Jun 27, 2023

Choose a reason for hiding this comment

meteorcloudy commented Jun 27, 2023

meteorcloudy commented Jun 27, 2023

tylerwilliams commented Jun 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerwilliams Jul 6, 2023 • edited

Choose a reason for hiding this comment

coeuvre Jul 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coeuvre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerwilliams commented Jul 20, 2023

coeuvre commented Jul 20, 2023

coeuvre commented Jul 21, 2023

tylerwilliams commented Jul 21, 2023

coeuvre commented Jul 21, 2023

brentleyjones commented Jul 24, 2023

tylerwilliams commented Jul 24, 2023

iancha1992 commented Sep 21, 2023

tylerwilliams commented Jun 27, 2023 •

edited

tylerwilliams commented Jun 30, 2023 •

edited

tylerwilliams Jul 6, 2023 •

edited

coeuvre Jul 6, 2023 •

edited