Share the decoder's argument queue to avoid superlinear option-skip cost - #767
Merged
Merged
Conversation
The type table size already asserts a bound on its wire-declared length, but the sibling counts in the header did not: the argument count (`Header.len`), a record or variant's field count (`Fields.len`), a function type's argument and result counts (`FuncType.arg_len`/`ret_len`), and a service type's method count (`ServType.len`). Each feeds a `#[br(count = len)]`, so an out-of-range value reserved a correspondingly large buffer up front. Cap each of them the way the type table already is: a well-formed message keeps every structural count proportional to the type description, so an out-of-range count now surfaces as an ordinary parse error. The argument count reuses the configurable `max_type_len` limit; the type-table-internal counts use the same default bound as the type table. The wire format is unchanged and valid messages decode identically. Also share the undecoded argument queue behind a reference count. When decoding a present `opt` whose wire and expected types differ, the deserializer snapshots itself to restore on a subtype mismatch. That snapshot only needs the fields a sub-decode can mutate, and the argument queue is not one of them — it is touched only at the top level — so it is now shared via `Rc` rather than cloned, turning the snapshot into a refcount bump. Decoding many optional trailing arguments is correspondingly cheaper and the result is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
✅ No security or compliance issues detected. Reviewed everything up to 8d37747. Security OverviewDetected Code Changes
|
Revert the candid/candid_derive 0.10.36 version bumps (and the lockfile) and replace the versioned changelog header with an Unreleased section. The decoder fix, the regression test, and the changelog bullets are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lwshang
changed the title
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Several new bounds and the backtracking restoration path lack effective regression coverage, and the public configuration contract is outdated.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 4
Open (4)
What changed in this PR
Bounds decoder structural counts and optimizes optional-value backtracking.
Changes:
- Adds limits for argument, field, function, and service counts.
- Shares the argument queue through
Rc. - Adds regression tests and release notes.
| File | Description |
|---|---|
| rust/candid/tests/arg_count_alloc.rs | Adds malformed-count and optional-decoding tests. |
| rust/candid/src/de.rs | Shares the undecoded argument queue. |
| rust/candid/src/binary_parser.rs | Enforces structural count bounds. |
| CHANGELOG.md | Documents decoder changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Document that `set_max_type_len` also bounds the top-level argument count, with the `set_max_type_len(1)` example, and add a config test asserting it rejects an over-limit argument count and accepts one within the limit. - Use a u32-fitting field count (2^31) in the record-field test so parsing reaches the new field-count bound instead of failing the u32 conversion first. - Add a function result-count test (zero args, oversized result count) so the `ret_len` bound has independent coverage. - Add a test that a present `opt` with a mismatched inner type takes the backtracking arm, decodes as `None`, and leaves the shared argument queue intact for the following argument. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Review measurement showed the count bounds this branch added were both unnecessary and a compatibility regression, and the shared-queue change is the actual fix: - binrw's `count` only pre-reserves for `Vec<u8>` (already handled by `read_len_prefixed`); for the header's element vectors it collects a `Result` iterator whose size hint lower bound is 0, so no oversized buffer was ever reserved. The malformed-count inputs already fail on the short read, so the asserts fixed no defect. - The 10_000 caps rejected messages that are valid today and that decode fine on master (for example a record with 10_001 null fields, a 30 KB message), and reusing `max_type_len` for the argument count silently changed a public config API. Both are removed. - Sharing the undecoded argument queue behind an `Rc` is what removes the superlinear cost: skipping present optional arguments went from O(args x present-opts) to linear in the argument count. The new regression test exercises this directly and trips only if the per-opt copy returns. Restores `binary_parser.rs` and the `set_max_type_len` doc to master, keeps the `de.rs` queue-sharing change, and replaces the count tests with `skip_optional_queue.rs`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lwshang
changed the title
marc0olo
approved these changes
Sep 21, 2026
marc0olo
left a comment
marc0olo
left a comment
Member
There was a problem hiding this comment.
Verified independently — the quadratic behaviour and the fix both reproduce.
Decoding arg 0 and skipping the rest (release): master 522 / 2065 / 8367 ms at
20k / 40k / 80k args, this branch 5.1 / 10.5 / 18.5 ms. Clean linear, and a
4000-entry type table only adds a constant factor, so nothing else in the
snapshot reintroduces it. skip_optional_queue.rs genuinely fails on master
(7.06 s). types is only touched at de.rs:74/93/119, all top level, so
make_mut never deep-copies — correctness looks right.
Two things worth adding:
- This also makes
decoding_quotahonest. The backtrack charges a flat
add_cost(10)no matter how big the copied queue was, so on master a quota of
10M still permitted ~2 s of wall clock on the 80k message; here the same quota
is enforced in ~7 ms. SinceDecoderConfig::newsets no quota by default, an
unquota'd endpoint eats the full 8.4 s on a ~240 KB payload — that seems worth
a 0.10.x point release rather than sitting under Unreleased. skipping_many_present_optionals_is_not_quadraticasserts on wall clock, and
CI runs debug three times over. 0.13 s vs a 2 s bound is fine margin, but a
test-localGlobalAlloccounting allocated bytes would be deterministic and
measure the copy directly.
LGTM otherwise.
lwshang
added a commit
that referenced
this pull request
Sep 22, 2026
) ## What this changes `deserialize_map` derives two fast paths for a map: a big-integer one from the **value** type, and a text one from the **key** type. Each lived on the deserializer for the whole entry, so each was still active while the *other* half was decoded. - `Style::Map` carries the map's own `value_bignum_fast`, so it survives key decoding without being globally active. - `next_key_seed` clears `bignum_vec_fast_path` for the duration of the key. - `next_value_seed` restores it, clears `text_fast_path` for the duration of the value, and re-establishes the value's `expect_type`/`wire_type` on every entry. ## The property this gives A map entry's key and value each decode under their own declared type, for every combination of key and value type: - a key always goes through its own type's entry point, keeping its own encoding — SLEB128 for `int`, LEB128 for `nat` — and its own subtype check; - a value keeps its own expected and wire types rather than whatever the key left behind, and its own subtype check even when its encoding coincides with the key's — `blob` shares text's length-prefixed encoding, so under a text key it was accepted in a text value's place and decoded silently, while the same coercion is rejected under a non-text key and outside a map; - this holds on both the ≤u64 fast path and the big-integer path. Restoring state per entry also means a nested compound inside a value cannot leave a fast path cleared for the entries that follow it — `Drop for Compound` clears both. ## Compatibility The wire format is unchanged and entries whose key and value types agree decode identically. Cost accounting is unchanged for the big-integer path: `any_fast` now reads the value stashed on `Style::Map` instead of the live deserializer flag, which held the same value on every entry. Clearing the text path means a text value now runs `unroll_type`, which charges 2 cost units per entry when the value type is a `Var`/`Knot` (a named or recursive type) and nothing otherwise. Decoding a value now always clones the value's two `Type`s (two `Rc` clones) rather than skipping them on the fast path. Measured by @marc0olo on 200k-entry maps in a release build: `nat`→`nat` +11%, `int`→`int` +5%, `text`→`nat` +1%. ## Tests `test_map_key_and_value_decode_under_own_type` in `rust/candid/tests/serde.rs` covers: - signed keys against `nat`, `int` and fixed-width values, and signed values against `nat`, `int` and text keys; - the big-integer path above u64 in key, value and both positions; - distinct keys staying distinct entries; - key subtype checks in both directions — `int` keys rejected where `nat` is expected, `nat` keys still accepted where `int` is expected; - `blob` rejected in a text value's place under both a text key and a non-text key, and text keys and values still round-tripping in both positions. Full workspace suite passes with `--all-features` and `--no-default-features`; clippy clean. Two pre-existing issues on `master` are unrelated to this change and left alone: an `ic_principal` doctest failure at `rust/ic_principal/src/lib.rs:55`, and a `dead_code` warning for `try_read_leb_i64` under `--no-default-features`. ## Release No version bump here — to be released explicitly, so the changelog entry sits under `## Unreleased`. Note that entry will conflict with #767's, which adds to the same block. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
lwshang
added a commit
that referenced
this pull request
Sep 22, 2026
Patch release of `candid` / `candid_derive`, 0.10.35 → 0.10.36. No source changes — it dates the two decoder fixes that are already on `master`. ## Summary - Bump `candid` and `candid_derive` 0.10.35 → 0.10.36 (and the `=` pin between them), refresh `Cargo.lock`, `rust/bench/Cargo.lock` and `tools/ui/Cargo.lock` - Move the `Unreleased` CHANGELOG entries into a dated `2026-09-22` / `Candid 0.10.36` section, covering: + Scoping a map's decode fast paths to their own half of an entry, so a key and a value each decode under their own declared type (#768) + Sharing the decoder's undecoded argument queue behind a reference count, so skipping present optional arguments is no longer superlinear in the argument count (#767) `tools/ui` patches `candid` to the workspace path, so its lockfile pins the path crate's version and has to move with the bump. `candid-ui-release.yml` builds `didjs` there with `--locked` and triggers on the date tag this release creates, so a stale lock there would only fail at tag time, after merge — not in PR checks. ## Test plan - [x] `cargo check -p candid -p candid_derive -p candid_parser` - [x] `cargo test -p candid` — all suites pass - [x] `cd tools/ui && cargo build --target wasm32-unknown-unknown --profile canister --package didjs --locked` — the release workflow's exact command, succeeds - [ ] Reviewer to confirm release scope and changelog date - [ ] Publish to crates.io after merge ## Follow-up (not in this PR) `rust/candid/fuzz/Cargo.lock` is stale at candid 0.10.34, already on `master`. Nothing builds it with `--locked`, so it is harmless, but all four lockfiles should move together as a release-checklist step. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The option/backtracking path in the deserializer snapshots itself so it can restore on a subtype mismatch. That snapshot copied the entire queue of remaining arguments on every present
opt, so skipping present optional arguments cost O(arguments × present-opts) — superlinear in a single message. The queue is only ever mutated at the top level (IDLDeserialize::get_value), never during a value sub-decode, so it now lives behind anRc: the snapshot is a refcount bump and skipping is linear in the argument count.Measurements
Decoding one argument and skipping the rest, where the first N present values are
?null(opt nulltype):skip_optional_queue.rslocks this in: it decodes a message with many present optionals and asserts the decode stays well under a loose time bound, tripping only if the per-optcopy returns. It also keeps two correctness tests for the shared queue across the top-level pops that follow a snapshot (including the mismatched-optrestore path).Compatibility
The wire format is unchanged and the decode result is identical. No public API or accepted-input change.
Note on the earlier revision
An earlier version of this branch also added
10_000caps on the header's structural counts (arguments, record fields, function arity, service methods). Those are dropped:binrw'scountdoes not pre-reserve for these element vectors (only forVec<u8>, whichread_len_prefixedalready covers), so the caps fixed no allocation defect; they rejected messages that are valid today (e.g. a 30 KB record with 10,001 fields) and changed the meaning of the publicmax_type_lenconfig. The queue-sharing change alone addresses the superlinear cost.🤖 Generated with Claude Code