Compile time: replace serde Content buffering with JSON-value deserializers - #15455
Compile time: replace serde Content buffering with JSON-value deserializers#15455vorporeal wants to merge 1 commit into
Conversation
8459a0a to
1fb2f10
Compare
1fb2f10 to
2f49498
Compare
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR replaces several serde derive-generated deserializers with handwritten JSON-value-based deserializers while preserving the existing serialization derives and wire formats. The implementation is broadly scoped to DCS hooks, bootstrapped shell payloads, AI agent context/attachments, and artifacts, with unit tests added around the main formats.
Concerns
- The new handwritten dispatch paths are not fully covered by the added tests. In particular, the DProtoHook tag match omits round-trip coverage for several variants, and the AIAgentContext tagged-variant table omits ExecutionEnvironment even though both paths are now maintained manually.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| D: Deserializer<'de>, | ||
| { | ||
| let raw = RawDProtoHook::deserialize(deserializer)?; | ||
| Ok(match raw.hook.as_str() { |
There was a problem hiding this comment.
DProtoHook variant; the added tests only exercise a subset, so tag/value-mapping regressions for the remaining variants would no longer be caught by serde derive.
|
|
||
| #[test] | ||
| fn ai_agent_context_round_trips_tagged_variants() { | ||
| let contexts = vec![ |
There was a problem hiding this comment.
ExecutionEnvironment, even though the PR adds a handwritten AIAgentContextTagged::ExecutionEnvironment arm; add that case so every tagged variant is covered.
…lizers Serde derives expensive generic buffering machinery (Content, ContentRefDeserializer, and per-field __DeserializeWith wrappers) for enums that are internally tagged, adjacently tagged, or mix tagged and untagged variants. These types are only deserialized from JSON, so this change replaces the derived Deserialize impls with hand-written impls that buffer into serde_json::Value and preserve the wire format exactly: - DProtoHook: match on the raw hook name, then parse the value payload. - BootstrappedValue: a raw twin struct replaces the per-field deserialize_with wrappers and applies the string conversions in one place. - Artifact: an adjacently tagged envelope struct replaces the ArtifactHelper enum. - AIAgentContext and AIAgentAttachment: a derived tagged twin enum handles the named variants, with an explicit fallback to the untagged BlockContext variant. Serialization derives are unchanged everywhere. New round-trip tests pin the wire formats. Co-Authored-By: Warp <agent@warp.dev>
2f49498 to
6224110
Compare
Description
This is PR 3 of a stack that reduces the compile time of the
warpcrate. It is stacked on #15454.Why
For enums that are internally tagged, adjacently tagged, or that mix tagged and untagged variants, serde's derive cannot deserialize in one pass. It generates buffering machinery (
Content,ContentRefDeserializer,ContentDeserializer) that copies the input into a generic tree and replays it for each candidate variant. Fields withdeserialize_withalso each get a private__DeserializeWithwrapper struct. This machinery is generic over the input lifetime and the error type, so it monomorphizes widely and it pulls large dependent trees (for exampleBlockContextand its field types) into the instantiations.The affected types are only deserialized from JSON (
serde_json::from_str/from_slice/from_value). I verified each call site before this change. Because the input format is known, a hand-writtenDeserializecan buffer intoserde_json::Valueonce and then parse the payload directly, with no generic replay machinery.What changed
Serialization derives are unchanged everywhere. Only deserialization is hand-written, and it accepts the exact same wire format as before:
DProtoHook(app/src/terminal/model/ansi/dcs_hooks.rs): a small raw struct captures thehooktag and thevaluepayload, and a 13-arm match parses the payload. TheSourcedRcFileForWarpshape is frozen because the payload literal ships inside user RC files; a test pins it.BootstrappedValue(same file): a raw twin struct replaces the per-fielddeserialize_withwrappers (empty_string_is_none, float parsing, shell-option splitting). The conversions now run in one non-generic place. ARawBootstrappedFieldhelper keeps the old missing-versus-present semantics for optional fields.Artifact(app/src/ai/artifacts/mod.rs): anArtifactEnvelopestruct replaces theArtifactHelpermirror enum for the adjacently tagged (artifact_type/data) format. ThePULL_REQUESTarm still derivesrepoandnumberfrom the URL.AIAgentContextandAIAgentAttachment(app/src/ai/agent/mod.rs): these mix externally tagged variants with an untaggedBlock(BlockContext)variant, which is the most expensive derive pattern. A private derived twin enum handles the tagged variants, and the impl falls back toBlockContextwhen the tagged parse fails — the same fallthrough order the derive used. This also removesBlockContext's large deserializer tree from theContentRefDeserializerinstantiations.Alternatives considered
Blocka tagged variant) would remove the machinery with less code, but it changes the wire format. These payloads live in persisted conversations, RC-file snippets, and server messages, so the format must not change.CodeSourcewas on the candidate list but is a plain externally tagged enum. Its derive does not generate the buffering machinery, so it was left alone.PrecmdValue/PromptMetadatause#[serde(flatten)], which generates similar buffering. That is left as follow-up work because the flatten structure is more intricate.Measured effect (vs. PR 2, Apple M5 Pro, rustc 1.92.0, dev profile)
cargo llvm-lines -p warp --lib: 17,022,929 → 16,966,243 lines (−0.33%); 573,201 → 571,576 copies.Content*/__DeserializeWithsymbols): 167,090 → 89,078 IR lines (−47%). The remainder comes from other crates' types that still use these patterns.DProtoHook24,664 → 6,773;BootstrappedValue13,299 → 7,158;Artifactsymbols 39,443 → 18,148 (ArtifactHelper23,101 → 0);AIAgentContext21,473 → 18,172;AIAgentAttachment15,841 → 13,418.-Zmacro-stats): 32,172,116 → 31,907,058 bytes (−0.8%).warplib rebuild (deps cached,CARGO_INCREMENTAL=0, two runs): 71.4 s / 71.1 s → 69.2 s / 69.1 s.The wall-clock gain is small (~2 s clean); the main value is removing the widest remaining serde instantiations in the crate and establishing the pattern for the flatten-based types.
Linked Issue
No linked issue. This PR comes from a compile-time investigation; see the conversation link below.
Testing
app/src/terminal/model/ansi/dcs_hooks_tests.rs(new file), plus new tests inapp/src/ai/artifacts/mod_tests.rsandapp/src/ai/agent/mod_tests.rs. They serialize each variant with the unchanged derivedSerializeand assert the hand-writtenDeserializereturns an equal value, and they pin raw JSON literals for the frozen formats (RC-file hook, artifact envelope, pull-request number coercion).cargo nextest run -p warp -E 'test(/ai::|conversation/)': 2372 passed../script/formatand all three presubmit clippy passes are clean../script/run; behavior is covered by the deserialization tests above, and the wire formats are unchanged.Agent Mode
Conversation: https://staging.warp.dev/conversation/024e797d-1d35-4c62-8394-abf93f7ddb0e
Co-Authored-By: Warp agent@warp.dev
CHANGELOG-NONE