Improve jdeps logic to find more explicit and implicit dependencies #1118

scosenza · 2024-02-26T18:38:18Z

Problem

Kotlin jdeps do not collect many direct and indirect dependencies that are required for compilation. These missing jdep found dependencies were discovered as Airbnb tests a "jdeps compile-time pruning" optimization which provides significant IntelliJ indexing, UI, and compilation performance benefits in our fork of the Bazel IntelliJ Plugin. See also #1115 as @Bencodes and I unknowingly started looking at this issue at about the same time!

Solution

As a followup to #1045, write test cases to repro missing deps found when compiling Airbnb's JVM monorepo, and then fix the underlying issues. Note that these fixes were made with very minimal knowledge of Kotlin compiler internals, so please suggest alternatives if there are better ways to collect these types. Thanks!

Test Plan:

Wrote Kotlin and Java test cases to repro missing dependencies found in the Airbnb monorepo
Updated Airbnb's minimal rules_kotlin fork to use the changes in this PR and the updated rules_kotlin fork has been successfully running in CI for over a week.

Appendix

For anyone interested in how we're planning to use jdeps pruning:

We've been experimenting with using IntelliJ's incremental compiler to perform compilations after the initial Bazel sync.
We use jdeps to prune the compile classpath, so that only the interface jars needed to compile the editable targets are added as IntelliJ libraries. For one of our large services, we see a 20x reduction in compile time jars (2000 -> 100). This reduction speeds up indexing, UI responsiveness, and incremental compilation speed (5s -> 1s).
Using IntelliJ's incremental compiler with a jdeps pruned classpath has been 30-70x faster than "bazel build/test" for the same large service (30-70s -> 1s) on an M2 Mac. Note that we follow "1:1:1" and have a BUILD file in every Java/Kotlin/Scala package.

…ncies (bazelbuild#17) # Problem Kotlin jdeps do not collect many direct and indirect dependencies that are required for compilation. These missing jdep found dependencies were discovered as Airbnb tests a "jdeps compile-time pruning" optimization which provides significant IntelliJ indexing, UI, and compilation performance benefits in our fork of the [Bazel IntelliJ Plugin](https://github.com/bazelbuild/intellij). # Solution As a followup to bazelbuild#1045, write test cases to repro missing dependencies found in Airbnb's monorepo, and then fix the underlying issues. In addition to these fixes helping our "jdeps compile-time pruning" optimization, these fixes should also help when the normal Bazel IntelliJ Plugin is used, as it also consults jdeps when deciding what jars to import into IntelliJ. # Test Plan: * Wrote Kotlin and Java test cases to repro missing dependencies found in the Airbnb monorepo * Updated Airbnb's minimal rules_kotlin fork to use the changes in this PR, which has been successfully running CI for over a week.

scosenza · 2024-02-26T18:39:08Z

src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt

@@ -46,6 +46,7 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) {
  val ctx = KotlinJvmTestBuilder()

  val TEST_FIXTURES_DEP = Dep.fromLabel(":JdepsParserTestFixtures")
+  val TEST_FIXTURES2_DEP = Dep.fromLabel(":JdepsParserTestFixtures2")


To enable test cases where Java deps depend on other Java deps I added a second set of java test fixtures here

scosenza · 2024-02-26T18:42:21Z

src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt

@@ -1385,7 +1621,7 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) {

    assertExplicit(jdeps).contains(depWithFunction.singleCompileJar())
    assertImplicit(jdeps).contains(depWithReturnType.singleCompileJar())
-    assertImplicit(jdeps).doesNotContain(depWithReturnTypesSuperType)
+    assertImplicit(jdeps).contains(depWithReturnTypesSuperType.singleCompileJar())


Note that the prior assertion always succeeded as it was missing .singleCompileJar()
When .singleCompileJar() is correctly added, the assertion fails.
We can confirm that depWithReturnTypesSuperType is needed for compilation by commenting out:
c.addTransitiveDependencies(depWithReturnTypesSuperType)
and then seeing the compilation fail with:

error: supertypes of the following classes cannot be resolved. Please make sure you have the required dependencies in the classpath: class something.SomeType, unresolved supertypes: something.SomeSuperType

Because the super type is actually needed, we've changed the assertion to assert that it is an implicit dep.

scosenza · 2024-02-26T18:46:53Z

src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt

@@ -1659,7 +1896,34 @@ class KotlinBuilderJvmJdepsTest(private val enableK2Compiler: Boolean) {

    assertExplicit(jdeps).containsAtLeast(depWithFunction.singleCompileJar(), depWithReturnType.singleCompileJar())
    assertImplicit(jdeps).doesNotContain(depWithReturnType.singleCompileJar())
-    assertImplicit(jdeps).doesNotContain(depWithReturnTypesSuperType)
+    assertImplicit(jdeps).contains(depWithReturnTypesSuperType.singleCompileJar())


Note that the prior assertion always succeeded as it was missing .singleCompileJar()
When .singleCompileJar() is correctly added, the assertion fails.
We can confirm that depWithReturnTypesSuperType is needed by commenting out:
c.addTransitiveDependencies(depWithReturnTypesSuperType)
and then seeing the compilation fail with:

error: supertypes of the following classes cannot be resolved. Please make sure you have the required dependencies in the classpath: class something.SomeType, unresolved supertypes: something.SomeSuperType

Because the super type is actually needed, we've changed the assertion to assert that it is an implicit dep.

I have a PR up that does full Deps diffing which should hopefully avoid issues like this where assertions aren't being correctly done.

https://github.com/bazelbuild/rules_kotlin/pull/1116/files

Bencodes · 2024-02-26T23:56:42Z

src/test/kotlin/io/bazel/kotlin/builder/tasks/jvm/KotlinBuilderJvmJdepsTest.kt

+      )
+    }
+
+    println("=====================================")


Did you mean to check this in?

Thanks for flagging. Removed.

Bencodes · 2024-02-27T00:21:20Z

src/test/kotlin/io/bazel/kotlin/builder/tasks/BUILD.bazel

+java_library(
+    name = "JdepsParserTestFixtures2",
+    testonly = True,
+    srcs = glob(["testFixtures2/*.java"]),


Might look less copy-paste grouping these different test figures inside directories within the testFixtures directory instead of just duplicating and adding a numerical index. Ex: testFixtures/a-sources/ testFixtures/b-sources/

Bencodes · 2024-03-01T18:56:23Z

@scosenza are you able to rebase this branch and resolve the conflicts + organize the test fixtures?

…enza/rules_kotlin into fix_missing_jdep_found_deps

scosenza · 2024-03-02T18:34:14Z

FYI, it looks like WorkerEnvironmentTest may be flakey, as it's passing on my local Mac machine, and passing in the Linux CI build. Update: I just triggered a new CI build by adding a newline, and now WorkerEnvironmentTest is passing.

scosenza · 2024-03-02T18:59:29Z

@scosenza are you able to rebase this branch and resolve the conflicts + organize the test fixtures?

@Bencodes I just updated my branch with the latest upstream changes. And thanks again for greatly improving the assertions!

Bencodes · 2024-03-02T19:22:52Z

src/main/kotlin/io/bazel/kotlin/plugin/jdeps/JdepsGenExtension.kt

 import org.jetbrains.kotlin.descriptors.impl.LocalVariableDescriptor
 import org.jetbrains.kotlin.extensions.StorageComponentContainerContributor
-import org.jetbrains.kotlin.load.java.descriptors.JavaMethodDescriptor
-import org.jetbrains.kotlin.load.java.descriptors.JavaPropertyDescriptor
+import org.jetbrains.kotlin.js.translate.callTranslator.getReturnType


Is this the right function to be using? It looks like it's being loaded from the javascript package.

I briefly looked around and didn't see anything that stands out. If anything you can just inline the function itself, which is just calling into another function with resolvedCall as an argument.

Thanks for flagging this.

…onger needed.

…azelbuild#1118) * Improve Kotlin jdeps logic to find more explicit and implicit dependencies (bazelbuild#17) # Problem Kotlin jdeps do not collect many direct and indirect dependencies that are required for compilation. These missing jdep found dependencies were discovered as Airbnb tests a "jdeps compile-time pruning" optimization which provides significant IntelliJ indexing, UI, and compilation performance benefits in our fork of the [Bazel IntelliJ Plugin](https://github.com/bazelbuild/intellij). # Solution As a followup to bazelbuild#1045, write test cases to repro missing dependencies found in Airbnb's monorepo, and then fix the underlying issues. In addition to these fixes helping our "jdeps compile-time pruning" optimization, these fixes should also help when the normal Bazel IntelliJ Plugin is used, as it also consults jdeps when deciding what jars to import into IntelliJ. # Test Plan: * Wrote Kotlin and Java test cases to repro missing dependencies found in the Airbnb monorepo * Updated Airbnb's minimal rules_kotlin fork to use the changes in this PR, which has been successfully running CI for over a week. * formatting * formatting * revert * Remove println used for testing * use new assertion style in jdeps test * add newline to trigger another build * Remove class in "js" package, that shouldn't be used and also is no longer needed. * Remove class in "js" package, that shouldn't be used and also is no longer needed. --------- Co-authored-by: Steve Cosenza <steve.cosenza@airbnb.com>

kolloch · 2024-04-23T04:56:20Z

@scosenza the uses of the jdeps pruning sound really exciting. Any chance that people outside of airbnb could use a similar setup or does it all depend on your bazel intellij plugin fork?

…azelbuild#1118) * Improve Kotlin jdeps logic to find more explicit and implicit dependencies (bazelbuild#17) # Problem Kotlin jdeps do not collect many direct and indirect dependencies that are required for compilation. These missing jdep found dependencies were discovered as Airbnb tests a "jdeps compile-time pruning" optimization which provides significant IntelliJ indexing, UI, and compilation performance benefits in our fork of the [Bazel IntelliJ Plugin](https://github.com/bazelbuild/intellij). # Solution As a followup to bazelbuild#1045, write test cases to repro missing dependencies found in Airbnb's monorepo, and then fix the underlying issues. In addition to these fixes helping our "jdeps compile-time pruning" optimization, these fixes should also help when the normal Bazel IntelliJ Plugin is used, as it also consults jdeps when deciding what jars to import into IntelliJ. # Test Plan: * Wrote Kotlin and Java test cases to repro missing dependencies found in the Airbnb monorepo * Updated Airbnb's minimal rules_kotlin fork to use the changes in this PR, which has been successfully running CI for over a week. * formatting * formatting * revert * Remove println used for testing * use new assertion style in jdeps test * add newline to trigger another build * Remove class in "js" package, that shouldn't be used and also is no longer needed. * Remove class in "js" package, that shouldn't be used and also is no longer needed. --------- Co-authored-by: Steve Cosenza <steve.cosenza@airbnb.com>

scosenza commented Feb 26, 2024

View reviewed changes

scosenza marked this pull request as ready for review February 26, 2024 18:50

scosenza requested review from ahumesky, cgruber, restingbull, Bencodes and nkoroste as code owners February 26, 2024 18:50

Steve Cosenza added 3 commits February 26, 2024 11:20

formatting

59c3840

formatting

c58339e

revert

816086e

Bencodes reviewed Feb 26, 2024

View reviewed changes

Bencodes reviewed Feb 27, 2024

View reviewed changes

Remove println used for testing

ab93f87

Steve Cosenza added 3 commits March 2, 2024 09:59

merge master

f91e46d

use new assertion style in jdeps test

425a78f

Merge branch 'fix_missing_jdep_found_deps' of https://github.com/scos…

1f5853b

…enza/rules_kotlin into fix_missing_jdep_found_deps

add newline to trigger another build

80ed7fb

Bencodes reviewed Mar 2, 2024

View reviewed changes

Steve Cosenza added 2 commits March 3, 2024 16:37

Remove class in "js" package, that shouldn't be used and also is no l…

f8605f8

…onger needed.

Remove class in "js" package, that shouldn't be used and also is no l…

46ca4d9

…onger needed.

scosenza mentioned this pull request Mar 4, 2024

WorkerEnvironmentTest is flakey #1124

Open

Bencodes approved these changes Mar 4, 2024

View reviewed changes

Bencodes merged commit e528366 into bazelbuild:master Mar 4, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve jdeps logic to find more explicit and implicit dependencies #1118

Improve jdeps logic to find more explicit and implicit dependencies #1118

scosenza commented Feb 26, 2024 •

edited

Loading

scosenza Feb 26, 2024

scosenza Feb 26, 2024 •

edited

Loading

scosenza Feb 26, 2024 •

edited

Loading

Bencodes Feb 27, 2024

Bencodes Feb 26, 2024

scosenza Feb 27, 2024 •

edited

Loading

Bencodes Feb 27, 2024

Bencodes commented Mar 1, 2024

scosenza commented Mar 2, 2024 •

edited

Loading

scosenza commented Mar 2, 2024

Bencodes Mar 2, 2024

Bencodes Mar 3, 2024

scosenza Mar 4, 2024 •

edited

Loading

kolloch commented Apr 23, 2024

Improve jdeps logic to find more explicit and implicit dependencies #1118

Improve jdeps logic to find more explicit and implicit dependencies #1118

Conversation

scosenza commented Feb 26, 2024 • edited Loading

Problem

Solution

Test Plan:

Appendix

scosenza Feb 26, 2024

Choose a reason for hiding this comment

scosenza Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

scosenza Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Bencodes Feb 27, 2024

Choose a reason for hiding this comment

Bencodes Feb 26, 2024

Choose a reason for hiding this comment

scosenza Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Bencodes Feb 27, 2024

Choose a reason for hiding this comment

Bencodes commented Mar 1, 2024

scosenza commented Mar 2, 2024 • edited Loading

scosenza commented Mar 2, 2024

Bencodes Mar 2, 2024

Choose a reason for hiding this comment

Bencodes Mar 3, 2024

Choose a reason for hiding this comment

scosenza Mar 4, 2024 • edited Loading

Choose a reason for hiding this comment

kolloch commented Apr 23, 2024

scosenza commented Feb 26, 2024 •

edited

Loading

scosenza Feb 26, 2024 •

edited

Loading

scosenza Feb 26, 2024 •

edited

Loading

scosenza Feb 27, 2024 •

edited

Loading

scosenza commented Mar 2, 2024 •

edited

Loading

scosenza Mar 4, 2024 •

edited

Loading