Bound function reference to stable class's method with arguments breaks skipping [302680514]

Fixed

Feature Request

Status Update

No update yet.

Description

el...@reaktor.com

created issue #1

Sep 30, 2023 02:18PM

Jetpack Compose version: 1.6.0-alpha06
Jetpack Compose component(s) used: @Composable
Kotlin version: 1.9.10
Compose compiler version: 1.5.3

Steps to Reproduce or Code Sample to Reproduce:

class MainActivity2 : ComponentActivity() {
    override fun onCreate(savedInstanceState: Bundle?) {
        super.onCreate(savedInstanceState)

        val cl = StableClass()
        setContent {
            Root(cl)
        }
    }
}

@Composable
fun Root(stableClass: StableClass) {
    Log.w("TEST", "Recomposing root")
    var internalCounter by remember { mutableIntStateOf(0) }

    Button(onClick = { internalCounter += 1 }) {
        Text("Increment ($internalCounter)")
    }

    internalCounter // recompose this scope when changed
    Skippable(stableClass::method)
}

var previous: Any? = null

class StableClass {
    fun method(arg: Int) {}
}

@Composable
fun Skippable(functionRef: (Int) -> Unit) {
    Log.w("TEST", "Recomposing while equals=${functionRef == previous}")
    previous = functionRef
}

Results in log output:

 W  Recomposing root
 W  Recomposing while equals=false
 W  Recomposing root
 W  Recomposing while equals=true
 W  Recomposing root
 W  Recomposing while equals=true
 W  Recomposing root
 W  Recomposing while equals=true
[...]

The Android Studio debugger claims that the parameter has changed even though equals returns true.

Composable fun Skippable(): Arguments: Different: ["functionRef"]

If the method signature is changed to not have arguments, skipping occurs normally:
fun method() {}
If a new lambda is created instead of passing the function reference, skipping occurs normally:
Skippable { stableClass.method(it) }

Comments

as...@google.com <as...@google.com> #2Oct 3, 2023 12:59PM

Status: Won't Fix (Intended Behavior)

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

js...@google.com <js...@google.com> #3Oct 3, 2023 03:11PM

I don't feel strongly either way, but if we cared we could probably rewrite function references to behave more like lambdas which preserve stability. It does seem like maybe a valid request/optimization even if a low-priority one?

el...@reaktor.com <el...@reaktor.com> #4Oct 4, 2023 06:55AM

Thanks for the explanation. I didn't realize that using a bound function reference actually creates a new reference on every access, since I assumed .equals() would be using referential equality!

However, there still seems to be a bug.

To my understanding the compiler should generate a remember expression around parameter-less function reference, but it doesn't seem to generate one for function references with more than 0 arguments. I would imagine this breaking it:

        if (expression.valueArgumentsCount != 0) {
            // If this syntax is as a curry syntax in the future, don't memoize.
            // The syntax <expr>::<method>(<params>) and ::<function>(<params>) is reserved for
            // future use. This ensures we don't try to memoize this syntax without knowing
            // its meaning.

IrFunctionReference.valueArgumentsCount seems to currently refer to the parameters of the method being referenced, not the parameters in the calling syntax. I might be misunderstanding as I'm not very familiar with the internals, but this still seems premature at this moment.

For others reading this issue: a workaround is to replace direct bound function references:

- klass::method
+ remember(klass) { klass::method }
// or
+ { arg -> klass.method(arg) }

Can this issue be reopened or should I file a new issue with a more specific description?

as...@google.com <as...@google.com> #5Oct 4, 2023 08:09AM

Assigned to as...@google.com.

I see, this feature request sounds legit to me I think we should be able to remember reference based on receiver types.

el...@reaktor.com <el...@reaktor.com> #6Oct 4, 2023 09:18AM

Regarding #3, #5; based on the compiler source it seems function reference memoization is already implemented in the compiler, it just doesn't work correctly (see #4).

as...@google.com <as...@google.com> #7Oct 4, 2023 09:26AM

It is not quite the case, the line of code you linked prevents memoization in cases if someone does cls::invoke(0)

We'll need to support function references separately, as IR shape for them is different

ap...@google.com <ap...@google.com> #8Nov 13, 2023 05:30PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit c05d2bbe2b44dfead66315174a4d82951fd56b46
Author: Elias Benkhodja <elias.benkhodja@reaktor.com>
Date: Wed Oct 04 20:57:13 2023

Memoize function references with arguments

`ComposerLambdaMemoization` tried to prematurely prevent potential
syntax addition not breaking function reference optimization by checking
for amount of value parameters.

This inadvertently broke memoization of function references to methods
with more than zero arguments, since `IrFunctionReferenceImpl` currently
reflects the amount of value arguments in the referenced function.

Add a check to disable memoization for function references with context
receivers. Kotlin doesn't support them yet, and behavior might change in
the future.

This change also enables memoizing function references without checking
for stability if strong skipping is enabled.

Test: Added new tests to `LambdaMemoizationTransformTests`:
* `testNonComposableFunctionReferenceWithNoArgumentsMemoization`
* `testNonComposableFunctionReferenceWithArgumentsMemoization`
* `testNonComposableFunctionReferenceWithStableContextReceiverNotMemoized`
* `testUnstableReceiverFunctionReferenceNotMemoized`
* `testUnstableExtensionReceiverFunctionReferenceNotMemoized``

Added new tests to `StrongSkippingModeTransformTests`:
* `testUnstableReceiverFunctionReferenceMemoized`
* `testUnstableExtensionReceiverFunctionReferenceMemoized`

Fixes:

b/302680514

Relnote: Memoize stable function references with arguments
Change-Id: I4d7bf3061e60eaa6fd3af60662d6a7b02c1a83b2

M compose/compiler/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/LambdaMemoizationTransformTests.kt
M compose/compiler/compiler-hosted/integration-tests/src/jvmTest/kotlin/androidx/compose/compiler/plugins/kotlin/StrongSkippingModeTransformTests.kt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testUnstableExtensionReceiverFunctionReferenceNotMemoized[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testUnstableExtensionReceiverFunctionReferenceNotMemoized[useFir = true].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testUnstableReceiverFunctionReferenceNotMemoized[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.LambdaMemoizationTransformTests/testUnstableReceiverFunctionReferenceNotMemoized[useFir = true].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.StrongSkippingModeTransformTests/testUnstableExtensionReceiverFunctionReferenceMemoized[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.StrongSkippingModeTransformTests/testUnstableExtensionReceiverFunctionReferenceMemoized[useFir = true].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.StrongSkippingModeTransformTests/testUnstableReceiverFunctionReferenceMemoized[useFir = false].txt
A compose/compiler/compiler-hosted/integration-tests/src/test/resources/golden/androidx.compose.compiler.plugins.kotlin.StrongSkippingModeTransformTests/testUnstableReceiverFunctionReferenceMemoized[useFir = true].txt
M compose/compiler/compiler-hosted/src/main/java/androidx/compose/compiler/plugins/kotlin/lower/ComposerLambdaMemoization.kt

https://android-review.googlesource.com/2822157

pr...@google.com <pr...@google.com> #9Nov 29, 2023 09:39PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.compose.compiler:compiler-hosted:1.5.5