Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onClick lambda will leak refrences. #538

Closed
seyedjafariy opened this issue Mar 26, 2021 · 6 comments
Closed

onClick lambda will leak refrences. #538

seyedjafariy opened this issue Mar 26, 2021 · 6 comments
Assignees
Labels
bug Something isn't working wait for reply Further information is requested

Comments

@seyedjafariy
Copy link

seyedjafariy commented Mar 26, 2021

If any reference is used in the onClick lambdas the reference will be leaked and won't be garbage collected.
that applies to both: Modifier.clickable{} and Button(onClick= {})

here is a small sample to showcase this issue:

@Composable
private fun ScreenAInternal(state: Int, presenter: ScreenAPresenter) {
    Box(contentAlignment = Alignment.Center, modifier = Modifier.background(highlightedAccentColor).fillMaxWidth()) {
        Text("state A= $state", modifier = Modifier.align(Alignment.TopStart).clickable {
            presenter.state.value = presenter.state.value + 1
        })

        val appState = AppStateManager.stateManager.current

        Button({
            appState.currentState = AppScreenState.BScreen
        }) {
            Text("to screen B")
        }

        if (state in 6..10) {
            ScreenAHidden(Modifier.align(Alignment.TopEnd).background(Color.Blue))
        }else if (state > 10){
            ScreenAHidden(Modifier.align(Alignment.TopEnd).background(Color.Red))
        }
    }
}

In here the presenter object will be leak just because it's been referenced in the onClick lambda. there is the object graph which leads to: root Recomposer, AppManager and awt.Window

summary of heap graph by JProfiler: (Jprofiler noob here sorry if I couldn't export it any better way!)

image

image

image

@olonho
Copy link
Contributor

olonho commented Mar 29, 2021

Not sure if this is what typically called "leak". That's how generally value capturing works. If certain object is used in lambda - its reference is held by lambda object. Probably slight rework on state management with more granular state objects may help.

@olonho olonho added question Not a bug, but question or comment wait for reply Further information is requested labels Mar 29, 2021
@seyedjafariy
Copy link
Author

Thanks for your comment @olonho . Passing the reference and holding it in the lambda is ok and is exactly what I want.
But the issue is that this reference will never get garbage collected.

After calling GC manually or even waiting a long time (for automatic GC to happen) all of the other useless objects were removed from the heap except this one.

ScreenAInternal is an "entire screen" @Composable. So if I change my "screen" to another and come back to this, I expect a new Presenter to be created (which it does) but I also expect that the old presenters to be GCed out in the next cycle(worth mentioning the garbage collection works for all other object and only fails for the references that passed to an onClick lambda), this part is not working. hence multiple ScreenAPresenter will be created and held in the heap.
So as I understood from the profiler (ScreenShots above) the problem is not my object it's the onClick lambdas that never get GCed. and they will live throughout the entire app runtime.

@igordmn
Copy link
Collaborator

igordmn commented Mar 30, 2021

Thanks!

I checked this code:

import androidx.compose.desktop.Window
import androidx.compose.foundation.layout.Column
import androidx.compose.material.*
import androidx.compose.runtime.*

fun main() = Window {
    App()
}

@Composable
fun App() {
    var i by remember { mutableStateOf(0) }
    Column {
        Button(onClick = {
            i++
            System.gc()
        }) {
            Text("Next")
        }
        if (i % 2 == 0) {
            View1()
        } else {
            View2()
        }
    }
}

@Composable
fun View1() {
    View(remember { Presenter("View1") })
}

@Composable
fun View2() {
    View(remember { Presenter("View2") })
}

@Composable
fun View(presenter: Presenter) {
    Button(onClick = { presenter.click() }) {
        Text(presenter.text)
    }
}

class Presenter(val text: String) {
    fun click() {
    }
}

And indeed, we have a memory leak. And probably we already have a solution. Will try it

@seyedjafariy
Copy link
Author

@igordmn Thanks for the complete example. I should have pasted my example completely too, sorry.
Is there a public link for the solution?
https://partnerissuetracker.corp.google.com/issues/170869626#comment4
I can not access this

@jimgoog
Copy link
Collaborator

jimgoog commented Mar 30, 2021

@worldsnas Try https://issuetracker.google.com/issues/170869626#comment4

@olonho olonho added bug Something isn't working and removed question Not a bug, but question or comment labels Mar 30, 2021
copybara-service bot pushed a commit to androidx/androidx that referenced this issue Apr 1, 2021
…posed.

Fixes JetBrains/compose-multiplatform#538 and https://partnerissuetracker.corp.google.com/issues/170869626#comment4

As far as I figure out how observation works, it is enough to call `clearInvalidObservations` in the end of the every frame.
Because all changes will be commited before (on recomposition/layout/draw).
Similar code is in https://cs.android.com/androidx/platform/frameworks/support/+/androidx-main:compose/ui/ui/src/androidMain/kotlin/androidx/compose/ui/platform/AndroidComposeView.android.kt;l=378;drc=f3b4f4f28cf61d4434af59d9247a9d2f6d5d70e7.

Bug: 183493610
Test: JetBrains/compose-multiplatform#538 (comment), click "Next" multiple times, run VisualVM, run GC, dump heap, find "Presenter"
Test: ./gradlew jvmTest desktopTest -Pandroidx.compose.multiplatformEnabled=true
Change-Id:  I864f4e7aeca28547465ea468229e19426d7b2827
@olonho
Copy link
Contributor

olonho commented Apr 6, 2021

Shall be fixed with 0.4.0-build179.

@olonho olonho closed this as completed Apr 6, 2021
MatkovIvan added a commit to MatkovIvan/compose-multiplatform that referenced this issue May 10, 2023
JetBrains#538)

* Add test reproducing reported user story. Implement fix. Document edge case bug.

* Update compose/material/material/src/commonMain/kotlin/androidx/compose/material/Slider.kt

Co-authored-by: Ivan Matkov <ivan.matkov@jetbrains.com>

* Update compose/material/material/src/commonMain/kotlin/androidx/compose/material/Slider.kt

Co-authored-by: Ivan Matkov <ivan.matkov@jetbrains.com>

* Revert "Update compose/material/material/src/commonMain/kotlin/androidx/compose/material/Slider.kt"

This reverts commit b3fd5cc7a6b668ecd2ee9ebe7864eb0e5c71b586.

---------

Co-authored-by: Elijah Semyonov <elijah.semyonov@jetbrains.com>
Co-authored-by: Ivan Matkov <ivan.matkov@jetbrains.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wait for reply Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants