From 82e54ff3e1244368572484e80eafb31d2724f1c8 Mon Sep 17 00:00:00 2001 From: Jake Wharton Date: Wed, 3 Apr 2024 16:26:49 -0400 Subject: [PATCH] Synthesize depth-first removes for old hosts This works around a since-fixed memory leak. --- .../api/android/redwood-protocol-guest.api | 6 +-- .../api/jvm/redwood-protocol-guest.api | 6 +-- .../redwood/protocol/guest/ProtocolState.kt | 12 ++++- .../redwood/protocol/guest/ProtocolWidget.kt | 36 +++++++++++++- .../protocol/guest/ProtocolWidgetChildren.kt | 43 ++++++++++++---- .../redwood/protocol/guest/ProtocolTest.kt | 49 +++++++++++++------ redwood-protocol/api/redwood-protocol.api | 1 + .../app/cash/redwood/protocol/protocol.kt | 13 ++--- .../app/cash/redwood/protocol/ProtocolTest.kt | 10 ++-- .../codegen/protocolGuestGeneration.kt | 23 ++++++--- 10 files changed, 144 insertions(+), 55 deletions(-) diff --git a/redwood-protocol-guest/api/android/redwood-protocol-guest.api b/redwood-protocol-guest/api/android/redwood-protocol-guest.api index 87d85039a0..fdecb1a94e 100644 --- a/redwood-protocol-guest/api/android/redwood-protocol-guest.api +++ b/redwood-protocol-guest/api/android/redwood-protocol-guest.api @@ -25,7 +25,7 @@ public final class app/cash/redwood/protocol/guest/ProtocolRedwoodCompositionKt public final class app/cash/redwood/protocol/guest/ProtocolState { public static final field $stable I - public fun ()V + public synthetic fun (Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun addWidget (Lapp/cash/redwood/protocol/guest/ProtocolWidget;)V public final fun append (Lapp/cash/redwood/protocol/Change;)V public final fun getChangesOrNull ()Ljava/util/List; @@ -35,23 +35,23 @@ public final class app/cash/redwood/protocol/guest/ProtocolState { } public abstract interface class app/cash/redwood/protocol/guest/ProtocolWidget : app/cash/redwood/widget/Widget { + public abstract fun depthFirstWalk (Lkotlin/jvm/functions/Function3;)V public abstract fun getId-0HhLjSo ()I public abstract fun getTag-BlhN7y0 ()I public synthetic fun getValue ()Ljava/lang/Object; public fun getValue ()Lkotlin/Unit; public abstract fun sendEvent (Lapp/cash/redwood/protocol/Event;)V - public abstract fun visitIds (Lkotlin/jvm/functions/Function1;)V } public final class app/cash/redwood/protocol/guest/ProtocolWidgetChildren : app/cash/redwood/widget/Widget$Children { public static final field $stable I public synthetic fun (IILapp/cash/redwood/protocol/guest/ProtocolState;Lkotlin/jvm/internal/DefaultConstructorMarker;)V + public final fun depthFirstWalk (Lapp/cash/redwood/protocol/guest/ProtocolWidget;Lkotlin/jvm/functions/Function3;)V public fun getWidgets ()Ljava/util/List; public fun insert (ILapp/cash/redwood/widget/Widget;)V public fun move (III)V public fun onModifierUpdated (ILapp/cash/redwood/widget/Widget;)V public fun remove (II)V - public final fun visitIds (Lkotlin/jvm/functions/Function1;)V } public final class app/cash/redwood/protocol/guest/VersionKt { diff --git a/redwood-protocol-guest/api/jvm/redwood-protocol-guest.api b/redwood-protocol-guest/api/jvm/redwood-protocol-guest.api index 87d85039a0..fdecb1a94e 100644 --- a/redwood-protocol-guest/api/jvm/redwood-protocol-guest.api +++ b/redwood-protocol-guest/api/jvm/redwood-protocol-guest.api @@ -25,7 +25,7 @@ public final class app/cash/redwood/protocol/guest/ProtocolRedwoodCompositionKt public final class app/cash/redwood/protocol/guest/ProtocolState { public static final field $stable I - public fun ()V + public synthetic fun (Ljava/lang/String;Lkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun addWidget (Lapp/cash/redwood/protocol/guest/ProtocolWidget;)V public final fun append (Lapp/cash/redwood/protocol/Change;)V public final fun getChangesOrNull ()Ljava/util/List; @@ -35,23 +35,23 @@ public final class app/cash/redwood/protocol/guest/ProtocolState { } public abstract interface class app/cash/redwood/protocol/guest/ProtocolWidget : app/cash/redwood/widget/Widget { + public abstract fun depthFirstWalk (Lkotlin/jvm/functions/Function3;)V public abstract fun getId-0HhLjSo ()I public abstract fun getTag-BlhN7y0 ()I public synthetic fun getValue ()Ljava/lang/Object; public fun getValue ()Lkotlin/Unit; public abstract fun sendEvent (Lapp/cash/redwood/protocol/Event;)V - public abstract fun visitIds (Lkotlin/jvm/functions/Function1;)V } public final class app/cash/redwood/protocol/guest/ProtocolWidgetChildren : app/cash/redwood/widget/Widget$Children { public static final field $stable I public synthetic fun (IILapp/cash/redwood/protocol/guest/ProtocolState;Lkotlin/jvm/internal/DefaultConstructorMarker;)V + public final fun depthFirstWalk (Lapp/cash/redwood/protocol/guest/ProtocolWidget;Lkotlin/jvm/functions/Function3;)V public fun getWidgets ()Ljava/util/List; public fun insert (ILapp/cash/redwood/widget/Widget;)V public fun move (III)V public fun onModifierUpdated (ILapp/cash/redwood/widget/Widget;)V public fun remove (II)V - public final fun visitIds (Lkotlin/jvm/functions/Function1;)V } public final class app/cash/redwood/protocol/guest/VersionKt { diff --git a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolState.kt b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolState.kt index 0bc1880dff..12dca7ac13 100644 --- a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolState.kt +++ b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolState.kt @@ -18,14 +18,24 @@ package app.cash.redwood.protocol.guest import app.cash.redwood.RedwoodCodegenApi import app.cash.redwood.protocol.Change import app.cash.redwood.protocol.Id +import app.cash.redwood.protocol.RedwoodVersion /** @suppress For generated code use only. */ @RedwoodCodegenApi -public class ProtocolState { +public class ProtocolState( + hostVersion: RedwoodVersion, +) { private var nextValue = Id.Root.value + 1 private val widgets = PlatformMap() private var changes = PlatformList() + /** + * Host versions prior to 0.10.0 contained a bug where they did not recursively remove widgets + * from the protocol map which leaked any child views of a removed node. We can work around this + * on the guest side by synthesizing removes for every node in the subtree. + */ + internal val synthesizeSubtreeRemoval = hostVersion < RedwoodVersion("0.10.0-SNAPSHOT") + public fun nextId(): Id { val value = nextValue nextValue = value + 1 diff --git a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidget.kt b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidget.kt index 16b8b8afd9..eae496ceb5 100644 --- a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidget.kt +++ b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidget.kt @@ -16,6 +16,7 @@ package app.cash.redwood.protocol.guest import app.cash.redwood.RedwoodCodegenApi +import app.cash.redwood.protocol.ChildrenTag import app.cash.redwood.protocol.Event import app.cash.redwood.protocol.Id import app.cash.redwood.protocol.WidgetTag @@ -36,6 +37,37 @@ public interface ProtocolWidget : Widget { public fun sendEvent(event: Event) - /** Recursively visit IDs in this widget's tree, starting with this widget's [id]. */ - public fun visitIds(block: (Id) -> Unit) + /** + * Perform a depth-first walk of this widget's children hierarchy. + * + * For example, given the hierarchy: + * ```kotlin + * Split( + * left = { + * Row { + * Text(..) + * Button(..) + * } + * }, + * right = { + * Column { + * Button(..) + * Text(..) + * } + * } + * } + * ``` + * You will see the following argument values passed to [block] if invoked on the `Split`: + * 1. parent: `Row`, childrenTag: 1, children: `[Text+Button]` + * 2. parent: `Split`, childrenTag: 1, children: `[Row]` + * 3. parent: `Column`, childrenTag: 1, children: `[Button+Text]` + * 4. parent: `Split`, childrenTag: 2, children: `[Column]` + */ + public fun depthFirstWalk( + block: ( + parent: ProtocolWidget, + childrenTag: ChildrenTag, + children: ProtocolWidgetChildren, + ) -> Unit, + ) } diff --git a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt index dabcab014f..6926f77fc0 100644 --- a/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt +++ b/redwood-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt @@ -39,15 +39,36 @@ public class ProtocolWidgetChildren( } override fun remove(index: Int, count: Int) { - val removedIds = ArrayList(count) - for (i in index until index + count) { - val widget = _widgets[i] - widget.visitIds(state::removeWidget) - removedIds += widget.id + if (state.synthesizeSubtreeRemoval) { + val removedIds = ArrayList(count) + for (i in index until index + count) { + val widget = _widgets[i] + removedIds += widget.id + state.removeWidget(widget.id) + + widget.depthFirstWalk { parent, childrenTag, children -> + val childIds = children.widgets.map(ProtocolWidget::id) + for (childId in childIds) { + state.removeWidget(childId) + } + state.append(ChildrenChange.Remove(parent.id, childrenTag, 0, childIds.size, childIds)) + } + } + state.append(ChildrenChange.Remove(id, tag, index, count, removedIds)) + } else { + for (i in index until index + count) { + val widget = _widgets[i] + state.removeWidget(widget.id) + widget.depthFirstWalk { _, _, children -> + for (childWidget in children.widgets) { + state.removeWidget(childWidget.id) + } + } + } + state.append(ChildrenChange.Remove(id, tag, index, count)) } _widgets.remove(index, count) - state.append(ChildrenChange.Remove(id, tag, index, count, removedIds)) } override fun move(fromIndex: Int, toIndex: Int, count: Int) { @@ -58,9 +79,13 @@ public class ProtocolWidgetChildren( override fun onModifierUpdated(index: Int, widget: Widget) { } - public fun visitIds(block: (Id) -> Unit) { - for (i in _widgets.indices) { - _widgets[i].visitIds(block) + public fun depthFirstWalk( + parent: ProtocolWidget, + block: (ProtocolWidget, ChildrenTag, ProtocolWidgetChildren) -> Unit, + ) { + for (widget in widgets) { + widget.depthFirstWalk(block) } + block(parent, tag, this) } } diff --git a/redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt b/redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt index 55df7fc2f8..5090deb093 100644 --- a/redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt +++ b/redwood-protocol-guest/src/commonTest/kotlin/app/cash/redwood/protocol/guest/ProtocolTest.kt @@ -32,6 +32,7 @@ import app.cash.redwood.protocol.Id import app.cash.redwood.protocol.ModifierChange import app.cash.redwood.protocol.PropertyChange import app.cash.redwood.protocol.PropertyTag +import app.cash.redwood.protocol.RedwoodVersion import app.cash.redwood.protocol.WidgetTag import app.cash.redwood.testing.TestRedwoodComposition import app.cash.redwood.ui.Cancellable @@ -40,6 +41,7 @@ import app.cash.redwood.ui.OnBackPressedDispatcher import app.cash.redwood.ui.UiConfiguration import assertk.assertFailure import assertk.assertThat +import assertk.assertions.containsExactly import assertk.assertions.isEqualTo import assertk.assertions.isInstanceOf import assertk.assertions.message @@ -58,15 +60,13 @@ import kotlinx.coroutines.test.runTest import kotlinx.serialization.json.JsonPrimitive class ProtocolTest { - private val bridge = TestSchemaProtocolBridge.create( - // Use latest guest version as the host version to avoid any compatibility behavior. - hostVersion = guestRedwoodVersion, - ) + // Use latest guest version as the host version to avoid any compatibility behavior. + private val latestVersion = guestRedwoodVersion @Test fun widgetVersionPropagated() = runTest { val composition = ProtocolRedwoodComposition( scope = this + BroadcastFrameClock(), - bridge = bridge, + bridge = TestSchemaProtocolBridge.create(latestVersion), changesSink = ::error, widgetVersion = 22U, onBackPressedDispatcher = object : OnBackPressedDispatcher { @@ -90,7 +90,7 @@ class ProtocolTest { } @Test fun protocolChangeOrder() = runTest { - val composition = testProtocolComposition() + val (composition) = testProtocolComposition() composition.setContent { TestRow { @@ -128,7 +128,7 @@ class ProtocolTest { } @Test fun protocolAlwaysSendsInitialLambdaPresence() = runTest { - val composition = testProtocolComposition() + val (composition) = testProtocolComposition() composition.setContent { Button("hi", onClick = null) Button("hi", onClick = {}) @@ -166,7 +166,7 @@ class ProtocolTest { } @Test fun protocolSkipsNullableLambdaChangeOfSamePresence() = runTest { - val composition = testProtocolComposition() + val (composition, bridge) = testProtocolComposition() var state by mutableStateOf(0) composition.setContent { @@ -241,7 +241,7 @@ class ProtocolTest { } @Test fun protocolSkipsNonNullLambdaChange() = runTest { - val composition = testProtocolComposition() + val (composition, bridge) = testProtocolComposition() var state by mutableStateOf(0) composition.setContent { @@ -285,8 +285,24 @@ class ProtocolTest { ) } - @Test fun entireSubtreeRemoved() = runTest { - val composition = testProtocolComposition() + @Test fun entireSubtreeRemovedLatest() = runTest { + assertThat(removeSubtree(latestVersion)) + .containsExactly( + ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1), + ) + } + + @Test fun entireSubtreeRemovedOldHostSynthesizesDepthFirstRemoval() = runTest { + assertThat(removeSubtree(RedwoodVersion("0.9.0"))) + .containsExactly( + ChildrenChange.Remove(Id(2), ChildrenTag(1), 0, 1, listOf(Id(3))), + ChildrenChange.Remove(Id(1), ChildrenTag(1), 0, 1, listOf(Id(2))), + ChildrenChange.Remove(Id.Root, ChildrenTag.Root, 0, 1, listOf(Id(1))), + ) + } + + private suspend fun TestScope.removeSubtree(hostVersion: RedwoodVersion): List { + val (composition, bridge) = testProtocolComposition(hostVersion) var clicks = 0 var remove by mutableStateOf(false) @@ -307,7 +323,7 @@ class ProtocolTest { assertThat(clicks).isEqualTo(1) remove = true - composition.awaitSnapshot() + val removeChanges = composition.awaitSnapshot() // If the whole tree was removed, we cannot target the button anymore. assertFailure { bridge.sendEvent(Event(Id(3), EventTag(2))) } @@ -315,9 +331,14 @@ class ProtocolTest { .message() .isEqualTo("Unknown node ID 3 for event with tag 2") assertThat(clicks).isEqualTo(1) + + return removeChanges } - private fun TestScope.testProtocolComposition(): TestRedwoodComposition> { + private fun TestScope.testProtocolComposition( + hostVersion: RedwoodVersion = latestVersion, + ): Pair>, ProtocolBridge> { + val bridge = TestSchemaProtocolBridge.create(hostVersion) val composition = TestRedwoodComposition( scope = backgroundScope, widgetSystem = bridge.widgetSystem, @@ -328,6 +349,6 @@ class ProtocolTest { backgroundScope.coroutineContext.job.invokeOnCompletion { composition.cancel() } - return composition + return composition to bridge } } diff --git a/redwood-protocol/api/redwood-protocol.api b/redwood-protocol/api/redwood-protocol.api index 2b1858c638..8b911fe38d 100644 --- a/redwood-protocol/api/redwood-protocol.api +++ b/redwood-protocol/api/redwood-protocol.api @@ -77,6 +77,7 @@ public final class app/cash/redwood/protocol/ChildrenChange$Move$Companion { public final class app/cash/redwood/protocol/ChildrenChange$Remove : app/cash/redwood/protocol/ChildrenChange { public static final field Companion Lapp/cash/redwood/protocol/ChildrenChange$Remove$Companion; + public synthetic fun (IIIILjava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public synthetic fun (IIIILjava/util/List;Lkotlin/jvm/internal/DefaultConstructorMarker;)V public fun equals (Ljava/lang/Object;)Z public final fun getCount ()I diff --git a/redwood-protocol/src/commonMain/kotlin/app/cash/redwood/protocol/protocol.kt b/redwood-protocol/src/commonMain/kotlin/app/cash/redwood/protocol/protocol.kt index ed57d7c873..062ba276ad 100644 --- a/redwood-protocol/src/commonMain/kotlin/app/cash/redwood/protocol/protocol.kt +++ b/redwood-protocol/src/commonMain/kotlin/app/cash/redwood/protocol/protocol.kt @@ -16,6 +16,7 @@ package app.cash.redwood.protocol import dev.drewhamilton.poko.Poko +import kotlin.DeprecationLevel.ERROR import kotlinx.serialization.KSerializer import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable @@ -151,12 +152,8 @@ public sealed interface ChildrenChange : Change { override val tag: ChildrenTag, public val index: Int, public val count: Int, - public val removedIds: List, - ) : ChildrenChange { - init { - require(count == removedIds.size) { - "Count $count != Removed ID list size ${removedIds.size}" - } - } - } + // TODO Remove this for Redwood 1.0.0. + @Deprecated("Only sent for compatibility with old hosts. Do not consume.", level = ERROR) + public val removedIds: List = emptyList(), + ) : ChildrenChange } diff --git a/redwood-protocol/src/commonTest/kotlin/app/cash/redwood/protocol/ProtocolTest.kt b/redwood-protocol/src/commonTest/kotlin/app/cash/redwood/protocol/ProtocolTest.kt index 878ac4e4db..120a9e5282 100644 --- a/redwood-protocol/src/commonTest/kotlin/app/cash/redwood/protocol/ProtocolTest.kt +++ b/redwood-protocol/src/commonTest/kotlin/app/cash/redwood/protocol/ProtocolTest.kt @@ -57,6 +57,8 @@ class ProtocolTest { Create(Id(1), WidgetTag(2)), ChildrenChange.Add(Id(1), ChildrenTag(2), Id(3), 4), ChildrenChange.Move(Id(1), ChildrenTag(2), 3, 4, 5), + ChildrenChange.Remove(Id(4), ChildrenTag(3), 2, 1), + // We send a list of removed IDs only for old hosts. ChildrenChange.Remove(Id(1), ChildrenTag(2), 3, 4, listOf(Id(5), Id(6), Id(7), Id(8))), ModifierChange( Id(1), @@ -90,6 +92,7 @@ class ProtocolTest { """["create",{"id":1,"tag":2}],""" + """["add",{"id":1,"tag":2,"childId":3,"index":4}],""" + """["move",{"id":1,"tag":2,"fromIndex":3,"toIndex":4,"count":5}],""" + + """["remove",{"id":4,"tag":3,"index":2,"count":1}],""" + """["remove",{"id":1,"tag":2,"index":3,"count":4,"removedIds":[5,6,7,8]}],""" + """["modifier",{"id":1,"elements":[[1,{}],[2,3],[3,[]],[4],[5]]}],""" + """["property",{"id":1,"tag":2,"value":"hello"}],""" + @@ -98,13 +101,6 @@ class ProtocolTest { assertJsonRoundtrip(ListSerializer(Change.serializer()), changes, json) } - @Test fun removeCountMustMatchListSize() { - val t = assertFailsWith { - ChildrenChange.Remove(Id(1), ChildrenTag(2), 3, 4, listOf(Id(5), Id(6), Id(7))) - } - assertThat(t).hasMessage("Count 4 != Removed ID list size 3") - } - @Test fun modifierElementSerialization() { assertJsonRoundtrip( ModifierElement.serializer(), diff --git a/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt b/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt index af8917a0ef..a94049eef8 100644 --- a/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt +++ b/redwood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolGuestGeneration.kt @@ -96,7 +96,7 @@ class ExampleProtocolBridge private constructor( json: Json, mismatchHandler: ProtocolMismatchHandler, ): ExampleProtocolBridge { - val state = ProtocolState() + val state = ProtocolState(hostVersion) val root = ProtocolWidgetChildren(Id.Root, ChildrenTag.Root, state) val widgetSystem = ExampleWidgetSystem( Example = ProtocolExampleWidgetFactory(state, json, mismatchHandler), @@ -178,7 +178,7 @@ internal fun generateProtocolBridge( .addParameter("json", KotlinxSerialization.Json) .addParameter("mismatchHandler", ProtocolGuest.ProtocolMismatchHandler) .returns(type) - .addStatement("val state = %T()", ProtocolGuest.ProtocolState) + .addStatement("val state = %T(hostVersion)", ProtocolGuest.ProtocolState) .addStatement("val root = %T(%T.Root, %T.Root, state)", ProtocolGuest.ProtocolWidgetChildren, Protocol.Id, Protocol.ChildrenTag) .apply { val arguments = buildList { @@ -327,8 +327,7 @@ internal class ProtocolButton( } } - override fun visitIds(block: (Id) -> Unit) { - block(id) + override fun depthFirstWalk(block: (parent: ProtocolWidget, childrenTag: ChildrenTag, children: ProtocolWidgetChildren) -> Unit) { } } */ @@ -527,14 +526,22 @@ internal fun generateProtocolWidget( .build(), ) .addFunction( - FunSpec.builder("visitIds") + FunSpec.builder("depthFirstWalk") .addModifiers(OVERRIDE) - .addParameter("block", LambdaTypeName.get(null, Protocol.Id, returnType = UNIT)) - .addStatement("block(id)") + .addParameter( + "block", + LambdaTypeName.get( + null, + ProtocolGuest.ProtocolWidget, + Protocol.ChildrenTag, + ProtocolGuest.ProtocolWidgetChildren, + returnType = UNIT, + ), + ) .apply { for (trait in widget.traits) { if (trait is ProtocolChildren) { - addStatement("%N.visitIds(block)", trait.name) + addStatement("%N.depthFirstWalk(this, block)", trait.name) } } }