Fetch `RepoSpec`s in parallel #19294

fmeum · 2023-08-22T07:00:13Z

Computing RepoSpecs for all selected Bazel modules is on the critical path for the computation of the main repository mapping and thus benefits from parallelized downloads.

On my machine, this change has the following effect on bazel build //src:bazel-dev --enable_bzlmod --nobuild:

before
compute main repo mapping: 8s 127ms

after
compute main repo mapping: 4s 226ms

fmeum · 2023-08-22T07:03:51Z

I briefly looked into whether the repo spec computation could be moved out of the compute main repo mapping's path, but couldn't figure it out.

fmeum · 2023-08-22T08:25:02Z

The flaky test may be related to this PR.

Wyverald · 2023-08-22T18:36:40Z

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java

-          entry.getKey(),
-          moduleFromInterimModule(
-              entry.getValue(), overrides.get(entry.getKey().getName()), eventHandler));
+    ExecutorService executorService = Executors.newWorkStealingPool(8);


First of all I'll admit I'm very unfamiliar with the Java parallel stream stuff, so apologies if I make any rookie mistakes here :)

From my read of things, it looks like we don't even need to have this executor service at all? We could just directly call collect on a parallel stream and Java would use the global fork join pool (ForkJoinPool.commonPool()) underneath. That's probably better than using a dedicated pool (as the global fork join pool is also used to carry virtual threads).

But I'm not sure what happens when an exception is thrown in that case. In sequential streams, I think the exception just gets propagated out, so we just need to "tunnel" it. Do you know what happens in parallel streams?

The problem with using the common pool is that it has NUM_CPUS - 1 threads by default, which can interact badly with other parallel operations relying on it, especially since we are blocking on IO.

Exceptions in parallel streams are also thrown by the terminal operation. I think that the first throw "wins", but I'm not sure about that.

Thanks for the explanation. I did some more reading on parallel streams and I increasingly feel like they're not a great fit for our use case (maybe I just don't understand them well enough). My main gripe is the fact that it tries to do all the MapReduce-esque splitting and merging which is basically completely unnecessary here where we're just doing a map and basically no reduce. Plus the concurrency primitives are completely hidden and it works "magically" depending on whether you're already in a ForkJoinPool or not; and this doesn't seem to be documented anywhere in the official JDK -- I had to believe StackOverflow. (For example, I'm not confident at all whether the introduction of Loom would automatically cause parallel stream processing to use virtual threads.)

On the other hand, I'll admit that the magic isn't all bad -- just slapping .parallel() on the stream and having it work is pretty neat.

I'd probably have preferred to use Skyframe for this; that is, create a RepoSpecFunction/Value and just give everything to Skyframe. To avoid the restart, we could use a SkyKeyComputeState to store everything right before the repo spec computation step. WDYT?

Yeah, the lack of official documentation on parallel streams' internals really hurts their usefulness. I can only hope that this explained by the desire to switch them over to a virtual thread executor by default once available - one can dream.

I don't quite see the problem with us not using the full feature set of a MapReduce. Isn't turning a list of values into a LinkedHashMap still an (associative) reduction? But we do need to believe StackOverflow that streams are just relying on ForkJoinPool#fork and thus just depens on the ForkJoinPool implementation that we started with (#commonPool or a custom one).

I am fine with switching to Skyframe here though, especially as it avoids anyone reading the code from having to reason about another concurrency framework.

I think I found a way to extract this work into a SkyFunction, please take a look. I am a bit unsure about how to implement Registry's equality correctly, which made me worried about using it in a SkyKey.

After spending like 10 seconds looking at this change, I don't think Registry is appropriate to be included in a SkyKey: SkyKeys are supposed to be small and serializable value objects, and IndexRegistry keeps references to a DownloadManager and a Gson instance, which means that they are not value objects at all.

From a quick glance, it looks like each Registry instance comes from a URI in ModuleFileFunction.REGISTRIES so maybe the URI could be used in the SkyKey instead?

I trimmed this down to a ModuleKey and a String.

