Optimize Modifier serialization in guest code#2253
Conversation
| ): WidgetSystem<Unit> | ||
|
|
||
| @RedwoodCodegenApi | ||
| public fun modifierTag(element: Modifier.Element): ModifierTag |
There was a problem hiding this comment.
Tempted to do one function that returns a Pair<ModifierTag, KSerializer<T>?> instead of two functions. The only hazard of doing one function is I’d like to avoid allocating a Pair if possible.
There was a problem hiding this comment.
We generate the KSerializer for every modifier. We can do two things:
- Switch to subtyping
SerializationStrategyin the generated type rather thanKSerializer. We generate custom serializers because we only want the serialization code, so we should only be implementing the serialization interface (no clue why I didn't do this initially). - Define a
ProtocolSerializationStrategyinterface which addsval tag: ModifierTagtoSerializationStrategyand switch our codegen to use that. This eliminates the need to do a double, type-based lookup.
There was a problem hiding this comment.
I like both of these ideas. I’ll add a commit to this PR to do that!
There was a problem hiding this comment.
Done. Unfortunately I couldn’t do it exactly as suggested 'cause we don’t have a serializer for stateless modifiers. So instead it’s a Pair<ModifierTag, SerializationStrategy>. Seems to work okay and read okay.
|
|
||
| /** Returns null if the modifier is stateless and should serialize as null. */ | ||
| @RedwoodCodegenApi | ||
| public fun <T : Modifier.Element> modifierSerializer(element: T): KSerializer<T>? |
There was a problem hiding this comment.
On my first attempt this returned a non-null KSerializer<T>, but I ran into a this bug in Kotlinx.serialization. I’d like to contribute a fix for that bug and then later I can change this to return non-null always.
| import app.cash.redwood.widget.WidgetSystem | ||
| import kotlinx.serialization.KSerializer | ||
|
|
||
| public interface ProtocolWidgetSystemFactory { |
There was a problem hiding this comment.
With these two new APIs I don’t think the Factory name fit as well as it did previously.
swankjesse
force-pushed
the
jwilson.0821.modifier_serialization
branch
from
9a44f86 to
3a8e791
Compare
| serializer == null -> null | ||
| else -> json.encodeToDynamic(serializer, element) | ||
| } | ||
| elements.push(js("""[tag,value]""")) |
There was a problem hiding this comment.
The protocol when used normally does not encode null values here. There's no real harm in it, though, except for the 5 extra bytes on the wire.
There was a problem hiding this comment.
We might need to move our protocol serialization tests upward to ensure these custom forms mirror it in behavior.
There was a problem hiding this comment.
Ooooh good catch, I missed that. I think it’s a nice improvement to test at the JSON-encoded layer for this, though I don’t think Zipline’s asDynamicFunction() makes this particularly easy. It’s trying to hide the JSON layer and the test wants to exercise it!
There was a problem hiding this comment.
Fixed to not encode null, but I haven’t yet fixed the test case to validate the JSON matches as specified. I’ll need to contemplate how to do that in a way that feels right at this layer.
swankjesse
force-pushed
the
jwilson.0821.modifier_serialization
branch
from
735934d to
bec3c19
Compare
I learned that encoding a type as a JsonElement and then encoding that as JSON is significantly less efficient than going directly from the type to JSON. This is due to some inefficient code in the internals of kotlinx.serialization. Skipping this intermediate step is useful on its own, but it also turns out to be significantly more efficient in practice.
swankjesse
force-pushed
the
jwilson.0821.modifier_serialization
branch
from
bec3c19 to
cedf07f
Compare
2 participants
I learned that encoding a type as a JsonElement and then encoding that as JSON is significantly less efficient than going directly from the type to JSON. This is due to some inefficient code in the internals of kotlinx.serialization.
Skipping this intermediate step is useful on its own, but it also turns out to be significantly more efficient in practice.
CHANGELOG.md's "Unreleased" section has been updated, if applicable.