Enables compiling against direct dependencies only

[nkoroste]

Defines --experimental_prune_transitive_deps that will allow you to make java and android to only use direct dependancies when compiling a given target. This significantly speeds up incremental compilations at the cost of adding explicit deps to all targets.

relates to bazelbuild/rules_kotlin#842

[keithkml]

Sorry for the naive questions, but:

• How does this differ from Bazel's built-in "reduced classpath mode"? (I can't really figure out what that is, and have not tried it)
• How does this relate to ijars and Turbine? Do we really need to reduce classpath if ijars work well?

For context, at Stripe we are starting to think about compile times and the dependency graph. We're starting with finding unused deps and compiling Turbine natively with GraalVM.

[oliviernotteghem]

@keithkml : Bazel's built-in reduced classpath mode may also suppress transitive dependencies, however it's done at action execution time. This means action input might still be very large (and you might hit 'too many files open' on mac, for instance). Also, Bazel's built-in implementation uses a fallback mechanism : so when compilation fails against reduced classpath, it attempts to rebuild against the full classpath (which might not be optimal for efficiency if this keep happening).

On a positive side, Bazel's built-in implementation may be better for developer experience (since eng don't have to follow strict deps) and better handles jvm_rules_external deps (again, devs don't have to list every single transitive dep in their BUILD file).

I think the consensus is that wether one uses such a PR or its own tooling to statically prune unused deps, it's fine. Just do NOT build using all transitives deps, this would create a large surface for invalidation (even when using ABI) and build time would be extremely high.

[nkoroste]

Hey, @larsrc-google and @meisterT, it was nice chatting with you guys during BazelCon. Do you mind reviewing this PR as we agreed that it would be useful to merge. Thank you!

[larsrc-google]

What's your experience with the number of explicit deps you need to add when using this? Also, if a library adds a new dependency to its API, who's responsible for updating all its users?

[keithkml]

Thanks for answering my question about reduced classpath mode. I see that this is similar but acts at the action graph level.

I am still wondering about the value of this PR when we already have ijars. Interface jars / Turbine achieve the same goal (faster incremental builds) but with a different tradeoff between ergonomics and speed.

Is it worth adding yet another way to alter the action graph for Java? Do we have any real-world numbers about cache invalidations with ijars vs. this PR?

[meisterT]

cc @cushon

[nkoroste]

@larsrc-google:

What's your experience with the number of explicit deps you need to add when using this?

I don't have the numbers as we've been using it for years now. We did a one time cleanup of the codebase and it was enabled since.

Also, if a library adds a new dependency to its API, who's responsible for updating all its users?

All users don't need to be updated. If the new dep is part of current libs public API it should be exposed via exports build attribute by the library author - that way all parents/users will just get it.

@keithkml:

I am still wondering about the value of this PR when we already have ijars. Interface jars / Turbine achieve the same goal (faster incremental builds) but with a different tradeoff between ergonomics and speed.
Is it worth adding yet another way to alter the action graph for Java? Do we have any real-world numbers about cache invalidations with ijars vs. this PR?

For us at Snapchat (and I know for other companies) this was a huge boost to incremental build times beyond what ijar/hjar/turbine currently provides. You can see some real world numbers from Uber in this PR #16457 :

--	Build time (secs)	# rebuilt targets
bazel	498.7003218	1378
bazel w/ transitive deps pruning (*)	195.7602742	228

^ the first row is with turbine

[larsrc-google]

Those numbers look quite appealing. I particularly like that this is done as part of the analysis phase, so it actually reduces the graph and prevents actions from being run unnecessarily.

All users don't need to be updated. If the new dep is part of current libs public API it should be exposed via exports build attribute by the library author - that way all parents/users will just get it.

I'm still concerned about this part. This means that any java_library target must put its entire public API in the exports section - classes, methods, fields, etc (or do we only need classes? That would not be too bad). That alone sounds like it would add a lot of noise to the build files. The exports list would then need to be updated whenever there is a change, or we risk silently getting a wrong compile - though it should be reasonably easy to add a presubmit check that the list is correct. Do you have a tool that updates build files to have the correct exports?

Another issue is that this changes the Java code in Bazel, which is being starlarkified.

[nkoroste]

I'm still concerned about this part. This means that any java_library target must put its entire public API in the exports section - classes, methods, fields, etc (or do we only need classes? That would not be too bad). That alone sounds like it would add a lot of noise to the build files. The exports list would then need to be updated whenever there is a change, or we risk silently getting a wrong compile - though it should be reasonably easy to add a presubmit check that the list is correct.

That's not exactly how it works - I'm talking about the java_library (or android_library, kt_library, etc.) exports attribute that is defined here https://bazel.build/reference/be/java#java_library.exports. It works on the target level and will only be needed for deps targets that happened to be part of the public API of a given target. For the given target, all public API in srcs is already available to all parents so no changes are required. For a given target, the only change that would be required is to promote a library from deps to exports attribute IFF that library is part of the public API in any of the classes under srcs attribute. This is expected behavior in other build system as well by the way.

Do you have a tool that updates build files to have the correct exports?

Not really but can be done via jdeps + buildozer. This PR is behind a flag, people can turn it on and do a mass refactor by simply compiling with --keep_going and fixing all issues in one batch. From then on, any new code will fail to compile if public API is not exposed properly to parent target at development time.

Another issue is that this changes the Java code in Bazel, which is being starlarkified.

Yes, this should make it into Starlark rules.

[Bencodes]

We also use a version of this internally that compiles against direct dependencies, but only for the current workspace (excludes included Bazel repositories as we don't want to break the tools that they use by stripping their transitive classpath deps). And we've also seen similar build improvements and less mass-action invalidation. Happy to dig up our benchmarks for those that are interested.

I also know of a few other large Java/Android companies that do this as well, and have also reported pretty significant build time improvements.

[oliviernotteghem]

@keithkml : using turbine/ijars is quite orthogonal (although the goal is the same : reduce the surface for invalidation). The key here, as @larsrc-google says, is that it reduces the graph. Keep in mind that ANY change to any of an action input WILL re-trigger this action. So, by reducing the graph / minimizing the list of input, you decrease the chance that an action will re-trigger (in our case, a java library recompiles) during an incremental build.

This explains also why the change here (or change like #16457) gives superior results to Bazel built-in java classpath reducer.

Let me give an example :
Let's say libA -> (depend on) LibB -> libC --> ... --> libZ, but libA does not really need libZ to compile.

With Bazel classpath reducer : even with turbine/ijar, an ABI change to libZ WILL recompile libA, libB, libC. Why? Because libZ ABI jar is an input to libA, because, by default, transitive dependencies are included as input to java lib targets.

With the change here, libZ is removed from libA input. Any change to libZ ABI jar will not cause a recompilation of libA.

I hope this make sense, and shed some light on why the build time improvements are quite large.

@larsrc-google : do you have a suggestion about how to replicate this in the upcoming jvm starlark? What is the status of the starlakification of jvm rules?

[cushon]

Let's say libA -> (depend on) LibB -> libC --> ... --> libZ, but libA does not really need libZ to compile.

With Bazel classpath reducer : even with turbine/ijar, an ABI change to libZ WILL recompile libA, libB, libC. Why? Because libZ ABI jar is an input to libA, because, by default, transitive dependencies are included as input to java lib targets.

That isn't how it's supposed to work. Originally the reduced classpath reduction happened during action execution, so changes in inputs would cause invalidation even if those inputs were pruned off the reduced classpath. But the classpath reduction now happens in Bazel, so in the example if libZ changes the action will still be a cache hit and won't be reexecuted:

https://cs.github.com/bazelbuild/bazel/blob/60ded2b53d4d9180c213e9e44a8c640368e60822/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java#L244

When that Bazel-side reduced classpath reduction work happened, we looked at alternatives, including trying to compute a sufficient classpath at analysis time. That was similar to the approach of only compiling against direct deps and relying on increased use of exports, except we were considering introducing a separate api_deps attribute that could have had different semantics and tooling support. I still think that's an interesting approach, but we weren't able to justify the added complexity over the input pruning approach that got implemented.

One of the disadvantages of exports is that it lets clients of libraries using exports depend on the exported targets directly in their implementation, which undermines strict deps enforcement, and means that if the libraries change they have to go and add explicit deps to all of libraries depending on them that were previously getting those deps for free.

[oliviernotteghem]

@cushon : I am afraid the code you're pointing to is not used by anyone at the moment. To start with, there is not code is Bazel repo that registers the JavaCompileActionContext, so you'll get a NPE as soon as you enable it (i.e forcing classpathMode=Bazel). I can take another look at this, and see if we can leverage this do our pruning. I remember I got it to work (something else was needed, beside register enable the flags / fixing the NPE), but results were still sub-optimal, I think. I will need to get back to you on this.

[larsrc-google]

@nkoroste Let me see if I understand this right. Say that I have these files:

Server.java:
import org.example.configuration.Config;
import org.example.FileUtils;
import org.protos.Request;
import org.protos.Response;
public class Server {
  private Config config;
  public void readConfig(FileUtils configFile) { config = configFile.read(); }
  public Response call(Request request) { ... }
}

RpcServer.java:
public class RpcServer extends Server {
  public int getPingTime() {...}
}

Game.java:
import org.gamers.UI;
public class Game {
  private RpcServer rpcServer;
  void showPingTime() { UI.display(rpcServer.getPingTime()); }
}

BUILD:
java_library(
  name = "server",
  srcs = ["Server.java"]
  deps = ["//org/example:utils", "//org/protos:all", "//org/example/config:config"]
)

java_library(
  name = "rpcserver",
  srcs = ["RpcServer.java"]
  deps = [":server"]
)

java_library(
  name = "game",
  srcs = ["Game.java"]
  deps = [":rpcserver"]
)

Am I right then that with --experimental_prune_transitive_deps, Server.java would need to export ["//org/example:utils", "//org/protos:all"] because they're visible in the method signatures, and RpcServer.java would need to export [":server"] because it's visible as a superclass? But RpcServer.java would not need to re-export ["//org/example:utils", "//org/protos:all"] because it exports those through the transitive closure of [":server"]? And "//org/example/config:config" would not be included in the transitive closure at all, being only used privately?

[cushon]

@cushon : I am afraid the code you're pointing to is not used by anyone at the moment. To start with, there is not code is Bazel repo that registers the JavaCompileActionContext, so you'll get a NPE as soon as you enable it (i.e forcing classpathMode=Bazel). I can take another look at this, and see if we can leverage this do our pruning. I remember I got it to work (something else was needed, beside register enable the flags / fixing the NPE), but results were still sub-optimal, I think. I will need to get back to you on this.

Thanks, that's not great. I spun off #16906 for that.

I'd be interested to hear what your results were.

My take is that we should fix that existing optimization and evaluate how much that helps Bazel first.

When that work happened for Blaze we considered options like api_deps and the return on complexity for the additional performance didn't seem to be there. Doing direct classpaths without that seems worse, because of the additional direct deps and increased use of exports.

[nkoroste]

@nkoroste Let me see if I understand this right. Say that I have these files:

Server.java:
import org.example.configuration.Config;
import org.example.FileUtils;
import org.protos.Request;
import org.protos.Response;
public class Server {
  private Config config;
  public void readConfig(FileUtils configFile) { config = configFile.read(); }
  public Response call(Request request) { ... }
}

RpcServer.java:
public class RpcServer extends Server {
  public int getPingTime() {...}
}

Game.java:
import org.gamers.UI;
public class Game {
  private RpcServer rpcServer;
  void showPingTime() { UI.display(rpcServer.getPingTime()); }
}

BUILD:
java_library(
  name = "server",
  srcs = ["Server.java"]
  deps = ["//org/example:utils", "//org/protos:all", "//org/example/config:config"]
)

java_library(
  name = "rpcserver",
  srcs = ["RpcServer.java"]
  deps = [":server"]
)

java_library(
  name = "game",
  srcs = ["Game.java"]
  deps = [":rpcserver"]
)

Am I right then that with --experimental_prune_transitive_deps, Server.java would need to export ["//org/example:utils", "//org/protos:all"] because they're visible in the method signatures, and RpcServer.java would need to export [":server"] because it's visible as a superclass? But RpcServer.java would not need to re-export ["//org/example:utils", "//org/protos:all"] because it exports those through the transitive closure of [":server"]? And "//org/example/config:config" would not be included in the transitive closure at all, being only used privately?

Yes that is correct!

By the way I have a working example with all java based rule types that you can play around with here bazelbuild/rules_kotlin#840

p.s. here is a sister PR for rules_kotlin bazelbuild/rules_kotlin#842

[larsrc-google]

I like that! It can be created automatically, it can be checked, it can reduce recompilations, it can reduce input sizes.

We would need to have the equivalent for the Starlark rules, they are ready to be used.

One problem with using the exports attribute is that it's already used for other purposes, so you can't tell if it's meant to uphold this contract. The earlier suggestion of api_deps would make it clearer, but would still not be enough to indicate whether a rule has declared its deps - an empty list is indistinguishable from nothing, as I understand it. For migration purposes, we would need an explicit "this rule exports its API deps correctly" bit.

[guw]

Similar discussion in Slack here:
https://bazelbuild.slack.com/archives/CDCE4DFAP/p1670867793420109

The TL;DR version from the thread: we are seeing unnecessary recompilations of a java_library due to transitive ijar changes. We do know the code compiles fine without the transitives (because we use the Eclipse Java Compiler and have it ignore transitive dependencies passed in from Bazel making any compile time transitive required to be added to deps).

Now we need Bazel to stop making transitive ijars action inputs.

Would that PR address this?

[cushon]

Would that PR address this?

#16921 should also help

[nkoroste]

@guw

Would that PR address this?

Yes this PR will.

I haven't tried yet --experimental_java_classpath=bazel fix that is recommended by @cushon in #16426 (comment)

[arunkumar9t2]

We are also interested in --experimental_java_classpath=bazel since even after ensuring all targets list their deps correctly an ABI change was causing all upstream target to invalidate and we were looking to reduce inputs during analysis phase for full compile avoidance as said by @oliviernotteghem.

Currently we remove transitive classpath manually in both Bazel/rules_kotlin to improve compile avoidance, I could get some numbers for comparison if needed.

Now that #16921 is done, few question remains especially for non Java JVM based projects like (Android + Kotlin) to take advantage of classpath reduction. I hope folks don't mind me asking here but there is good context in this thread.

1. Starlarkification of rules_java: how would input pruning i.e java_classpath=bazel work for starlark based rules since starlark code can't read files during analysis?
2. Can a java_common API be added to do classpath reduction in Bazel process? This way rules_kotlin could potentially call java_common.reduced_classpath(transitive_classpath, jdeps) to get a reduced classpath before registering Kotlin/Java compile actions. IIUC we would need to expose this method via java_common. Is this something that aligns with Bazel design? I see few impls like JavaCommon.mergeJavaProviders doing calculations inline instead of using Actions. If this is being phased away what would be the right way to do pruning going forward?
3. Given the interest for this and significant wins being reported, would an official way for rules to modify action graph based on previous compile state be considered for Bazel? Currently the only option seems to be stateful workers. JVM build time improvements via tracking unused deps or classes [Java part] #16457 could be adapted to use an official mechanism. Buck2 for example calls this dynamic dependencies. If I am not wrong, C++ include scanning does something similar currently in Bazel.

[cushon]

Starlarkification of rules_java: how would input pruning i.e java_classpath=bazel work for starlark based rules since starlark code can't read files during analysis?

One quick note on this - the plan for the Java starlark rules, at least for now, is to use java_common.compile, which would still be implemented in Java code. The classpath reduction happens in the implementation of java_common.compile. Making it possible for a pure starlark rule to do something similar is an open question at this point, I think.

[larsrc-google]

The JavaImport file disappeared due to Starlarkification :(

[rsalvador]

@cushon We tried using --experimental_java_classpath=bazel after the #16921 fix.
The full and incremental builds are faster, but the improved speed seems to come only from the reduced classpath in the compile action, there is no reduction in the number of java compiler actions executed. Java compiler actions still get executed whenever any transitive ijar changes.
The classpath reduction seems to still happen during action execution?:

bazel/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java

Line 404 in 073f54b

if (classpathMode == JavaClasspathMode.BAZEL) {

[cushon]

The classpath reduction seems to still happen during action execution?

I think that's right, the builds I'm thinking of are using remote execution, so there's another level of caching that avoids re-execution those actions when the reduced classpath hasn't changed.

[fmeum]

I think that's right, the builds I'm thinking of are using remote execution, so there's another level of caching that avoids re-execution those actions when the reduced classpath hasn't changed.

Is this logic wired up in Bazel to the point where it would just work with --disk_cache=/some/path --experimental_java_classpath=bazel?

[rsalvador]

Is this logic wired up in Bazel to the point where it would just work with --disk_cache=/some/path --experimental_java_classpath=bazel?

Just tried, it does get cached with --disk_cache=... when the reduced classpath hasn't changed

[larsrc-google]

@nkoroste This looks good, but I would like to see a test. Also there are merge conflicts.

[cushon]

I still have concerns that this is going to make builds more fragile and harder to maintain. The classes that are needed at compile-time depend on implementation details of the compiler, and of the libraries that are being used. When those implementation details change, it will require churning deps lists. I don't think this is going to be a good trade-off for very large repos.

That doesn't necessarily mean the feature shouldn't be available as an option, or that it wouldn't be beneficial to some repos. But there are trade-offs and it's probably not something that will ever be enabled by default.

I am curious how close --disk_cache=/some/path --experimental_java_classpath=bazel can get to the performance of this feature, now that it works properly for Bazel. I think evaluating that would be a good next step before deciding to add this.

And if there is still a big performance gap between --experimental_java_classpath=bazel and this feature, I think the api_deps approach might be able to unlock most of the same benefit with fewer downsides for very large repos.

[larsrc-google]

Another downside to using exports for this is that it makes life harder for automation. exports is meant to be used for migration, and leads to the same target being "defined" in multiple places. Having that all over the build graph would make this much worse.

[larsrc-google]

There's also the smaller issue that compiler errors change: Trying to access a private member of a class might give you a class-not-found error instead of a no-access error.

[cushon]

compiler errors change: Trying to access a private member of a class might give you a class-not-found error instead of a no-access error.

I think we've already crossed that bridge: ijar and turbine remove private members from interface jars, so trying to access a private member across compilation boundaries results in a error: cannot find symbol. Or would api_deps cause more or different problems like that?

[rsalvador]

Is there some doc that explains what the api_deps option is?

[cushon]

Is there some doc that explains what the api_deps option is?

There's some discussion of the idea in this PR, there isn't a design doc.

[raviagarwal7]

With strict deps enabled (--experimental_strict_java_deps) the compilation classpath will still contain the full transitive jars but will fail the build if a jar is used in compilation but not defined as a direct dependency. It also shows a message on how it can be fixed (i.e add the dep as a direct dependency).

With this change do we now not put the full transitive jars in the compilation classpath? Would it make sense to add this feature to be one of the values for strict java deps something like --experimental_strict_java_deps=prune?
This way we don't add another flag but reuse the existing one since prune will not intersect with any other flag for experimental_strict_java_deps.

[guw]

@nkoroste We had an interesting discovery recently. We use a different Java compiler with a flag to only use direct dependencies for compilation. Essentially it's implemented in the compiler but not in Bazel.

Here is the thing:
It had an implementation gap, which left the jdeps file empty. As a result, we got pretty good build avoidance for Java implementation changes (not modifying the ijars) in combination with --disk_cache.

Once we implemented support for jdeps we noticed a lot more unnecessary rebuilds in Bazel.

Thus, I am thinking with the combination of --experimental_java_classpath=bazel and --strict_java_deps=error we probably don't need an explicit prune but can achieve a similar build avoidance by reducing the cache key?

On a side note, the performance gain from invoking the compiler only with direct dependencies is significant.

• rebuild time with empty jdeps files and dependency pruning: 171s
• rebuild time with proper jdeps and --experimental_java_classpath=bazel: 609s
• rebuild time with proper jdeps and --experimental_java_classpath=bazel and dependency pruning: 432s

However, I haven't figure out yet why the empty jdeps file would allow such high build avoidance. JavaCompileAction key contains all the transitives all the time.

[guw]

@cushon Few more thoughts. Appreciate your feedback here.

I still have concerns that this is going to make builds more fragile and harder to maintain. The classes that are needed at compile-time depend on implementation details of the compiler, and of the libraries that are being used. When those implementation details change, it will require churning deps lists. I don't think this is going to be a good trade-off for very large repos.

All compilation should happen with ijars (or header jars). When those jars do not change as input, there is a guarantee that the API and ABI did not change. If there is a signature change then this problem will become obvious as part of making the change. Thus, I am not sure I agree that this change will make builds more fragile. I do agree that it increases maintenance effort for developers because there is no more Bazel strict deps error messages but hard compile errors. Those are harder to understand/read and translate into actions (modify deps attribute).

I am curious how close --disk_cache=/some/path --experimental_java_classpath=bazel can get to the performance of this feature, now that it works properly for Bazel. I think evaluating that would be a good next step before deciding to add this.

See timings in my comment from above. The scenario is an implementation change (not signature) which triggers modification of the jdeps file, i.e. the list of actual used compile jars by the compiler changes. The change to the jdeps file causes significant rebuild activity even with --disk_cache=/some/path --experimental_java_classpath=bazel. It's much better when the jdeps file is empty. That triggers something in Bazel which avoids the rebuilds.

[fmeum]

@guw What happens if you combine --experimental_java_classpath=bazel with --strict_java_deps=off? As far as I understand the action definitions, this should keep the transitive .jdeps files out of the inputs.

[rsalvador]

The good timings we got when we tried --experimental_java_classpath=bazel, and that were close to the timings provided by this PR, were due to the empty .jdeps files (which the empty .jdeps file classpath=bazel is equivalent to only using the direct deps, like this PR is proposing). Without the empty .jdeps files --experimental_java_classpath=bazel doesn't provide an optimization close to what this PR would provide.

It would be really nice to have an experimental flag that only uses direct deps in bazel to have this option available without
needing a fork, we have been implicitly using direct deps only with a very big code base this past year and it was working pretty well.

[arunkumar9t2]

I think where the reduction happens matter, AFAIK --experimental_java_classpath=bazel does not do the input pruning before action execution so action key still contains all the transitives however it would still improve build time since during execution there is significantly less no of jars being used.

We are interested in pruning because, action key itself will not contain transitives so action will not be started at all, in addition to compile avoidance on remote builds there are less inputs to stage. We are working with large build graph (Android + Kotlin) and without pruning ABI change on leaf nodes cause significant rebuilds mostly up until the _binary target.

[guw]

@fmeum Using --experimental_java_classpath=bazel with --strict_java_deps=off does not make a difference. It is as bad as with the jdeps files in terms of rebuilds.

[larsrc-google]

The naming of this PR is confusing. Unless you use a different definition of "direct deps" than the normal "anything that's imported", you are not getting the deps you need. This PR suggests setting up a way to get "necessary deps" out of the transitive closure.

[nkoroste]

The naming of this PR is confusing. Unless you use a different definition of "direct deps" than the normal "anything that's imported", you are not getting the deps you need. This PR suggests setting up a way to get "necessary deps" out of the transitive closure.

@larsrc-google we discussed this above in #16426 (comment) - Given A -> B -> C (i.e. A depends on B depends on C) then B would be a "direct dep of A". If C is also required by A because C is part of the public API of B then B should export C in it's build definition which will make it available for A. Otherwise, if C is required by A for other reasons (i.e. not through public API of B) then it needs to be explicitly added as a dep in A build definition (which is the correct thing to do).

[raviagarwal7]

@nkoroste any plans to rebase this PR to latest bazel 7?

The native impl were moved to starlark in bazel 7 which doesn't allow this PR to be rebased cleanly. Want to know if similar support was added in starlark impl or something that will need to be added.