Yep, that sounds much nicer. I'll leave the review to @Wyverald since he is much more familiar with the area (unless he indicates that he could use my brain)

I don't quite see the problem with us not using the full feature set of a MapReduce. Isn't turning a list of values into a LinkedHashMap still an (associative) reduction?

That was poorly worded on my part -- what I wanted to say was that the extra considerations about splitting the stream into appropriately sized substreams so that it could be sent to child ForkJoinTasks and then merging the results together are kind of wasteful. Yes technically a "map" operation is a "reduce" operation itself too, but we really don't need a tree structure for this. Just linearly going over the keys and sending each one off for a download will suffice.

So I'm very happy with the new Skyframe-based implementation :)

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java

fmeum · 2023-08-23T10:21:12Z

@Wyverald I stacked this onto the profiling PR for the moment.

fmeum · 2023-08-23T11:12:41Z

@lberki Do you happen to know how the value of HttpDownloader#MAX_PARALLEL_DOWNLOADS was determined? Increasing it to 16 saves another 0.5s (out of 4.5s) for me.

lberki · 2023-08-23T11:23:38Z

Nope, sorry. I checked the change history but there doesn't seem to be any indication as to how the author of the change that added the parallelism arrived at that number. Naively, 8 seems to be both too small and too unconfigurable, but I'm saying this with literally zero research, so that it with a grain of salt.

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RepoSpecFunction.java

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java

Wyverald · 2023-08-23T17:22:08Z

Also please rebase; the profiling PR is merged :)

fmeum · 2023-08-23T19:40:15Z

src/main/java/com/google/devtools/build/lib/skyframe/SkyFunctions.java

@@ -158,6 +158,7 @@ public final class SkyFunctions {
      SkyFunctionName.createHermetic("BAZEL_DEP_GRAPH");
  public static final SkyFunctionName BAZEL_LOCK_FILE =
      SkyFunctionName.createHermetic("BAZEL_LOCK_FILE");
+  public static final SkyFunctionName REPO_SPEC = SkyFunctionName.createNonHermetic("REPO_SPEC");


Just an observation: I noticed that BAZEL_MODULE_RESOLUTION was marked hermetic even though it probably wasn't before this PR (due to the registry fetches).

ah, that does look like an oversight. However in practice this shouldn't matter much since we assume existing registry contents are immutable anyway.

Computing `RepoSpec`s for all selected Bazel modules is on the critical path for the computation of the main repository mapping and thus benefits from parallelized downloads. On my machine, this change has the following effect on `bazel build //src:bazel-dev --enable_bzlmod --nobuild`: ``` before compute main repo mapping: 8s 127ms after compute main repo mapping: 4s 226ms ```

Wyverald

nice!

fmeum · 2023-08-23T20:28:13Z

Thanks for pushing me towards the Skyframe solution. A side effect of that is that trivial changes to the root module file now no longer result in any network I/O (assuming the Skyframe cache is retained during invocations).

fmeum · 2023-08-24T18:46:17Z

@bazel-io flag

Wyverald · 2023-08-24T20:53:49Z

@bazel-io fork 6.4.0

Computing `RepoSpec`s for all selected Bazel modules is on the critical path for the computation of the main repository mapping and thus benefits from parallelized downloads. On my machine, this change has the following effect on `bazel build //src:bazel-dev --enable_bzlmod --nobuild`: ``` before compute main repo mapping: 8s 127ms after compute main repo mapping: 4s 226ms ``` Closes bazelbuild#19294. PiperOrigin-RevId: 559819452 Change-Id: Ieef957fcfe402c909d2863ba4a4ca3540781a56d

Computing `RepoSpec`s for all selected Bazel modules is on the critical path for the computation of the main repository mapping and thus benefits from parallelized downloads. On my machine, this change has the following effect on `bazel build //src:bazel-dev --enable_bzlmod --nobuild`: ``` before compute main repo mapping: 8s 127ms after compute main repo mapping: 4s 226ms ``` Closes #19294. Commit 8a68310 PiperOrigin-RevId: 559819452 Change-Id: Ieef957fcfe402c909d2863ba4a4ca3540781a56d

iancha1992 · 2023-09-21T20:19:16Z

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!

fmeum requested review from Wyverald and meteorcloudy as code owners August 22, 2023 07:00

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Aug 22, 2023

Wyverald reviewed Aug 22, 2023

View reviewed changes

fmeum force-pushed the bzlmod-futures branch 2 times, most recently from d1dfd64 to df70c95 Compare August 23, 2023 10:18

fmeum force-pushed the bzlmod-futures branch from df70c95 to 2460e21 Compare August 23, 2023 11:37

fmeum requested a review from Wyverald August 23, 2023 11:54

fmeum requested a review from a team as a code owner August 23, 2023 12:15

fmeum requested review from aranguyen and removed request for a team August 23, 2023 12:15

Wyverald reviewed Aug 23, 2023

View reviewed changes

fmeum force-pushed the bzlmod-futures branch from 62c3c71 to bf9be0b Compare August 23, 2023 19:38

fmeum commented Aug 23, 2023

View reviewed changes

fmeum requested a review from Wyverald August 23, 2023 19:40

fmeum force-pushed the bzlmod-futures branch from bf9be0b to ccf8a5e Compare August 23, 2023 19:41

Wyverald approved these changes Aug 23, 2023

View reviewed changes

Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Aug 23, 2023

copybara-service bot closed this in 8a68310 Aug 24, 2023

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Aug 24, 2023

fmeum deleted the bzlmod-futures branch August 24, 2023 18:46

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 24, 2023

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 24, 2023

bazel-io mentioned this pull request Aug 24, 2023

[6.4.0] Fetch RepoSpecs in parallel #19328

Closed

bazel-io mentioned this pull request Aug 24, 2023

[Duplicate] Fetch RepoSpecs in parallel #19329

Closed

fmeum mentioned this pull request Aug 28, 2023

[6.4.0] Fetch RepoSpecs in parallel #19354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch `RepoSpec`s in parallel #19294

Fetch `RepoSpec`s in parallel #19294

fmeum commented Aug 22, 2023 •

edited

Loading

fmeum commented Aug 22, 2023

fmeum commented Aug 22, 2023

Wyverald Aug 22, 2023

fmeum Aug 22, 2023

Wyverald Aug 22, 2023

Wyverald Aug 22, 2023

fmeum Aug 23, 2023

fmeum Aug 23, 2023

lberki Aug 23, 2023 •

edited

Loading

fmeum Aug 23, 2023

lberki Aug 23, 2023

Wyverald Aug 23, 2023

fmeum commented Aug 23, 2023

fmeum commented Aug 23, 2023

lberki commented Aug 23, 2023

Wyverald commented Aug 23, 2023

fmeum Aug 23, 2023

Wyverald Aug 23, 2023

Wyverald left a comment

fmeum commented Aug 23, 2023

fmeum commented Aug 24, 2023

Wyverald commented Aug 24, 2023

iancha1992 commented Sep 21, 2023

Fetch RepoSpecs in parallel #19294

Fetch RepoSpecs in parallel #19294

Conversation

fmeum commented Aug 22, 2023 • edited Loading

fmeum commented Aug 22, 2023

fmeum commented Aug 22, 2023

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

lberki Aug 23, 2023 • edited Loading

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

fmeum commented Aug 23, 2023

fmeum commented Aug 23, 2023

lberki commented Aug 23, 2023

Wyverald commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wyverald left a comment

Choose a reason for hiding this comment

fmeum commented Aug 23, 2023

fmeum commented Aug 24, 2023

Wyverald commented Aug 24, 2023

iancha1992 commented Sep 21, 2023

Fetch `RepoSpec`s in parallel #19294

Fetch `RepoSpec`s in parallel #19294

fmeum commented Aug 22, 2023 •

edited

Loading

lberki Aug 23, 2023 •

edited

Loading