From 2c4647d3d99cd1809080248fc06eca14a804fc46 Mon Sep 17 00:00:00 2001 From: Patrick Michalik <120058021+patrickmichalik@users.noreply.github.com> Date: Wed, 27 Sep 2023 15:44:07 +0200 Subject: [PATCH] Resolve caching and concurrency issues affecting `ChartEntryModelProducer` and `ComposedChartEntryModelProducer` (WIP) Co-authored-by: Patryk Goworowski --- .../chart/entry/ChartEntryModelExtensions.kt | 61 ++++---- .../core/entry/ChartEntryModelProducer.kt | 109 ++++++------- .../ComposedChartEntryModelProducer.kt | 148 ++++++++++-------- .../vico/views/chart/BaseChartView.kt | 61 +++++--- 4 files changed, 208 insertions(+), 171 deletions(-) diff --git a/vico/compose/src/main/java/com/patrykandpatrick/vico/compose/chart/entry/ChartEntryModelExtensions.kt b/vico/compose/src/main/java/com/patrykandpatrick/vico/compose/chart/entry/ChartEntryModelExtensions.kt index 88a34e292..8c5d00527 100644 --- a/vico/compose/src/main/java/com/patrykandpatrick/vico/compose/chart/entry/ChartEntryModelExtensions.kt +++ b/vico/compose/src/main/java/com/patrykandpatrick/vico/compose/chart/entry/ChartEntryModelExtensions.kt @@ -32,6 +32,7 @@ import com.patrykandpatrick.vico.core.chart.values.ChartValuesManager import com.patrykandpatrick.vico.core.entry.ChartEntryModel import com.patrykandpatrick.vico.core.entry.ChartModelProducer import com.patrykandpatrick.vico.core.entry.diff.MutableDrawingModelStore +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.launch @@ -65,52 +66,54 @@ public fun ChartModelProducer.collectAsState( val scope = rememberCoroutineScope() val isInPreview = LocalInspectionMode.current - var animationJob: Job? = null + var outerAnimationJob: Job? = null + var innerAnimationJob: Job? = null var isAnimationRunning: Boolean? DisposableEffect(chart, producerKey, runInitialAnimation, isInPreview) { - val afterUpdate = { progressModel: (chartKey: Any, progress: Float) -> Unit -> + val afterUpdate: (progressModel: (chartKey: Any, progress: Float) -> Unit) -> Unit = { progressModel -> if (animationSpec != null && !isInPreview && (chartEntryModelState.value.first != null || runInitialAnimation) ) { - isAnimationRunning = false - animationJob = scope.launch { + isAnimationRunning = true + outerAnimationJob = scope.launch { animate( initialValue = Animation.range.start, targetValue = Animation.range.endInclusive, animationSpec = animationSpec, ) { value, _ -> - if (isAnimationRunning == false) { - progressModel(chart, value) + if (isAnimationRunning == true) { + innerAnimationJob = scope.launch(Dispatchers.Default) { progressModel(chart, value) } } } } } else { - progressModel(chart, Animation.range.endInclusive) + scope.launch { progressModel(chart, Animation.range.endInclusive) } } } - registerForUpdates( - key = chart, - cancelAnimation = { - runBlocking { animationJob?.cancelAndJoin() } - isAnimationRunning = true - }, - startAnimation = afterUpdate, - getOldModel = { chartEntryModelState.value.first }, - modelTransformerProvider = modelTransformerProvider, - drawingModelStore = drawingModelStore, - updateChartValues = { model -> - chartValuesManager.resetChartValues() - chart.updateChartValues(chartValuesManager, model, getXStep?.invoke(model)) - chartValuesManager - }, - ) { updatedModel -> - chartEntryModelState.set(updatedModel) - } - onDispose { - unregisterFromUpdates(chart) - animationJob = null - isAnimationRunning = null + scope.launch { + registerForUpdates( + key = chart, + cancelAnimation = { + runBlocking { + outerAnimationJob?.cancelAndJoin() + innerAnimationJob?.cancelAndJoin() + } + isAnimationRunning = false + }, + startAnimation = afterUpdate, + getOldModel = { chartEntryModelState.value.first }, + modelTransformerProvider = modelTransformerProvider, + drawingModelStore = drawingModelStore, + updateChartValues = { model -> + chartValuesManager.resetChartValues() + chart.updateChartValues(chartValuesManager, model, getXStep?.invoke(model)) + chartValuesManager + }, + ) { updatedModel -> + chartEntryModelState.set(updatedModel) + } } + onDispose { unregisterFromUpdates(chart) } } return chartEntryModelState } diff --git a/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/ChartEntryModelProducer.kt b/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/ChartEntryModelProducer.kt index 272ae4b2b..7780e5084 100644 --- a/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/ChartEntryModelProducer.kt +++ b/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/ChartEntryModelProducer.kt @@ -16,42 +16,34 @@ package com.patrykandpatrick.vico.core.entry +import androidx.annotation.WorkerThread import com.patrykandpatrick.vico.core.DEF_THREAD_POOL_SIZE import com.patrykandpatrick.vico.core.chart.Chart import com.patrykandpatrick.vico.core.chart.values.ChartValuesManager import com.patrykandpatrick.vico.core.entry.diff.DrawingModelStore import com.patrykandpatrick.vico.core.entry.diff.MutableDrawingModelStore -import com.patrykandpatrick.vico.core.extension.copy -import com.patrykandpatrick.vico.core.extension.setToAllChildren import java.util.concurrent.Executor import java.util.concurrent.Executors /** * A [ChartModelProducer] implementation that generates [ChartEntryModel] instances. * - * @param entryCollections a two-dimensional list of [ChartEntry] instances used to generate the [ChartEntryModel]. - * @param backgroundExecutor an [Executor] used to generate instances of the [ChartEntryModel] off the main thread. + * @param entryCollections the initial list of series (where each series is a list of [ChartEntry] instances). + * @param backgroundExecutor used to handle data updates. For synchronous behavior, use [Runnable.run] + * (`Runnable::run`). * * @see ChartModelProducer */ public class ChartEntryModelProducer( entryCollections: List>, - backgroundExecutor: Executor = Executors.newFixedThreadPool(DEF_THREAD_POOL_SIZE), + private val backgroundExecutor: Executor = Executors.newFixedThreadPool(DEF_THREAD_POOL_SIZE), ) : ChartModelProducer { + private var series = emptyList>() private var cachedInternalModel: InternalModel? = null - - private var entriesHashCode: Int? = null - + private var runningUpdateCount = 0 private val updateReceivers: HashMap = HashMap() - private val executor: Executor = backgroundExecutor - - /** - * A mutable two-dimensional list of the [ChartEntry] instances used to generate the [ChartEntryModel]. - */ - private val entries: ArrayList> = ArrayList() - public constructor( vararg entryCollections: List, backgroundExecutor: Executor = Executors.newFixedThreadPool(DEF_THREAD_POOL_SIZE), @@ -62,17 +54,19 @@ public class ChartEntryModelProducer( } /** - * Updates the two-dimensional list of [ChartEntry] instances and notifies listeners about the update. + * Requests a data update. If the update is accepted, `true` is returned. If the update is rejected, which occurs + * when there’s already an update in progress, `false` is returned. * * @see entries * @see registerForUpdates */ - public fun setEntries(entries: List>) { - this.entries.setToAllChildren(entries) - entriesHashCode = entries.hashCode() + public fun setEntries(entries: List>): Boolean { + if (runningUpdateCount++ != 0) return false + series = entries.toList() cachedInternalModel = null updateReceivers.values.forEach { updateReceiver -> - executor.execute { + runningUpdateCount++ + backgroundExecutor.execute { updateReceiver.cancelAnimation() updateReceiver.modelTransformer?.prepareForTransformation( oldModel = updateReceiver.getOldModel(), @@ -81,50 +75,53 @@ public class ChartEntryModelProducer( chartValuesManager = updateReceiver.updateChartValues(getModel()), ) updateReceiver.startAnimation(::progressModel) + runningUpdateCount-- } } + runningUpdateCount-- + return true } /** - * Updates the two-dimensional list of [ChartEntry] instances and notifies listeners about the update. + * Requests a data update. If the update is accepted, `true` is returned. If the update is rejected, which occurs + * when there’s already an update in progress, `false` is returned. * * @see entries * @see registerForUpdates */ - public fun setEntries(vararg entries: List) { - setEntries(entries.toList()) - } - - private fun getInternalModel(drawingModelStore: DrawingModelStore = DrawingModelStore.empty): InternalModel { - cachedInternalModel.let { if (it != null) return it } - val xRange = entries.xRange - val yRange = entries.yRange - val aggregateYRange = entries.calculateStackedYRange() - return InternalModel( - entries = entries.copy(), - minX = xRange.start, - maxX = xRange.endInclusive, - minY = yRange.start, - maxY = yRange.endInclusive, - stackedPositiveY = aggregateYRange.endInclusive, - stackedNegativeY = aggregateYRange.start, - xGcd = entries.calculateXGcd(), - id = entriesHashCode ?: entries.hashCode().also { entriesHashCode = it }, - drawingModelStore = drawingModelStore, - ) - } + public fun setEntries(vararg entries: List): Boolean = setEntries(entries.toList()) + + private fun getInternalModel(drawingModelStore: DrawingModelStore = DrawingModelStore.empty) = + cachedInternalModel?.copy(drawingModelStore = drawingModelStore) + ?: run { + val xRange = series.xRange + val yRange = series.yRange + val aggregateYRange = series.calculateStackedYRange() + InternalModel( + entries = series, + minX = xRange.start, + maxX = xRange.endInclusive, + minY = yRange.start, + maxY = yRange.endInclusive, + stackedPositiveY = aggregateYRange.endInclusive, + stackedNegativeY = aggregateYRange.start, + xGcd = series.calculateXGcd(), + id = series.hashCode(), + drawingModelStore = drawingModelStore, + ).also { cachedInternalModel = it } + } override fun getModel(): ChartEntryModel = getInternalModel() + @WorkerThread override fun progressModel(key: Any, progress: Float) { with(updateReceivers[key] ?: return) { - executor.execute { - modelTransformer?.transform(drawingModelStore, progress) - onModelCreated(getInternalModel(drawingModelStore.copy())) - } + modelTransformer?.transform(drawingModelStore, progress) + onModelCreated(getInternalModel(drawingModelStore.copy())) } } + @WorkerThread override fun registerForUpdates( key: Any, cancelAnimation: () -> Unit, @@ -145,16 +142,14 @@ public class ChartEntryModelProducer( getOldModel, updateChartValues, ) - executor.execute { - cancelAnimation() - modelTransformer?.prepareForTransformation( - oldModel = getOldModel(), - newModel = getModel(), - drawingModelStore = drawingModelStore, - chartValuesManager = updateChartValues(getModel()), - ) - startAnimation(::progressModel) - } + cancelAnimation() + modelTransformer?.prepareForTransformation( + oldModel = getOldModel(), + newModel = getModel(), + drawingModelStore = drawingModelStore, + chartValuesManager = updateChartValues(getModel()), + ) + startAnimation(::progressModel) } override fun unregisterFromUpdates(key: Any) { diff --git a/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/composed/ComposedChartEntryModelProducer.kt b/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/composed/ComposedChartEntryModelProducer.kt index afe516f3d..92f40e62c 100644 --- a/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/composed/ComposedChartEntryModelProducer.kt +++ b/vico/core/src/main/java/com/patrykandpatrick/vico/core/entry/composed/ComposedChartEntryModelProducer.kt @@ -16,6 +16,7 @@ package com.patrykandpatrick.vico.core.entry.composed +import androidx.annotation.WorkerThread import com.patrykandpatrick.vico.core.DEF_THREAD_POOL_SIZE import com.patrykandpatrick.vico.core.chart.Chart import com.patrykandpatrick.vico.core.chart.composed.ComposedChartEntryModel @@ -44,14 +45,17 @@ public class ComposedChartEntryModelProducer private constructor( private val backgroundExecutor: Executor = Executors.newFixedThreadPool(DEF_THREAD_POOL_SIZE), ) : ChartModelProducer> { - private var dataSets = mutableListOf>>() - private var cachedInternalModel: InternalModel? = null + private var dataSets = emptyList>>() + private var cachedInternalComposedModel: InternalComposedModel? = null + private var runningUpdateCount = 0 private val updateReceivers = mutableMapOf() - private fun setDataSets(entries: List>>) { - this.dataSets.setAll(entries) - cachedInternalModel = null + private fun setDataSets(dataSets: List>>): Boolean { + if (runningUpdateCount++ != 0) return false + this.dataSets = dataSets.toList() + cachedInternalComposedModel = null updateReceivers.values.forEach { updateReceiver -> + runningUpdateCount++ backgroundExecutor.execute { updateReceiver.cancelAnimation() updateReceiver.modelTransformer?.prepareForTransformation( @@ -61,56 +65,67 @@ public class ComposedChartEntryModelProducer private constructor( chartValuesManager = updateReceiver.getChartValuesManager(getModel()), ) updateReceiver.startAnimation(::progressModel) + runningUpdateCount-- } } + runningUpdateCount-- + return true } - private fun getInternalModel(drawingModelStore: DrawingModelStore = DrawingModelStore.empty): InternalModel { - cachedInternalModel.let { if (it != null) return it } - val models = dataSets.map { dataSet -> - val xRange = dataSet.xRange - val yRange = dataSet.yRange - val aggregateYRange = dataSet.calculateStackedYRange() - object : ChartEntryModel { - override val entries: List> = dataSet - override val minX: Float = xRange.start - override val maxX: Float = xRange.endInclusive - override val minY: Float = yRange.start - override val maxY: Float = yRange.endInclusive - override val stackedPositiveY: Float = aggregateYRange.endInclusive - override val stackedNegativeY: Float = aggregateYRange.start - override val xGcd: Float = dataSet.calculateXGcd() - override val drawingModelStore: DrawingModelStore = drawingModelStore + private fun getInternalModel(drawingModelStore: DrawingModelStore = DrawingModelStore.empty) = + cachedInternalComposedModel + ?.let { composedModel -> + composedModel.copy( + composedEntryCollections = composedModel.composedEntryCollections + .map { model -> model.copy(drawingModelStore = drawingModelStore) }, + drawingModelStore = drawingModelStore, + ) + } + ?: run { + val models = dataSets.map { dataSet -> + val xRange = dataSet.xRange + val yRange = dataSet.yRange + val aggregateYRange = dataSet.calculateStackedYRange() + InternalModel( + entries = dataSet, + minX = xRange.start, + maxX = xRange.endInclusive, + minY = yRange.start, + maxY = yRange.endInclusive, + stackedPositiveY = aggregateYRange.endInclusive, + stackedNegativeY = aggregateYRange.start, + xGcd = dataSet.calculateXGcd(), + drawingModelStore = drawingModelStore, + ) + } + InternalComposedModel( + composedEntryCollections = models, + entries = models.map { it.entries }.flatten(), + minX = models.minOf { it.minX }, + maxX = models.maxOf { it.maxX }, + minY = models.minOf { it.minY }, + maxY = models.maxOf { it.maxY }, + stackedPositiveY = models.maxOf { it.stackedPositiveY }, + stackedNegativeY = models.minOf { it.stackedNegativeY }, + xGcd = models.fold(null) { gcd, model -> + gcd?.gcdWith(model.xGcd) ?: model.xGcd + } ?: 1f, + id = models.map { it.id }.hashCode(), + drawingModelStore = drawingModelStore, + ).also { cachedInternalComposedModel = it } } - } - return InternalModel( - composedEntryCollections = models, - entries = models.map { it.entries }.flatten(), - minX = models.minOf { it.minX }, - maxX = models.maxOf { it.maxX }, - minY = models.minOf { it.minY }, - maxY = models.maxOf { it.maxY }, - stackedPositiveY = models.maxOf { it.stackedPositiveY }, - stackedNegativeY = models.minOf { it.stackedNegativeY }, - xGcd = models.fold(null) { gcd, model -> - gcd?.gcdWith(model.xGcd) ?: model.xGcd - } ?: 1f, - id = models.map { it.id }.hashCode(), - drawingModelStore = drawingModelStore, - ) - } override fun getModel(): ComposedChartEntryModel = getInternalModel() + @WorkerThread override fun progressModel(key: Any, progress: Float) { with(updateReceivers[key] ?: return) { - backgroundExecutor.execute { - modelTransformer?.transform(drawingModelStore, progress) - onModelCreated(getInternalModel(drawingModelStore.copy())) - } + modelTransformer?.transform(drawingModelStore, progress) + onModelCreated(getInternalModel(drawingModelStore.copy())) } } + @WorkerThread override fun registerForUpdates( key: Any, cancelAnimation: () -> Unit, @@ -131,16 +146,14 @@ public class ComposedChartEntryModelProducer private constructor( getOldModel, updateChartValues, ) - backgroundExecutor.execute { - cancelAnimation() - modelTransformer?.prepareForTransformation( - oldModel = getOldModel(), - newModel = getModel(), - drawingModelStore = drawingModelStore, - chartValuesManager = updateChartValues(getModel()), - ) - startAnimation(::progressModel) - } + cancelAnimation() + modelTransformer?.prepareForTransformation( + oldModel = getOldModel(), + newModel = getModel(), + drawingModelStore = drawingModelStore, + chartValuesManager = updateChartValues(getModel()), + ) + startAnimation(::progressModel) } override fun isRegistered(key: Any): Boolean = updateReceivers.containsKey(key) @@ -155,11 +168,9 @@ public class ComposedChartEntryModelProducer private constructor( public fun createTransaction(): Transaction = Transaction() /** - * Creates a [Transaction], runs [block], and calls [Transaction.commit]. + * Creates a [Transaction], runs [block], and calls [Transaction.commit], returning its output. */ - public fun runTransaction(block: Transaction.() -> Unit) { - createTransaction().also(block).commit() - } + public fun runTransaction(block: Transaction.() -> Unit): Boolean = createTransaction().also(block).commit() /** * Handles data updates. An initially empty list of data sets is created and can be updated via the class’s @@ -225,11 +236,10 @@ public class ComposedChartEntryModelProducer private constructor( } /** - * Finalizes the data update. + * Requests a data update. If the update is accepted, `true` is returned. If the update is rejected, which + * occurs when there’s already an update in progress, `false` is returned. */ - public fun commit() { - setDataSets(newEntries) - } + public fun commit(): Boolean = setDataSets(newEntries) } private data class UpdateReceiver( @@ -243,7 +253,19 @@ public class ComposedChartEntryModelProducer private constructor( ) private data class InternalModel( - override val composedEntryCollections: List, + override val entries: List>, + override val minX: Float, + override val maxX: Float, + override val minY: Float, + override val maxY: Float, + override val stackedPositiveY: Float, + override val stackedNegativeY: Float, + override val xGcd: Float, + override val drawingModelStore: DrawingModelStore, + ) : ChartEntryModel + + private data class InternalComposedModel( + override val composedEntryCollections: List, override val entries: List>, override val minX: Float, override val maxX: Float, @@ -253,13 +275,13 @@ public class ComposedChartEntryModelProducer private constructor( override val stackedNegativeY: Float, override val xGcd: Float, override val id: Int, - override val drawingModelStore: DrawingModelStore = DrawingModelStore.empty, + override val drawingModelStore: DrawingModelStore, ) : ComposedChartEntryModel public companion object { /** * Creates a [ComposedChartEntryModelProducer], running an initial [Transaction]. [backgroundExecutor] is used - * to run calculations off the main thread. + * used to handle data updates. For synchronous behavior, use [Runnable.run] (`Runnable::run`). */ public fun build( backgroundExecutor: Executor = Executors.newFixedThreadPool(DEF_THREAD_POOL_SIZE), diff --git a/vico/views/src/main/java/com/patrykandpatrick/vico/views/chart/BaseChartView.kt b/vico/views/src/main/java/com/patrykandpatrick/vico/views/chart/BaseChartView.kt index 860a89170..1743411cb 100644 --- a/vico/views/src/main/java/com/patrykandpatrick/vico/views/chart/BaseChartView.kt +++ b/vico/views/src/main/java/com/patrykandpatrick/vico/views/chart/BaseChartView.kt @@ -76,6 +76,11 @@ import com.patrykandpatrick.vico.views.scroll.copy import com.patrykandpatrick.vico.views.theme.ThemeHandler import kotlin.properties.Delegates.observable import kotlin.properties.ReadWriteProperty +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.cancel +import kotlinx.coroutines.launch /** * The base for [View]s that display a chart. Subclasses define a [Model] implementation they can handle. @@ -139,6 +144,10 @@ public abstract class BaseChartView internal constructo private val drawingModelStore = MutableDrawingModelStore() + private var coroutineScope: CoroutineScope? = null + + private var animationJob: Job? = null + private var markerTouchPoint: Point? = null private var wasMarkerVisible: Boolean = false @@ -243,39 +252,47 @@ public abstract class BaseChartView internal constructo } private fun registerForUpdates() { - entryProducer?.registerForUpdates( - key = this, - cancelAnimation = { handler.post(animator::cancel) }, - startAnimation = { - if (model != null || runInitialAnimation) { - handler.post(animator::start) - } else { - progressModelOnAnimationProgress(progress = Animation.range.endInclusive) + coroutineScope?.launch { + entryProducer?.registerForUpdates( + key = this@BaseChartView, + cancelAnimation = { + handler.post(animator::cancel) + animationJob?.cancel() + }, + startAnimation = { + if (model != null || runInitialAnimation) { + handler.post(animator::start) + } else { + progressModelOnAnimationProgress(progress = Animation.range.endInclusive) + } + }, + getOldModel = { model }, + modelTransformerProvider = chart?.modelTransformerProvider, + drawingModelStore = drawingModelStore, + updateChartValues = { model -> + chartValuesManager.resetChartValues() + chart?.updateChartValues(chartValuesManager, model, getXStep?.invoke(model)) + chartValuesManager + }, + ) { model -> + post { + setModel(model = model, updateChartValues = false) + postInvalidateOnAnimation() } - }, - getOldModel = { model }, - modelTransformerProvider = chart?.modelTransformerProvider, - drawingModelStore = drawingModelStore, - updateChartValues = { model -> - chartValuesManager.resetChartValues() - chart?.updateChartValues(chartValuesManager, model, getXStep?.invoke(model)) - chartValuesManager - }, - ) { model -> - post { - setModel(model = model, updateChartValues = false) - postInvalidateOnAnimation() } } } override fun onAttachedToWindow() { super.onAttachedToWindow() + coroutineScope = CoroutineScope(Dispatchers.Default) if (entryProducer?.isRegistered(key = this) != true) registerForUpdates() } override fun onDetachedFromWindow() { super.onDetachedFromWindow() + coroutineScope?.cancel() + coroutineScope = null entryProducer?.unregisterFromUpdates(key = this) } @@ -467,7 +484,7 @@ public abstract class BaseChartView internal constructo } private fun progressModelOnAnimationProgress(progress: Float) { - entryProducer?.progressModel(this, progress) + animationJob = coroutineScope?.launch { entryProducer?.progressModel(this@BaseChartView, progress) } } override fun onMeasure(widthMeasureSpec: Int, heightMeasureSpec: Int) {