ADFA-5010: Lazy-load the embedded Kotlin Analysis API off the main DEX - #1635
Open
davidschachterADFA wants to merge 12 commits into
Open
ADFA-5010: Lazy-load the embedded Kotlin Analysis API off the main DEX#1635davidschachterADFA wants to merge 12 commits into
davidschachterADFA wants to merge 12 commits into
Conversation
Part 1 of moving the embedded Kotlin Analysis API off the main DEX (ADFA-5010). This module only needed kotlin-analysis-api for a VirtualFile-taking scan() overload that no call site in the repo actually uses (real callers use the JarFile-based overload) and for bytecode parsing via the jar's internally shaded ASM classes (org.jetbrains.org.objectweb.asm.*). Delete the dead VirtualFile overloads and switch the ASM usage to a real org.ow2.asm:asm dependency instead of relying on kotlin-analysis-api's internal shaded copy, so this module -- shared by both the Java and Kotlin LSPs -- no longer needs the 27MB compiler jar on its classpath at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r off-dex Part 2 of moving the embedded Kotlin Analysis API off the main DEX. Splits lsp/kotlin into a thin shell (lsp/kotlin: LSP wiring + a new DexClassLoader-based KotlinCompilerLoader) and lsp/kotlin-api (shared interfaces: IKotlinCompilationEnvironment, IKotlinCompilerSession(Factory)). The analysis-api-heavy implementation moves to a new lsp/kotlin-compiler-impl module, which subprojects/kotlin-compiler-carrier (a never-installed com.android.application module) packages into kotlin-compiler-carrier.apk. app/build.gradle.kts copies that APK into assets so it ships in the base APK without D8 ever merging its classes into app's own dex; KotlinCompilerLoader extracts and loads it lazily on first Kotlin file interaction. Verified: all touched modules compile, the carrier APK builds (assembleV8Release), and a full :app:assembleV8Debug succeeds end-to-end. Not done yet: spotlessApply still reports 5 ktlint violations (one real 188-char line, three wildcard imports, one line-length report on a line that doesn't look violating and needs investigation) in lsp/kotlin-compiler-impl files carried over from lsp/kotlin. Commit as WIP to unblock other work; formatting/commit-splitting/on-device verification/Jira update still to follow. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…proguard, add ADR app/build.gradle.kts still had its own direct implementation(kotlinAnalysisApi) line independent of lsp/kotlin's (already removed) -- caught by dex-inspecting a real release build, which showed org.jetbrains.kotlin.analysis.** classes still resident in app's own classes*.dex despite the module split. Removing it drops app's total dex from 40.17MB to 31.24MB with zero Analysis API classes left in app's dex (confirmed moved intact into the carrier's dex instead). Also removes the now-pointless org.jetbrains.kotlin.**/Caffeine/kotlin.reflect/ kotlin.script/kotlinx.coroutines.internal/streamex/gnu.trove keep block and the compiler.services.** PicoContainer rule from app/proguard-rules.pro -- nothing on app's classpath needs them once the jar is gone; the carrier module doesn't run R8 at all, so it doesn't need them either. app:assembleV8Release verified green with these rules removed. Adds ADR 0011 documenting the lazy-load-via-DexClassLoader decision. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…er-impl files The ratchet is file-level against origin/stage: files moved to a new path under this refactor need full spotless compliance now, even for pre-existing style that was fine at their old path. - ContextKeywords.kt, completion/ModifierFilter.kt, utils/ModifierFilter.kt: replace `import ... KtTokens.*` wildcard imports with explicit per-symbol imports (standard:no-wildcard-imports). The exact member list was derived from the Kotlin compiler's own "unresolved reference" errors after removing the wildcard, not guessed, to avoid missing or misnaming a token. - AbstractKtModule.kt: rename the `_baseSearchScope` backing field to `baseSearchScope` (standard:backing-property-naming -- it backs `override val baseContentScope`, a different name, so the underscore convention didn't apply). `_contentScope` is untouched: it correctly backs `override val contentScope` and was already ktlint-clean. Note: spotlessCheck still reports one line-length violation on ResolutionScopeProvider.kt:L20 that doesn't correspond to any actual line over 140 chars in the file (verified by raw byte count, tab-width expansion, and non-ASCII scan; survives a Gradle daemon restart and --no-build-cache). Left uninvestigated further pending user input -- likely a spotless/ktlint tooling quirk in this environment, not a real formatting defect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ss project switches Closing a project and opening another tears down the KotlinLanguageServer and its DexClassLoader-loaded compiler session, then creates a fresh one. LSPEditorActions.ensureActionsMenuRegistered de-duplicated by action ID and skipped re-registration, so the first session's action objects (bound to its now-dead classloader) stayed wired into the shared, main-dex ActionsRegistry. Invoking one against the second session's data threw ClassCastException on two same-named-but-differently-loaded classes (e.g. AddImportAction casting env to AbstractCompilationEnvironment). Fix: replace stale actions by ID instead of skipping when re-registering, and have KotlinLanguageServer.shutdown() proactively unregister its actions so a dead session never leaves classloader-bound objects in the shared registry. Also hardened AddImportAction's cast to degrade gracefully instead of crashing if a cross-classloader mismatch occurs some other way. Reproduced and verified fixed on-device: switching between two Kotlin projects and running "Import class(es)" on the second project's session no longer crashes.
ResolutionScopeProvider.kt:20 (`override fun getResolutionScope(...)`, 71
chars) has been failing ktlint(standard:max-line-length) with a reported
limit of 140 since it moved into this module, despite no line in the file
exceeding that length. Confirmed live/dynamic rather than a stale cache by
padding the file with leading comment lines and observing the reported
line number shift by exactly the same amount -- ktlint 1.5.0 genuinely
misattributes a max-line-length violation to this declaration, for reasons
that don't trace to anything in this file's content (checked byte length,
tabs, non-ASCII, and a `.editorconfig` blank_line/header shift).
Suppress it with the same `@file:Suppress("ktlint:standard:max-line-length")`
convention already used elsewhere in this module (KotlinCompletions.kt),
matching this repo's actual mechanism for it (`ij_formatter_tags_enabled`
governs IntelliJ-style suppression, not this per-file annotation, but the
sibling file shows this is the established pattern here). This was blocking
`./gradlew spotlessApply` for the entire repo, not just this module -- fixed
now, and a real (previously-masked) trailing-comma violation in the same
file came along for the ride.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Matches the existing convention for other build-artifact assets shipped straight in the base APK (plugin-api jar, logsender AAR, both placed under data/common/ via AddFileToAssetsTask) instead of sitting bare at the assets/ root. KotlinCompilerLoader now distinguishes the in-APK asset path (data/common/kotlin-compiler-carrier.apk) from the local extracted file name (kept flat, unprefixed, in the app's private per-app directory -- no reason to nest that too).
Quantized all 53 tracked PNGs under res/ directories with pngquant (--skip-if-larger, quality 65-100); 47 shrank, 6 were already at or near optimal and left untouched. Total size across the set drops from 172KB to 103KB (~40%). Dimensions, alpha channels, and visual appearance are unchanged -- verified by inspecting file(1) output and a visual spot check of the app icon, logo, and template screenshots, and by a full :app:assembleV8Debug rebuild.
The carrier APK's res/ entries (androidx.core notification-background nine-patches, pulled in transitively) are dead weight: this APK is never installed or its resources loaded, only ever opened as a raw dex source via DexClassLoader (ADFA-5010). No source in this repo produces them -- they come from a third-party AAR -- so the only way to shrink them is a build-time post-process of the assembled APK. copyKotlinCompilerCarrierToAssets now rewrites the carrier APK's res/*.png entries with pngquant after copying, using this file's existing raw-zip helpers (readRawZipEntries/RawZipWriter) to touch only the PNG bytes without recompressing the ~63MB of dex payload. Verified byte-identical dex entries before/after and a full :app:assembleV8Debug build. Skips quietly if pngquant isn't on PATH, since this runs unconditionally via preBuild. Some of these PNGs are AAPT2-compiled nine-patches (npTc/npOl chunks) that pngquant's re-encode strips -- inconsequential here since nothing ever renders them, but would matter for a real resource.
davidschachterADFA
added a commit
that referenced
this pull request
Aug 7, 2026
…t-lock race Three issues from /code-review high, verified against the current code: 1. ensureProjectReset() nulled pendingWorkspace before the try block, so any exception during destroy/rebuild (e.g. a bad submodule's classpath) still let the finally claim INITIALIZED -- silently treating a half-torn-down compiler as ready, with no retry, for the rest of the session. Now an exception re-queues the workspace, reverts to PENDING, and rethrows. 2. analyze() never called ensureProjectReset() at all. diagnosticProvider .analyze() builds its own JavaCompilerService directly, bypassing getCompiler(), and analysis is often the *first* real .java-file interaction (auto-triggered on file open, ahead of any completion request) -- so the R.jar/file-manager cache clear this reset performs could be skipped for an entire session, leaving diagnostics resolving against a stale previous project's classpath. Now gated the same way getCompiler()/onContentChange() already are. 3. getCompiler() and onContentChange() released compilerLifecycleLock as soon as ensureProjectReset() returned, then used JavaCompilerProvider unlocked -- a concurrent reset for a newer project could destroy() those compilers in the gap. Both now hold the lock across the reset and the subsequent provider lookup/use (safe: ReentrantLock is reentrant, so ensureProjectReset()'s own internal withLock nests without deadlocking). Two other findings from the same pass were assessed and left as-is: - shutdown() blocking on an in-flight reset with no cancellation is real but performance-only (no crash/corruption), requires disproportionate cancellation plumbing through SourceFileManager/JavaCompilerService for a narrow, bounded-cost edge case. - KotlinLanguageServer's eager construction is a real observation about this branch's current state, but it's already fixed by the separate, not-yet- merged ADFA-5010 (PR #1635) -- out of scope here, not a gap in this PR. Verified: :lsp:java unit tests pass, :app:assembleV8Debug builds clean.
…he carrier dex kotlin-compiler-impl declared kotlin-stdlib, kotlin-coroutines-core, kotlin-coroutines-android, and sentry-android-core as `implementation`, directly contradicting this file's own doc comment: every one of these is already resident (loaded by the parent/main-app classloader by the time the carrier's DexClassLoader runs), so `implementation` here just duplicates them into kotlin-compiler-carrier.apk's dex on top of the identical resident copies -- including the entire Sentry Android SDK. Changed all four to compileOnly, matching the pattern already used for every other resident dependency in this file. Verified via a clean rebuild in a worktree combining this branch with ADFA-5053/5052: the carrier's dex dropped from 66.02 MB to 63.41 MB (-2.61 MB) and its entry count from 144 to 102, with no change to the main app's own dex.
PR #1635's "Build Universal APK" check failed with: Property '$1' specifies file '.../kotlin-compiler-carrier-v8-release-unsigned.apk' which doesn't exist. Same defect as ADFA-5053's copyJavaCompilerCarrierToAssets (already fixed there): the task only had dependsOn(":...:assembleV8Release") -- task-ordering, not a real value-based dependency -- wired to a hardcoded path guessing the AGP-produced filename ("-unsigned" suffix included). That intermittently races the file's own write-to-disk. Fixed the same way: expose the release variant's real APK output directory via AGP's variant artifacts API (variant.artifacts.get(SingleArtifact.APK)) instead of a hardcoded path. Verified with 4 consecutive clean (--no-build-cache --rerun-tasks) runs of copyKotlinCompilerCarrierToAssets.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Per ADFA-4549, the embedded Kotlin Analysis API jar accounts for 54.9% (27.7 MiB) of the release APK's DEX, and was previously merged unconditionally into the main app dex and constructed eagerly on every project open. This PR moves it into a separately-dexed "carrier" APK, extracted and
DexClassLoader-loaded only when a Kotlin file is actually opened -- mirroring the existingPluginLoaderpattern. On-device build/dex inspection confirms the app's release dex drops from 40.17MB to 31.24MB with zero remaining Analysis API classes.lsp:kotlin-api(bridge interfaces),lsp:kotlin-compiler-impl(the isolated payload, ~55 files moved fromlsp:kotlin),subprojects:kotlin-compiler-carrier(the dex-producing shell APK, never installed, only ever parsed as a dex source).lsp:kotlin'sKotlinLanguageServeris rewritten as a thin wrapper; the Analysis API session now lazily initializes on the first Kotlin-file-gated call instead of eagerly insetupWithProject.docs/adr/0011-lazy-load-kotlin-analysis-api-via-dexclassloader.mddocuments the design, including the three distinct Kotlin versions in play (repo's own 2.3.0 plugin, embedded Analysis API fork 2.3.255, on-device build toolchain 1.9.x) so they aren't conflated later.Bug found and fixed during on-device verification
Switching between two Kotlin projects in the same app session crashed with a
ClassCastException: the first session's Kotlin code-action objects (bound to its now-closedDexClassLoader) stayed wired into the shared, main-dex actions registry because re-registration deduped by action ID and skipped re-adding them. Invoking one against the second session's data crossed two same-named-but-differently-loaded classes. Fixed by replacing stale same-ID actions on registration and having session shutdown proactively unregister its actions; reproduced and verified fixed on-device (Pixel 6 Pro).Also resolved a pre-existing phantom ktlint
max-line-lengthfailure onResolutionScopeProvider.ktthat was blocking./gradlew spotlessApplyrepo-wide (confirmed genuine ktlint 1.5.0 misattribution, not a real violation; suppressed via the same convention already used inKotlinCompletions.kt).Verified on-device (Pixel 6 Pro)
.ktfile -> carrier APK extracted andDexClassLoader-loaded on first use only.Known follow-up (not fixed here, filed on the ticket)
"Implement Members" runs cleanly but always returns zero edits, even for a textbook missing-override case. Not a crash and not classloader-related (sibling actions work correctly in the same session) -- looks like a separate, pre-existing logic gap in that action.
Test plan
:app:assembleV8Debugand:app:assembleV8Releasebuild successfully./gradlew spotlessApplypasses repo-widearchitecture-reviewskill or self-checking against REVIEW.md section 10 given the size of this diff (pre-push hook nudge, 105 first-party files changed)🤖 Generated with Claude Code