Skip to content

Restore R8 per-function DCE: sync getValueCached + ProGuard rule fix#201

Merged
kirich1409 merged 5 commits into
developfrom
feat/sync-getvaluecached
May 20, 2026
Merged

Restore R8 per-function DCE: sync getValueCached + ProGuard rule fix#201
kirich1409 merged 5 commits into
developfrom
feat/sync-getvaluecached

Conversation

@kirich1409
Copy link
Copy Markdown
Contributor

What changed

Restores R8 per-function dead-code elimination for the codegen-emitted is*Enabled() extensions, which was silently degraded by PR #200 (multi-module sample). Two coupled latent bugs are addressed:

  1. ConfigValues got a sync read path (getValueCached) so the codegen-emitted is*Enabled() extensions can be non-suspend again. The internal snapshot is a copy-on-write AtomicReference<Map<String, ConfigValue<*>>>, populated via write-through from override / fetch / suspend getValue. Cold reads return ConfigValue(defaultValue, Source.DEFAULT) — forgiving like Firebase Remote Config.
  2. ProguardRulesGenerator was targeting the legacy @file:JvmName-derived class name (Featured_FlagExtensionsKt), but PR D removed @file:JvmName so the real Kotlin-compiled class is GeneratedFlagExtensions…Kt. The -assumevalues rules silently no-op'd; R8 couldn't apply per-function DCE in real consumer builds. Rule target now derives from ExtensionFunctionGenerator.fileName(modulePath) — single source of truth for the class name.

ExtensionFunctionGenerator stops emitting suspend and switches the underlying call to getValueCached. The JVM signature returns to plain boolean isXEnabled(ConfigValues) (no Continuation), matching what ProguardRulesGenerator emits. SyntheticBytecodeFactory + R8 elimination tests sync to the new class name so the shrinker-tests suite actually exercises the production rule shape.

ConfigValues.isEnabled extension is no longer suspend — delegates to getValueCached. Breaking change; callers can drop runBlocking { … } / coroutine-scope wrappers. Featured does not keep API compatibility.

resetOverride(param) re-resolves through the provider priority chain synchronously (Phase A review caught a race in the initial background-launch implementation). ConfigValues no longer needs an internal coroutine scope or AutoCloseable lifecycle.

Why

PR #200 (multi-module sample) introduced suspend on the generated extensions because the underlying ConfigValues.getValue was suspend. This was tactical — accepted by the maintainer with the understanding that R8 DCE would be silently degraded until a proper sync path landed. This PR closes that gap.

Scope rationale (Phase 2 was dropped)

