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.

Sign up for GitHub

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

Eliminate boxing of protocol types for serialization #2330

Merged
merged 5 commits into from
Sep 27, 2024
Merged

Eliminate boxing of protocol types for serialization #2330

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
338e316
Eliminate boxing of protocol types for serialization
JakeWharton Sep 25, 2024
3882fe7
Fix test
JakeWharton Sep 26, 2024
f47d390
WHOOPS
JakeWharton Sep 26, 2024
e75a34f
Push initialization of subtree synthesization to base
JakeWharton Sep 26, 2024
6e93c15
API dump
JakeWharton Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions redwood-protocol-guest/api/redwood-protocol-guest.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@ public final class app/cash/redwood/protocol/guest/DefaultGuestProtocolAdapter :
public final fun takeChanges ()Ljava/util/List;
}

public abstract interface class app/cash/redwood/protocol/guest/GuestProtocolAdapter : app/cash/redwood/protocol/EventSink {
public abstract class app/cash/redwood/protocol/guest/GuestProtocolAdapter : app/cash/redwood/protocol/EventSink {
public static final field $stable I
public fun <init> ()V
public abstract fun appendAdd-ARs5Qwk (IIILapp/cash/redwood/protocol/guest/ProtocolWidget;)V
public abstract fun appendCreate-kyz2zXs (II)V
public abstract fun appendModifierChange-z3jyS0k (ILapp/cash/redwood/Modifier;)V
Expand All @@ -31,8 +33,8 @@ public abstract interface class app/cash/redwood/protocol/guest/GuestProtocolAda
public abstract fun appendPropertyChange-M7EZMwg (III)V
public abstract fun appendPropertyChange-e3iP1vo (IIZ)V
public abstract fun appendRemove-HpxY78w (IIIILjava/util/List;)V
public static synthetic fun appendRemove-HpxY78w$default (Lapp/cash/redwood/protocol/guest/GuestProtocolAdapter;IIIILjava/util/List;ILjava/lang/Object;)V
public abstract fun emitChanges ()V
public final fun getChildrenVisitor ()Lapp/cash/redwood/protocol/guest/ProtocolWidget$ChildrenVisitor;
public abstract fun getJson ()Lkotlinx/serialization/json/Json;
public abstract fun getRoot ()Lapp/cash/redwood/widget/Widget$Children;
public abstract fun getSynthesizeSubtreeRemoval ()Z
Expand All @@ -58,18 +60,22 @@ public final class app/cash/redwood/protocol/guest/ProtocolRedwoodCompositionKt
}

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 depthFirstWalk (Lapp/cash/redwood/protocol/guest/ProtocolWidget$ChildrenVisitor;)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 interface class app/cash/redwood/protocol/guest/ProtocolWidget$ChildrenVisitor {
public abstract fun visit-KUkifdM (Lapp/cash/redwood/protocol/guest/ProtocolWidget;ILapp/cash/redwood/protocol/guest/ProtocolWidgetChildren;)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 <init> (IILapp/cash/redwood/protocol/guest/GuestProtocolAdapter;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
public final fun depthFirstWalk (Lapp/cash/redwood/protocol/guest/ProtocolWidget;Lkotlin/jvm/functions/Function3;)V
public final fun depthFirstWalk (Lapp/cash/redwood/protocol/guest/ProtocolWidget;Lapp/cash/redwood/protocol/guest/ProtocolWidget$ChildrenVisitor;)V
public fun detach ()V
public fun getWidgets ()Ljava/util/List;
public fun insert (ILapp/cash/redwood/widget/Widget;)V
Expand Down
62 changes: 36 additions & 26 deletions redwood-protocol-guest/api/redwood-protocol-guest.klib.api
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion ...uest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/DefaultGuestProtocolAdapter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class DefaultGuestProtocolAdapter(
hostVersion: RedwoodVersion,
private val widgetSystemFactory: ProtocolWidgetSystemFactory,
private val mismatchHandler: ProtocolMismatchHandler = ProtocolMismatchHandler.Throwing,
) : GuestProtocolAdapter {
) : GuestProtocolAdapter() {
private var nextValue = Id.Root.value + 1
private val widgets = mutableMapOf<Int, ProtocolWidget>()
private val changes = mutableListOf<Change>()
Expand Down
66 changes: 48 additions & 18 deletions ...tocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/GuestProtocolAdapter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,54 +36,54 @@ import kotlinx.serialization.json.Json
*
* This interface is for generated code use only.
*/
public interface GuestProtocolAdapter : EventSink {
public abstract class GuestProtocolAdapter : EventSink {
@RedwoodCodegenApi
public val json: Json
public abstract val json: Json

/**
* 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.
*/
@RedwoodCodegenApi
public val synthesizeSubtreeRemoval: Boolean
public abstract val synthesizeSubtreeRemoval: Boolean

/**
* The provider of factories of widgets which record property changes and whose children changes
* are also recorded. You **must** attach returned widgets to [root] or the children of a widget
* in the tree beneath [root] in order for it to be tracked.
*/
public val widgetSystem: WidgetSystem<Unit>
public abstract val widgetSystem: WidgetSystem<Unit>

/**
* The root of the widget tree onto which [widgetSystem]-produced widgets can be added. Changes to
* this instance are recorded as changes to [Id.Root] and [ChildrenTag.Root].
*/
public val root: Widget.Children<Unit>
public abstract val root: Widget.Children<Unit>

public fun initChangesSink(changesSink: ChangesSink)
public abstract fun initChangesSink(changesSink: ChangesSink)

public fun emitChanges()
public abstract fun emitChanges()

@RedwoodCodegenApi
public fun nextId(): Id
public abstract fun nextId(): Id

@RedwoodCodegenApi
public fun appendCreate(
public abstract fun appendCreate(
id: Id,
tag: WidgetTag,
)

@RedwoodCodegenApi
public fun <T> appendPropertyChange(
public abstract fun <T> appendPropertyChange(
id: Id,
tag: PropertyTag,
serializer: KSerializer<T>,
value: T,
)

@RedwoodCodegenApi
public fun appendPropertyChange(
public abstract fun appendPropertyChange(
id: Id,
tag: PropertyTag,
value: Boolean,
Expand All @@ -97,28 +97,28 @@ public interface GuestProtocolAdapter : EventSink {
* https://github.com/Kotlin/kotlinx.serialization/issues/2713
*/
@RedwoodCodegenApi
public fun appendPropertyChange(
public abstract fun appendPropertyChange(
id: Id,
tag: PropertyTag,
value: UInt,
)

@RedwoodCodegenApi
public fun appendModifierChange(
public abstract fun appendModifierChange(
id: Id,
value: Modifier,
)

@RedwoodCodegenApi
public fun appendAdd(
public abstract fun appendAdd(
id: Id,
tag: ChildrenTag,
index: Int,
child: ProtocolWidget,
)

@RedwoodCodegenApi
public fun appendMove(
public abstract fun appendMove(
id: Id,
tag: ChildrenTag,
fromIndex: Int,
Expand All @@ -127,14 +127,44 @@ public interface GuestProtocolAdapter : EventSink {
)

@RedwoodCodegenApi
public fun appendRemove(
public abstract fun appendRemove(
id: Id,
tag: ChildrenTag,
index: Int,
count: Int,
removedIds: List<Id> = listOf(),
removedIds: List<Id>,
Comment on lines -135 to +138
Copy link
Collaborator Author

@JakeWharton JakeWharton Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused boxing of the Id and ChildrenTag in JS if the caller omitted this last argument and relied on the default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we can validate boxing doesn't occur in a test case? Or is it only possible when inspecting the generated code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll think about it. You definitely can see the boxing in a full compiler plugin, and maybe in FIR only. Probably also as a lint check.

Filed #2336

)

@RedwoodCodegenApi
public fun removeWidget(id: Id)
public abstract fun removeWidget(id: Id)

@RedwoodCodegenApi
public val childrenVisitor: ProtocolWidget.ChildrenVisitor = if (synthesizeSubtreeRemoval) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocate only once, rather than once per children wrapper.

object : ProtocolWidget.ChildrenVisitor {
override fun visit(
parent: ProtocolWidget,
childrenTag: ChildrenTag,
children: ProtocolWidgetChildren,
) {
// This boxes Id values. Don't bother optimizing since it only serves very old hosts.
val childIds = children.widgets.map(ProtocolWidget::id)
for (childId in childIds) {
removeWidget(childId)
}
appendRemove(parent.id, childrenTag, 0, childIds.size, childIds)
}
}
} else {
object : ProtocolWidget.ChildrenVisitor {
override fun visit(
parent: ProtocolWidget,
childrenTag: ChildrenTag,
children: ProtocolWidgetChildren,
) {
for (childWidget in children.widgets) {
removeWidget(childWidget.id)
}
}
}
}
}
12 changes: 7 additions & 5 deletions ...od-protocol-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidget.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,19 @@ public interface ProtocolWidget : Widget<Unit> {
* }
* }
* ```
* You will see the following argument values passed to [block] if invoked on the `Split`:
* You will see the following argument values passed to [visitor] 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: (
public fun depthFirstWalk(visitor: ChildrenVisitor)

public fun interface ChildrenVisitor {
public fun visit(
parent: ProtocolWidget,
childrenTag: ChildrenTag,
children: ProtocolWidgetChildren,
) -> Unit,
)
)
}
}
23 changes: 7 additions & 16 deletions ...col-guest/src/commonMain/kotlin/app/cash/redwood/protocol/guest/ProtocolWidgetChildren.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,32 +38,23 @@ public class ProtocolWidgetChildren(

override fun remove(index: Int, count: Int) {
if (guestAdapter.synthesizeSubtreeRemoval) {
// This boxes Id values. Don't bother optimizing since it only serves very old hosts.
val removedIds = ArrayList<Id>(count)
for (i in index until index + count) {
val widget = _widgets[i]
removedIds += widget.id
guestAdapter.removeWidget(widget.id)

widget.depthFirstWalk { parent, childrenTag, children ->
val childIds = children.widgets.map(ProtocolWidget::id)
for (childId in childIds) {
guestAdapter.removeWidget(childId)
}
guestAdapter.appendRemove(parent.id, childrenTag, 0, childIds.size, childIds)
}
widget.depthFirstWalk(guestAdapter.childrenVisitor)
}
guestAdapter.appendRemove(id, tag, index, count, removedIds)
} else {
for (i in index until index + count) {
val widget = _widgets[i]
guestAdapter.removeWidget(widget.id)
widget.depthFirstWalk { _, _, children ->
for (childWidget in children.widgets) {
guestAdapter.removeWidget(childWidget.id)
}
}
widget.depthFirstWalk(guestAdapter.childrenVisitor)
}
guestAdapter.appendRemove(id, tag, index, count)
guestAdapter.appendRemove(id, tag, index, count, emptyList())
}

_widgets.remove(index, count)
Expand All @@ -79,12 +70,12 @@ public class ProtocolWidgetChildren(

public fun depthFirstWalk(
parent: ProtocolWidget,
block: (ProtocolWidget, ChildrenTag, ProtocolWidgetChildren) -> Unit,
visitor: ProtocolWidget.ChildrenVisitor,
) {
for (widget in widgets) {
widget.depthFirstWalk(block)
widget.depthFirstWalk(visitor)
}
block(parent, tag, this)
visitor.visit(parent, tag, this)
}

override fun detach() {
Expand Down
15 changes: 11 additions & 4 deletions redwood-protocol/api/redwood-protocol.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ public synthetic class app/cash/redwood/protocol/ChildrenChange$Add$$serializer
}

public final class app/cash/redwood/protocol/ChildrenChange$Add$Companion {
public final fun invoke-11bgaWM (IIII)Lapp/cash/redwood/protocol/ChildrenChange$Add;
public final fun serializer ()Lkotlinx/serialization/KSerializer;
}

Expand Down Expand Up @@ -72,12 +73,12 @@ public synthetic class app/cash/redwood/protocol/ChildrenChange$Move$$serializer
}

public final class app/cash/redwood/protocol/ChildrenChange$Move$Companion {
public final fun invoke-HpxY78w (IIIII)Lapp/cash/redwood/protocol/ChildrenChange$Move;
public final fun serializer ()Lkotlinx/serialization/KSerializer;
}

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 <init> (IIIILjava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public synthetic fun <init> (IIIILjava/util/List;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun equals (Ljava/lang/Object;)Z
public final fun getCount ()I
Expand All @@ -101,6 +102,8 @@ public synthetic class app/cash/redwood/protocol/ChildrenChange$Remove$$serializ
}

public final class app/cash/redwood/protocol/ChildrenChange$Remove$Companion {
public final fun invoke-HpxY78w (IIIILjava/util/List;)Lapp/cash/redwood/protocol/ChildrenChange$Remove;
public static synthetic fun invoke-HpxY78w$default (Lapp/cash/redwood/protocol/ChildrenChange$Remove$Companion;IIIILjava/util/List;ILjava/lang/Object;)Lapp/cash/redwood/protocol/ChildrenChange$Remove;
public final fun serializer ()Lkotlinx/serialization/KSerializer;
}

Expand Down Expand Up @@ -157,12 +160,12 @@ public synthetic class app/cash/redwood/protocol/Create$$serializer : kotlinx/se
}

public final class app/cash/redwood/protocol/Create$Companion {
public final fun invoke-kyz2zXs (II)Lapp/cash/redwood/protocol/Create;
public final fun serializer ()Lkotlinx/serialization/KSerializer;
}

public final class app/cash/redwood/protocol/Event {
public static final field Companion Lapp/cash/redwood/protocol/Event$Companion;
public synthetic fun <init> (IILjava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public synthetic fun <init> (IILjava/util/List;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun equals (Ljava/lang/Object;)Z
public final fun getArgs ()Ljava/util/List;
Expand All @@ -184,6 +187,8 @@ public synthetic class app/cash/redwood/protocol/Event$$serializer : kotlinx/ser
}

public final class app/cash/redwood/protocol/Event$Companion {
public final fun invoke-OtSvs1c (IILjava/util/List;)Lapp/cash/redwood/protocol/Event;
public static synthetic fun invoke-OtSvs1c$default (Lapp/cash/redwood/protocol/Event$Companion;IILjava/util/List;ILjava/lang/Object;)Lapp/cash/redwood/protocol/Event;
public final fun serializer ()Lkotlinx/serialization/KSerializer;
}

Expand Down Expand Up @@ -254,7 +259,6 @@ public final class app/cash/redwood/protocol/Id$Companion {

public final class app/cash/redwood/protocol/ModifierChange : app/cash/redwood/protocol/ValueChange {
public static final field Companion Lapp/cash/redwood/protocol/ModifierChange$Companion;
public synthetic fun <init> (ILjava/util/List;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public synthetic fun <init> (ILjava/util/List;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun equals (Ljava/lang/Object;)Z
public final fun getElements ()Ljava/util/List;
Expand All @@ -275,6 +279,8 @@ public synthetic class app/cash/redwood/protocol/ModifierChange$$serializer : ko
}

public final class app/cash/redwood/protocol/ModifierChange$Companion {
public final fun invoke-z3jyS0k (ILjava/util/List;)Lapp/cash/redwood/protocol/ModifierChange;
public static synthetic fun invoke-z3jyS0k$default (Lapp/cash/redwood/protocol/ModifierChange$Companion;ILjava/util/List;ILjava/lang/Object;)Lapp/cash/redwood/protocol/ModifierChange;
public final fun serializer ()Lkotlinx/serialization/KSerializer;
}

Expand Down Expand Up @@ -320,7 +326,6 @@ public final class app/cash/redwood/protocol/ModifierTag$Companion {

public final class app/cash/redwood/protocol/PropertyChange : app/cash/redwood/protocol/ValueChange {
public static final field Companion Lapp/cash/redwood/protocol/PropertyChange$Companion;
public synthetic fun <init> (IILkotlinx/serialization/json/JsonElement;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public synthetic fun <init> (IILkotlinx/serialization/json/JsonElement;Lkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun equals (Ljava/lang/Object;)Z
public fun getId-0HhLjSo ()I
Expand All @@ -342,6 +347,8 @@ public synthetic class app/cash/redwood/protocol/PropertyChange$$serializer : ko
}

public final class app/cash/redwood/protocol/PropertyChange$Companion {
public final fun invoke-e3iP1vo (IILkotlinx/serialization/json/JsonElement;)Lapp/cash/redwood/protocol/PropertyChange;
public static synthetic fun invoke-e3iP1vo$default (Lapp/cash/redwood/protocol/PropertyChange$Companion;IILkotlinx/serialization/json/JsonElement;ILjava/lang/Object;)Lapp/cash/redwood/protocol/PropertyChange;
public final fun serializer ()Lkotlinx/serialization/KSerializer;
}

Expand Down
Loading
Loading