KT-45777: Compute classpath changes for incremental Kotlin compile #4564

hungvietnguyen · 2021-09-01T11:34:50Z

No description provided.

to prevent broken serialization caused by missing fields/methods. (Without this commit, the next commit will break tests.)

to organize tested scenarios better and make it easier to see the effects of the next change when the classpath snapshot feature is enabled.

Test: Updated unit tests + incremental compilation integration tests

hungvietnguyen · 2021-09-03T09:25:02Z

Updated PR to fix failed tests:

Added commit "KT-45777: Update keep rules for serializable classes"
Amended commit "KT-45777: Compute classpath changes for incremental Kotlin compile" (reverted some changes in externalizers.kt)

gavra0 · 2021-09-03T09:59:29Z

...on-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/IncrementalCompilationMultiProjectIT.kt

        }
    }

+    /** Expected files to be recompiled for [testCompileLibWithGroovy], which may be overridden in subclasses. */
+    open fun testCompileLibWithGroovy_expectedFiles(project: Project): Iterable<File> {


Let's override the test case instead? I know there is some duplication, but it is better than having expected values overriden.

Yup, I override the test case now, and extracted the common code to another method.

gavra0 · 2021-09-03T10:06:07Z

...on-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/IncrementalCompilationMultiProjectIT.kt

        )
    }

    @Test
-    fun testMoveFunctionFromLibToApp() {
+    fun testAddDependencyInLib() {


There used to be one more incremental build in this test case, asserting that we can run have build -> (change dep) non-incremental build -> (change source) incremental build i.e that we can "recover" after a non-incremental one. Should we restore old behavior?

That's a good catch! I thought about that too and felt the "recovery" check is not necessary.

The reason is that a non-incremental build should be deterministic, regardless of what happened before (with the exception of cache hit with @LocalState, but we're not testing caching here). So if a build is non-incremental, the next build should always be incremental (given a typical change).

The "recovery" check has a tiny bit of value, but it is added to several test cases making the tests not focused and increasing test execution time.

If the check is important, I can add a separate test just for it, but it's not clear to me which scenario we should use to set up the test.

Also: When classpath snapshot is enabled, app compilation will be incremental right in the second build, so I guess this won't be an issue in the long run when the feature is enabled by default.

SGTM, I agree there are other incremental scenario tests that cover this.

gavra0 · 2021-09-03T10:07:39Z

...-gradle-plugin-integration-tests/src/test/kotlin/org/jetbrains/kotlin/gradle/BaseGradleIT.kt

@@ -122,7 +122,7 @@ abstract class BaseGradleIT {

        // gradle wrapper version to wrapper directory
        private val gradleWrappers = hashMapOf<String, File>()
-        private const val MAX_DAEMON_RUNS = 30
+        private const val MAX_DAEMON_RUNS = 100


I'm curious, why do we need to increase this? The existing infra should restart daemon when MAX_DAEMON_RUNS is reached.

The test infra keeps track of the number of active daemons (MAX_ACTIVE_GRADLE_PROCESSES=1) and the number of times daemons are invoked (MAX_DAEMON_RUNS=30). I'm not sure why the second is necessary, but if I keep the old number, this PR--which adds more tests--will cross this threshold and start failing the last few tests. I added a comment below:

if (DaemonRegistry.runCountForDaemon(version) >= MAX_DAEMON_RUNS) { // Warning: Stopping the Gradle daemon while the test suite has not finished running can break tests in weird ways. // TODO: Find a safe way to do this or consider deleting this code. stopDaemon(version, environmentVariables) }

Increasing this threshold to ~40 would have fixed the issue, but the next time someone else runs into this issue, they would need to increase it again (after some significant time debugging if they're not aware of this code), so I increased it to 100 to avoid that trouble.

gavra0 · 2021-09-03T10:20:06Z

...e-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/incremental/ClasspathChangesComputer.kt

-        }
+        val workingDir =
+            FileUtil.createTempDirectory(this::class.java.simpleName, "_WorkingDir_${UUID.randomUUID()}", /* deleteOnExit */ true)
+        val incrementalJvmCache = IncrementalJvmCache(workingDir, /* targetOutputDir */ null, FileToCanonicalPathConverter)


For the next PR(s), we can also explore storing the previous compilation in IncrementalJvmCache so that we can avoid populating it with the previous snapshot.

That's a good idea. It's a bit tricky since we want to compare the changed parts of the classpath only, but I've noted your suggestion to explore later once we have more insights on what parts of the task are taking time.

gavra0 · 2021-09-03T10:21:43Z

...e-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/incremental/ClasspathChangesComputer.kt

-                return result
+        // Store previous class snapshots in incrementalJvmCache, the returned ChangesCollector result is not used.
+        val unusedChangesCollector = ChangesCollector()
+        for (previousSnapshot in previousClassSnapshots) {


(next PR) Can you please add more performance-related logging, using info level is probably ok? It'd be interesting to know how much time is spent in: computing individual ABI snapshots, loading previous, loading current, diffing the two etc.

Yes, once we've guaranteed correctness, I'll start measuring & optimizing.

gavra0 · 2021-09-03T10:37:43Z

libraries/tools/kotlin-gradle-plugin/src/main/kotlin/org/jetbrains/kotlin/gradle/tasks/Tasks.kt

+    private fun getClasspathChanges(knownChangedFiles: ChangedFiles.Known): ClasspathChanges {
+        // Find current snapshot files that have been changed (added or modified)
+        val currentSnapshotFiles = classpathSnapshotProperties.classpathSnapshot.files
+        val addedOrModifiedFiles = knownChangedFiles.modified.toSet()


Can we use Gradle APIs to the the list of changes directly?

Done - I've used InputChanges now. But as we discussed offline, comparing the changed parts only will be incorrect if the classpath contains duplicate classes, so I'm comparing the entire classpath now to be safe. I'll try to address this in the next PR.

…edFiles

Test: Updated existing incremental compilation integration tests

hungvietnguyen · 2021-09-07T10:56:24Z

Added 1 more commit to support compile avoidance.

hungvietnguyen · 2021-09-14T09:50:58Z

5/6 commits of this PR have been merged. The remaining commit will be included in the next PR: #4574.

abelkov added the Gradle Plugin label Sep 1, 2021

abelkov assigned nav-nav Sep 1, 2021

hungvietnguyen added 3 commits September 3, 2021 09:34

KT-45777: Update keep rules for serializable classes

e3c4e98

to prevent broken serialization caused by missing fields/methods. (Without this commit, the next commit will break tests.)

KT-45777: Refactor incremental compilation integration tests

ab64db7

to organize tested scenarios better and make it easier to see the effects of the next change when the classpath snapshot feature is enabled.

KT-45777: Compute classpath changes for incremental Kotlin compile

e89a0c7

Test: Updated unit tests + incremental compilation integration tests

hungvietnguyen force-pushed the aprr/hungnv/KT-45777-computeClasspathChanges branch from 165315e to e89a0c7 Compare September 3, 2021 09:22

gavra0 reviewed Sep 3, 2021

View reviewed changes

hungvietnguyen added 3 commits September 6, 2021 14:41

KT-45777: Address review comments - update tests

3565e96

KT-45777: Address review comments - use InputChanges instead of Chang…

cbded39

…edFiles

KT-45777: Support compile avoidance

462243e

Test: Updated existing incremental compilation integration tests

gavra0 approved these changes Sep 7, 2021

View reviewed changes

hungvietnguyen closed this Sep 14, 2021

hungvietnguyen deleted the aprr/hungnv/KT-45777-computeClasspathChanges branch September 14, 2021 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KT-45777: Compute classpath changes for incremental Kotlin compile #4564

KT-45777: Compute classpath changes for incremental Kotlin compile #4564

hungvietnguyen commented Sep 1, 2021

hungvietnguyen commented Sep 3, 2021

gavra0 Sep 3, 2021

hungvietnguyen Sep 6, 2021

gavra0 Sep 3, 2021

hungvietnguyen Sep 6, 2021

gavra0 Sep 7, 2021

gavra0 Sep 3, 2021

hungvietnguyen Sep 6, 2021

gavra0 Sep 7, 2021

gavra0 Sep 3, 2021

hungvietnguyen Sep 6, 2021

gavra0 Sep 3, 2021

hungvietnguyen Sep 6, 2021

gavra0 Sep 3, 2021

hungvietnguyen Sep 6, 2021

hungvietnguyen commented Sep 7, 2021

hungvietnguyen commented Sep 14, 2021

KT-45777: Compute classpath changes for incremental Kotlin compile #4564

KT-45777: Compute classpath changes for incremental Kotlin compile #4564

Conversation

hungvietnguyen commented Sep 1, 2021

hungvietnguyen commented Sep 3, 2021

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

hungvietnguyen commented Sep 7, 2021

hungvietnguyen commented Sep 14, 2021