Initial design proposed a SnapshotConfigValueProvider bulk warm-up so prod providers (DataStore / NSUserDefaults / Firebase) could pre-fill the snapshot at app start. After discussion, the production direction is per-module ConfigValues — each feature module owns a small ConfigValues instance with only its own params, warmed via observe/getValue naturally. Bulk aggregation matters only for the debug UI, which already iterates GeneratedFeaturedRegistry.all lazily. Per-module ConfigValues architecture itself is filed as a separate follow-up (tracker task #16) — out of scope here.

Artifacts

  • swarm-report/proguard-suspend-followup-plan.md — architecture-expert design proposal (accepted with Q4 picked as Option B = re-resolve through priority chain).
  • swarm-report/sync-getvaluecached-state.md — phase tracker with user-confirmed decisions baked in.
  • Per-phase stage reports sync-getvaluecached-stage-1.md, stage-3.md, stage-4-fix.md, stage-5.md.

How to test

CI matrix runs the full Gradle suite. Verified locally:

  • ./gradlew :core:test :core:koverVerify — green, ≥ 90% line coverage
  • ./gradlew -p build-logic :featured-gradle-plugin:check — green
  • ./gradlew :featured-shrinker-tests:test — green (R8 elimination tests now actually exercise the real class name)
  • ./gradlew :sample:android-app:assembleDebug — green
  • ./gradlew :sample:desktop:assemble — green
  • ./gradlew :sample:shared:linkDebugFrameworkIosSimulatorArm64 — green
  • ./gradlew spotlessCheck — green

Manual QA: the sample's runtime path uses observe-bridges (Flow<T> + stateIn), so the existing PR #200 24/24 Pixel 10 + Desktop + iOS Sim runs cover this PR's runtime behavior — no UI surfaces changed.

Release Notes

Added

  • ConfigValues.getValueCached(param) — non-suspend cached reader; returns ConfigValue<T> with Source.DEFAULT before any warm-up write. Safe from any thread including Android main.

Changed

  • Breaking: ConfigValues.isEnabled(param) extension is no longer suspend. Drop runBlocking { … } / coroutine-scope wrappers; the call site can stay in a @Composable or any sync context.
  • Breaking: codegen-emitted is<Name>Enabled() / get<Name>() extensions are no longer suspend. Same migration as above.
  • ConfigValues.override / fetch / suspend getValue write through to the in-memory snapshot so subsequent getValueCached reads see the value with the source the provider reported.
  • ConfigValues.resetOverride re-resolves through the provider priority chain inline before returning.

Fixed

  • R8 per-function DCE for unused flag accessors: -assumevalues rules now target the real Kotlin-compiled class name (GeneratedFlagExtensions…Kt). Previously the rules silently no-op'd after PR Multi-module sample showcasing Featured aggregator plugin #200 removed @file:JvmName. Module paths with hyphens (:feature-checkout) also produced JVM-illegal class names — corrected via the shared ExtensionFunctionGenerator.fileName derivation.

Status

Phase Outcome
Phase 1 (:core sync API) done, 651126b
Phase 2 (bulk SnapshotConfigValueProvider) dropped (per-module is the production pattern)
Phase 3 (codegen non-suspend) done, 6c1208a
Phase 4 (ProGuard class-name fix) done, 2a16bc7
Phase 5 (docs / CHANGELOG / sample) done, 39d7e0c
/finalize Phase A (code-reviewer opus) WARN → fixed 1bf0cbc
/check (build / lint / tests / shrinker) PASS

Follow-ups (not blocking)

🤖 Generated with Claude Code

kirich1409 and others added 5 commits May 20, 2026 10:57
ConfigValues now owns an in-memory snapshot (Map<String, ConfigValue<*>>
held in kotlin.concurrent.AtomicReference, copy-on-write) and exposes
getValueCached(...) — a non-suspend reader that returns the cached
ConfigValue or a DEFAULT-sourced fallback before any warm-up write.
Reads are safe on any thread including the Android main thread.

override(), fetch(), and suspend getValue() all write-through into the
snapshot so subsequent sync reads reflect the latest value with the
Source the provider reported. resetOverride() clears the local override
and dispatches a background coroutine (Dispatchers.Default) that re-
resolves the param through the provider priority chain, refreshing the
snapshot to whatever remote / default value is current. ConfigValues
now owns a SupervisorJob-backed CoroutineScope and implements
AutoCloseable so the scope can be torn down deterministically.

isEnabled extension is no longer suspend — it delegates to
getValueCached so generated is*Enabled() codegen can return to a
non-suspend shape (preparing the path for restoring R8 per-function
DCE in later phases). Callers that previously awaited isEnabled lose
the suspend boundary; Featured does not keep API compatibility.

initialize() snapshot warm-up still depends on providers implementing
the upcoming SnapshotConfigValueProvider — Phase 2 wires that.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ExtensionFunctionGenerator stops emitting suspend on the per-flag
extensions and switches the underlying call from suspend getValue to
the new non-suspend getValueCached (added in the previous commit).
The generated is<Name>Enabled() / get<Name>() helpers can now be
called from any context — including @composable bodies — without
dragging a coroutine scope along.

The JVM method signature returns to plain `boolean
is<Name>Enabled(ConfigValues)` (no Continuation parameter), so the
ProGuard -assumevalues rule shape that ProguardRulesGenerator already
emits matches the real method again and R8 can resume per-function
DCE on dead-flag branches. The shrinker-tests subsystem and the
ProGuard rule format itself need no changes — Phase 4 just removes
the "no-op" caveats from the relevant KDoc.

Generator unit tests pin the new non-suspend / getValueCached shape;
end-to-end sample assemble (Android / Desktop / iOS sim) stays green.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ProguardRulesGenerator derived the target Kotlin file class name via
ExtensionFunctionGenerator.jvmFileName, which reproduced the old
@file:JvmName pattern that PR D removed. The real compiled class is
named after the source file (e.g. GeneratedFlagExtensionsSampleFeatureUiKt),
so R8 silently no-op'd the -assumevalues directive — per-function dead-
code elimination did not actually happen in consumer builds. Module
paths with hyphens additionally produced JVM-illegal identifiers in
the emitted rules, which R8 also ignored without warning.

Switch the rule target to the file-name-derived class (drop jvmFileName,
reuse ExtensionFunctionGenerator.fileName so the sanitization stays in
one place) and update SyntheticBytecodeFactory + the R8 elimination
tests to match. The shrinker-tests suite now exercises R8 against
bytecode whose JVM class name matches what the rules target — the
pipeline is consistent again and the existing R8 DCE assertions are
finally exercising the production shape.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Records the [Unreleased] CHANGELOG entries for the sync ConfigValues
work: new getValueCached reader, non-suspend isEnabled extension,
generated codegen losing suspend, AutoCloseable on ConfigValues, and
the R8 DCE ProGuard rule fix. The sample CLAUDE.md gains a one-
paragraph pointer noting that non-reactive call sites should use
getValueCached or the codegen extensions, while reactive UI keeps the
observe-bridge pattern that already exists in the multi-module sample.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Phase A code review flagged that the backgroundScope.launch in
resetOverride could clobber a concurrent override(p, X) write: the
background re-resolve writes the just-resolved REMOTE value after the
caller's LOCAL=X write lands, and snapshot/provider diverge until the
next getValue. resetOverride is already suspend, so do the re-resolve
inline — the caller pays the same cost and the ordering becomes
deterministic. The explicit writeSnapshot call stays because getValue
intentionally does not write DEFAULT into the snapshot; without the
explicit write a previously overridden slot would stay stale when both
providers return null.

The synchronous re-resolve makes the internal CoroutineScope unused, so
ConfigValues no longer implements AutoCloseable. The CHANGELOG entry is
swapped accordingly. observe()'s KDoc gets a more accurate description
of write-through behaviour, and the concurrent-write test is renamed
to reflect what runTest actually exercises (single-thread coroutine
interleaving, not OS-level parallelism).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@kirich1409 kirich1409 added enhancement New feature or request core Changes to the core module gradle-plugin featured-gradle-plugin work labels May 20, 2026
@kirich1409 kirich1409 marked this pull request as ready for review May 20, 2026 09:05
Copilot AI review requested due to automatic review settings May 20, 2026 09:05
@qodo-code-review
Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@kirich1409 kirich1409 merged commit b2b1220 into develop May 20, 2026
11 checks passed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR restores R8 per-function dead-code elimination for codegen-emitted flag accessors by reintroducing a synchronous cached read path in ConfigValues and fixing the ProGuard rule generator to target the actual Kotlin-compiled extensions class name.

Changes:

  • Add ConfigValues.getValueCached() backed by an in-memory AtomicReference snapshot, and switch ConfigValues.isEnabled() to a non-suspend cached read.
  • Update ExtensionFunctionGenerator to emit non-suspend is*Enabled() / get*() extensions that delegate to getValueCached().
  • Fix ProguardRulesGenerator (and shrinker tests) to derive the correct GeneratedFlagExtensions…Kt JVM class name via ExtensionFunctionGenerator.fileName().

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
sample/CLAUDE.md Updates sample guidance to use getValueCached for sync reads and reflect non-suspend generated accessors.
featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8IntFlagEliminationTest.kt Updates shrinker test documentation to match the new generated extensions class naming.
featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/r8/R8BooleanFlagEliminationTest.kt Updates shrinker test documentation to match the new generated extensions class naming.
featured-shrinker-tests/src/test/kotlin/dev/androidbroadcast/featured/shrinker/bytecode/SyntheticBytecodeFactory.kt Aligns synthetic bytecode internal class names with GeneratedFlagExtensions…Kt output.
core/src/commonTest/kotlin/dev/androidbroadcast/featured/ConfigValuesExtensionsTest.kt Adjusts tests for isEnabled becoming non-suspend and snapshot-driven.
core/src/commonTest/kotlin/dev/androidbroadcast/featured/ConfigValuesCachedTest.kt Adds unit tests for getValueCached behavior and snapshot write-through semantics.
core/src/commonMain/kotlin/dev/androidbroadcast/featured/ConfigValuesExtensions.kt Changes isEnabled extension to be synchronous and snapshot-based with updated KDoc.
core/src/commonMain/kotlin/dev/androidbroadcast/featured/ConfigValues.kt Implements the snapshot + getValueCached, and wires write-through from getValue/override/resetOverride.
CHANGELOG.md Documents the new sync API and the restored R8 DCE behavior.
build-logic/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/ProguardRulesGeneratorTest.kt Updates ProGuard generator tests to expect the new Kotlin file-derived JVM class name.
build-logic/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/FeaturedPluginIntegrationTest.kt Updates integration test expectations for the new extensions class name.
build-logic/featured-gradle-plugin/src/test/kotlin/dev/androidbroadcast/featured/gradle/ExtensionFunctionGeneratorTest.kt Updates generator tests for non-suspend extensions and getValueCached usage.
build-logic/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/ProguardRulesGenerator.kt Fixes -assumevalues rule target class name derivation using fileName() as source of truth.
build-logic/featured-gradle-plugin/src/main/kotlin/dev/androidbroadcast/featured/gradle/ExtensionFunctionGenerator.kt Emits non-suspend extension functions and removes the legacy jvmFileName path.
Comments suppressed due to low confidence (1)

core/src/commonMain/kotlin/dev/androidbroadcast/featured/ConfigValues.kt:235

  • clearOverrides() clears the local provider but intentionally leaves the snapshot unchanged. With the new sync read path (getValueCached / isEnabled), this means previously overridden params may continue to read as Source.LOCAL even after clearing. Consider updating the snapshot here (e.g., dropping entries whose cached source == LOCAL) so cached reads reflect that overrides were cleared.
    /**
     * Removes all locally overridden values, resetting the local provider to an empty state.
     *
     * After this call, every [getValue] call falls back to the remote provider or
     * [ConfigParam.defaultValue]. Has no effect when no local provider is configured.
     *
     * Note: the sync snapshot is **not** cleared here. Individual param slots are updated
     * lazily when [getValue] or [resetOverride] is called for each param. This is consistent
     * with the fact that [ConfigValues] does not maintain a registry of all known params.
     */
    public suspend fun clearOverrides() {
        localProvider?.clear()
    }

@Suppress("HardcodedFlagValue") // intentional: this IS the provider fallback path
return ConfigValue(param.defaultValue, ConfigValue.Source.DEFAULT)
val defaultValue = ConfigValue(param.defaultValue, ConfigValue.Source.DEFAULT)
// Do not write DEFAULT into the snapshot: a later override / fetch should still win.
Comment on lines +62 to +64
* [ConfigParam.defaultValue] before the snapshot is warmed by [ConfigValues.getValue],
* [ConfigValues.fetch], or [ConfigValues.override] — matching Firebase Remote Config's
* "activate-then-read" contract.
Comment thread CHANGELOG.md

### Added

- `ConfigValues.getValueCached(param: ConfigParam<T>): ConfigValue<T>` — non-suspend synchronous reader. Returns the last-written `ConfigValue<T>` from the in-memory cache; the cache is warmed on the first `getValue` / `override` / `fetch` call, and returns `Source.DEFAULT` until then.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Changes to the core module enhancement New feature or request gradle-plugin featured-gradle-plugin work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants