patrykandpatrick / vico

A light and extensible chart library for Android.
https://patrykandpatrick.com/vico/wiki
Apache License 2.0
2.08k stars 125 forks source link

Throwing OOM Exception when using Chart in LazyColumn #670

Closed BreakZero closed 3 months ago

BreakZero commented 4 months ago

How to reproduce

I have a LazyColumn with hundreds of items. And using LineChart as below in ItemView

@Composable
internal fun MarketCoinItem(
    modifier: Modifier = Modifier,
    marketCoin: MarketCoin
) {
    val modelProducer = remember { CartesianChartModelProducer.build {
        lineSeries { series(marketCoin.price) }
    } }
   CartesianChartHost(
              rememberCartesianChart(
                  rememberLineCartesianLayer(),
                  startAxis = rememberStartAxis(
                      label = null,
                      axis = null,
                      tick = null,
                      guideline = null
                  ),
                  bottomAxis = rememberBottomAxis(
                      label = null,
                      axis = null,
                      tick = null,
                      guideline = null
                  ),
              ),
              modelProducer,
              horizontalLayout = HorizontalLayout.fullWidth()
          )
}

then scroll down about 20~30 items, Crash wish OOM Exception.

Observed behavior

scroll down with lazy column, by more and more items scroll up, then block and crash with OOM Exception.

Expected behavior

Scroll smoothly.

Vico version(s)

Latest unstable version

Android version(s)

Android 14

Additional information

Hi, maybe i did not explain detail enough, and I will spend more time to figure it out . But I think it is a bug.

Here is some of error messages:

image

BTW, I found if let all Axis keep null as default, it work a little better, So i guess maybe calculate axis logic need improvement.

CartesianChartHost(
                rememberCartesianChart(
                    rememberLineCartesianLayer()
                ),
                modelProducer,
                horizontalLayout = HorizontalLayout.fullWidth()
            )
Gowsky commented 4 months ago

Hi @BreakZero, please provide a MRE. I tried doing the same in the sample app like so:

@Composable
internal fun Chart1(
    uiSystem: UISystem,
    modifier: Modifier,
) {
    LazyColumn(modifier = Modifier.fillMaxSize()) {
        items(200) {
            Column(modifier = Modifier.fillMaxWidth()) {
                Text(
                    text = it.toString(),
                    modifier = Modifier.padding(top = 24.dp, start = 20.dp),
                    fontWeight = FontWeight.Bold
                )

                val modelProducer = remember { CartesianChartModelProducer.build() }
                LaunchedEffect(Unit) {
                    withContext(Dispatchers.Default) {
                        while (isActive) {
                            modelProducer.tryRunTransaction { lineSeries { series(x, x.map { Random.nextFloat() * 15 }) } }
                            delay(Defaults.TRANSACTION_INTERVAL_MS)
                        }
                    }
                }

                when (uiSystem) {
                    UISystem.Compose -> ComposeChart1(modelProducer, modifier)
                    UISystem.Views -> ViewChart1(modelProducer, modifier)
                }
            }
        }
    }
}

It's working fine (see the attached video). Also, I didn't observe any increasing memory usage in the profiler.

https://github.com/patrykandpatrick/vico/assets/14221573/176e14f4-6328-49ef-8809-db7b859e0940

BreakZero commented 4 months ago

Hi @Gowsky , Here is the MRE Looks GC trigger frequently.

image

And when I add modifier for Chart, Chart will gone.

image
BreakZero commented 4 months ago

From memory record, I found it will create a lot of LineCartesianLayerModel.Entry instances when chart attach into view.

patrickmichalik commented 4 months ago

Thanks, @BreakZero! I wanted to start by addressing the issue where the charts aren’t rendered. We faced it in the minimal reproducible example both with and without the Modifier. We confirmed this to be a Vico bug, identified the root cause, and released an update, Vico 2.0.0 Alpha 19. We then updated Vico in the minimal reproducible example, at which point the charts showed up. We recommend that you update Vico in your project.

Regarding the OutOfMemoryError, there seems to be a mismatch between the code snippet that you initially shared and the minimal reproducible example. In the former, a single CartesianChartModelProducer.Transaction is run via CartesianChartModelProducer.build. In the latter, there’s an endless loop that requests identical CartesianChartModelProducer.Transactions repeatedly—see here. This loop appears to be unnecessary. It leads to increased memory usage and is the reason for the large number of LineCartesianLayerModel.Entry instantiations. Could you clarify whether the OutOfMemoryError occurs with both solutions? We haven’t been able to get the minimal reproducible example to crash in either case, but we saw significantly worse performance with the loop than with the solution from the initial code snippet.

BreakZero commented 4 months ago

Hi @patrickmichalik , i bump version to alpha19 and charts aren't rendered issue is confirmed to fix. I updated the minimal reproducible example using data from network same as how i done in my project. right now OutOfMemoryError Exception can reproduce 100 percent. Here is the root cause error message:

image

Hope it can help.

patrickmichalik commented 4 months ago

Cheers, @BreakZero! We’ve been able to reproduce the crash with the updated minimal reproducible example. We’re looking into it. Please note that the empty axes are unnecessary. Rather than setting label, axis, tick, and guideline to null, you can leave startAxis and bottomAxis null, as in the code snippet under “Additional information.” The chart layout will be the same, and with no VerticalAxis, the OutOfMemoryError won’t occur. Of course, we’ll fix the problem regardless, especially since it appears to affect some nonempty VerticalAxis instances too.

patrickmichalik commented 3 months ago

Vico 2.0.0 Alpha 21 addresses this. We plan on making some further enhancements in this area for better performance and improved default behavior, but the OutOfMemoryError should no longer occur. Please keep in mind that it remains preferable to avoid adding redundant Axis instances to CartesianCharts. Cheers!