Build kotlin-analysis-symbols module with the latest compiler version
#4182
Conversation
vmishenev
force-pushed
the
vmishenev/update-aa-to-2.3.0-dev-1719
branch
from
e41e3b7 to
d790338
Compare
|
In DGPv2, a shadowed |
vmishenev
force-pushed
the
vmishenev/update-aa-to-2.3.0-dev-1719
branch
from
64b693b to
9d240c7
Compare
vmishenev
force-pushed
the
vmishenev/update-aa-to-2.3.0-dev-1719
branch
from
9d240c7 to
a8f07e6
Compare
vmishenev
force-pushed
the
vmishenev/update-aa-to-2.3.0-dev-1719
branch
from
7f5a163 to
773365a
Compare
vmishenev
force-pushed
the
vmishenev/update-aa-to-2.3.0-dev-1719
branch
from
42b24ac to
7bb7898
Compare
dokka-runners/dokka-gradle-plugin/src/testFunctional/kotlin/DokkaGeneratorLoggingTest.kt
Show resolved
Hide resolved
whyoleg
left a comment
whyoleg
left a comment
There was a problem hiding this comment.
Great job!!
I do have some comments, which might (or might not) could simplify the things a bit. I would also like to wait for @adam-enko review before merging it
build-logic/src/main/kotlin/dokkabuild/utils/gradleKotlinCompatibility.kt
Show resolved
Hide resolved
...ation-tests/gradle/src/testTemplateProjectMultimodule0/kotlin/MultiModule0IntegrationTest.kt
Outdated
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/tasks/DokkaGenerateTask.kt
Show resolved
Hide resolved
...jetbrains/dokka/analysis/kotlin/symbols/translators/DefaultSymbolToDocumentableTranslator.kt
Outdated
Show resolved
Hide resolved
|
Please check CI :) |
vmishenev
force-pushed
the
vmishenev/update-aa-to-2.3.0-dev-1719
branch
from
7deeb3c to
eebbb41
Compare
| * To be compatible with the AA, Dokka analysis should be compiled with approximately the same version | ||
| * See https://kotlinlang.org/docs/kotlin-evolution-principles.html#evolving-the-binary-format | ||
| */ | ||
| val analysisK2Projects = listOf("analysis-kotlin-symbols") |
There was a problem hiding this comment.
I'd really rather avoid referring to subprojects by name in convention plugins. It makes the conventions more fragile, and it's harder to reason about.
Instead, I'd suggest updating DokkaBuildProperties to either add btaCompilerVersion and btaLanguageVersion, or a a single boolean on/off flag and zip it with the properties. Then the behaviour can be controlled per subproject more idiomatically.
There was a problem hiding this comment.
Instead, I'd suggest updating DokkaBuildProperties to either add btaCompilerVersion and btaLanguageVersion, or a a single boolean on/off flag and zip it with the properties.
This is enforceGradleKotlinCompatibility, not an extra flag, but I cannot set it per project due to gradle/gradle#23572. (see the discussion above)
Although, I could use a workaround from there. WDYT?
There was a problem hiding this comment.
Both workaround and explicit project specification here are fine for me, as in reality, we don't need a general solution here, but only for this specific module, because of a specific issue
There was a problem hiding this comment.
Let's leave it as is. If I have time I'll come back and tinker with it later.
dokka-runners/dokka-gradle-plugin/src/main/kotlin/internal/dokkaBootstrapFactory.kt
Outdated
Show resolved
Hide resolved
dokka-runners/dokka-gradle-plugin/src/main/kotlin/internal/dokkaBootstrapFactory.kt
Outdated
Show resolved
Hide resolved
|
CI OOMs during DGPv1, Jekyll, and Jackson? |
| ":moduleA:dokkaHtmlMultiModule", | ||
| ":moduleA:dokkaGfmMultiModule", | ||
| ":moduleA:dokkaJekyllMultiModule", | ||
| jvmArgs = listOf("-Xmx2G", "-XX:MaxMetaspaceSize=1600m") |
There was a problem hiding this comment.
TBH, I don't really like that we increased this value from -XX:MaxMetaspaceSize=1G to 1.6 GB already.
WDYT about splitting the PR into two parts:
- re-setup analysis modules, building, and Dokka running
- updating the version of AA
So far, it's not clear what causes this 50% spike in metaspace memory usage. We should probably not just ignore it by increasing the memory used.
There was a problem hiding this comment.
I like the idea of splitting it up. We could also split the custom classloader into a separate PR (it'd help me be able to review only the parts that are related to DGP)
There was a problem hiding this comment.
It seems we have the same problem we encountered during the stress tests on JVM 8 — #3802.
The change in the order of dependencies (and the stdlib version) in the classpath somehow affects GC that leads to flaky integration tests.
Nevertheless, there is no memory leak.
I also noticed one misuse in Dokka runtime classpath.
Actually it has two different versions of stdlib: a shadowed version in kotlin-analysis-* and one from other dependencies.
For example, it has 2 different versions: 2.2 (inkotlin-analysis-* ) and 2.0.21
Version aligning does not work for the shadowed stdlib.
I think it causes the classloader/GC to perform additional work.
There was a problem hiding this comment.
JIC aligning the stdlib version with 2.2.0 does NOT help to avoid increasing (aggressive GC) metaspace in the tests.
I used this hack for an experiment - b7fb1d2
(the same works for the base-plugin). Notice: at compile time the old stdlib is used.
The build scan
An alternative solution is to build base-plugin with Kotlin 2.2.0 so it will have stdlib 2.2.0. In this case, we don't need to change the DGPv1. Also, we can consider to exclude the shadowed stdlib from kotlin-analysis-*
There was a problem hiding this comment.
I also noticed one misuse in Dokka runtime classpath.
Actually it has two different versions of stdlib: a shadowed version in kotlin-analysis-* and one from other dependencies.
But I believe that it should not cause any issues, as we load stdlib classes from kotlin-analysis-* artefacts first?
I mean, we had the same situation previously, as shadowed kotlin-analysis-* always contains kotlin-stdlib coming from kotlin-compiler
Or am I missing something?
An alternative solution is to build base-plugin with Kotlin 2.2.0 so it will have stdlib 2.2.0.
I'm not sure I understand how this will help, as we will still have multiple stdlib versions (one from kotlin-compiler-k2 and one from base), though they will be closer to each other (2.2.0-dev/2.3.0-dev and 2.2.0).
There was a problem hiding this comment.
I mean, we had the same situation previously, as shadowed kotlin-analysis-* always contains kotlin-stdlib coming from kotlin-compiler
Sorry, I forgot that kotlin-compiler already includes a shadowed stdlib, although kotlin-compiler-for-ide does not have.
So, for now, there is nothing we can do about it.
But I believe that it should not cause any issues, as we load stdlib classes from kotlin-analysis-* artefacts first?
Yes, in theory, it could only lead to performance issues, but I’m not sure.
| ":moduleA:dokkaJekyllMultiModule", | ||
| jvmArgs = listOf( | ||
| "-Xmx1G", "-XX:MaxMetaspaceSize=600m", | ||
| "-XX:SoftRefLRUPolicyMSPerMB=10" // to free up the metaspace on JVM 8, see https://youtrack.jetbrains.com/issue/KT-55831/ |
There was a problem hiding this comment.
Interesting!
Do you think we should consider adding this arg to the process-isolated workers in DGPv2?
Also, perhaps -XX:+AlwaysPreTouch would help here too? (I'm not very confident it helps with DGPv2, but it doesn't seem to hurt.)
dokka/dokka-runners/dokka-gradle-plugin/src/main/kotlin/DokkaBasePlugin.kt
Lines 115 to 123 in 71fa92f
There was a problem hiding this comment.
Do you think we should consider adding this arg to the process-isolated workers in DGPv2?
I prefer using a cached classloader if it is possible with Shared Build Services.
We can temporarily use this flag in DGPv2 until a cached classloader is implemented.
If it’s really needed, let’s do it in another PR.
Also, perhaps -XX:+AlwaysPreTouch would help here too?
Here "-XX:SoftRefLRUPolicyMSPerMB=10" help us very well. I’d like to minimize the use of "magical" JVM flags.
There was a problem hiding this comment.
Do you think we should consider adding this arg to the process-isolated workers in DGPv2?
I think we previously decided that it's fine to use this option in tests only in #3802, so I suggest sticking with it.
There was a problem hiding this comment.
We discussed briefly in KBT and we think it could be helpful, but it's a sensitive flag so let's leave it for now.
vmishenev
force-pushed
the
vmishenev/update-aa-to-2.3.0-dev-1719
branch
from
892e4df to
3b3ddbe
Compare
vmishenev
changed the title
vmishenev
changed the title
kotlin-analysis-symbols module with the latest compiler version
| * To be compatible with the AA, Dokka analysis should be compiled with approximately the same version | ||
| * See https://kotlinlang.org/docs/kotlin-evolution-principles.html#evolving-the-binary-format | ||
| */ | ||
| val analysisK2Projects = listOf("analysis-kotlin-symbols") |
There was a problem hiding this comment.
Let's leave it as is. If I have time I'll come back and tinker with it later.
| ":moduleA:dokkaJekyllMultiModule", | ||
| jvmArgs = listOf( | ||
| "-Xmx1G", "-XX:MaxMetaspaceSize=600m", | ||
| "-XX:SoftRefLRUPolicyMSPerMB=10" // to free up the metaspace on JVM 8, see https://youtrack.jetbrains.com/issue/KT-55831/ |
There was a problem hiding this comment.
We discussed briefly in KBT and we think it could be helpful, but it's a sensitive flag so let's leave it for now.
Here,
kotlin-analysis-symbolsis built with the latest compiler version (#4178), and Dokka uses the shadowed stdlib fromkotlin-analysis-symbolsorkotlin-analysis-descriptors.