Skip to content

fix: Scope the map decode fast paths to their own half of an entry - #768

Merged
lwshang merged 3 commits into
masterfrom
scope-bignum-fast-path-to-map-values
Sep 22, 2026
Merged

lwshang merged 3 commits into
masterfrom
scope-bignum-fast-path-to-map-values

Conversation

@lwshang

@lwshang lwshang commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

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 Types (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

`deserialize_map` derives a big-integer fast path from the map's value
type. That flag lived on the deserializer for the whole map, so it was
still active while an entry's key was decoded, and the value relied on
the expected/wire types set up before the first entry.

Scope it to values instead: `Style::Map` carries the map's own fast
path, `next_key_seed` clears it for the duration of the key, and
`next_value_seed` restores it and re-establishes the value's expected
and wire types on every entry.

A key now always goes through its own type's entry point, keeping its
own encoding (SLEB128 for `int`, LEB128 for `nat`) and its own subtype
check, and a value keeps its own types, for every combination of key
and value type. Restoring per entry also means a nested compound
inside a value cannot leave the fast path cleared for the entries that
follow it.

The wire format is unchanged, entries whose key and value types agree
decode identically, and cost accounting is unchanged: `any_fast` reads
the value stashed on `Style::Map`, which held the same value on every
entry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

No unresolved blocking issues were identified, and the changes include regression coverage.

Review effort: Lite
Findings: None

What changed in this PR

Fixes map deserialization so big-integer fast-path state applies only to values while keys and values retain their own types.

Changes:

  • Scopes fast-path state and restores per-entry type metadata.
  • Adds comprehensive map key/value regression tests.
  • Documents the fix under Unreleased.
File Description
rust/​candid/​tests/​serde.rs Adds map key/value decoding regression coverage.
rust/​candid/​src/​de.rs Corrects map fast-path and type-state handling.
CHANGELOG.md Documents the fix.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@lwshang
lwshang marked this pull request as ready for review September 21, 2026 09:41
@lwshang
lwshang requested a review from a team as a code owner September 21, 2026 09:41
@zeropath-ai

zeropath-ai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

✅ No security or compliance issues detected. Reviewed everything up to 2f5d1b2.

Security Overview
Detected Code Changes
Change Type Relevant files
Enhancement ► rust/candid/src/de.rs
      Improve map key/value decoding to use separate types for keys and values and manage bignum fast path
► rust/candid/tests/serde.rs
      Add test: map entry key and value decode under their own types

@marc0olo marc0olo left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified the change and the new test; it fails on master as expected. Full suite green on --all-features and --no-default-features, clippy clean. I also checked nested maps, maps inside records, multi-entry maps and IDLValue, plus both subtype directions on keys — all correct.

One thing before I approve: key_text_fast is the symmetric case and isn't handled here. It is derived from the key type but stays set while the value decodes, so deserialize_str can take the same shortcut this PR removes for bignum. One line in next_value_seed:

                 self.de.expect_type = expect.1.clone();
                 self.de.wire_type = wire.1.clone();
+                self.de.text_fast_path = false;
                 #[cfg(feature = "bignum")]
                 {
                     self.de.bignum_vec_fast_path = value_bignum_fast;
                 }

I tried it on top of this branch and the candid suite still passes. Happy to approve with that plus a test case, or with the "every combination of key and value type" claim in the description narrowed and a follow-up filed.

Minor: worth putting a number on the extra per-entry Type clones. 200k-entry maps, release build: nat->nat +11%, int->int +5%, text->nat +1%. Cheap for what it buys.

The text fast path is the mirror image of the big-integer one: it is
derived from the map's key type, but stayed active while the value was
decoded, letting `deserialize_str` read a value without its subtype
check. `blob` shares text's length-prefixed encoding, so under a text
key a `blob` value was accepted in a text value's place and decoded
silently — the same coercion is rejected under a non-text key and
outside a map.

Clear it for the duration of the value; `next_key_seed` sets it again
for the following key. Both fast paths are now scoped to their own
half of an entry.

Extends the regression test with the text side: `blob` against a text
value rejected under a text key and under a non-text key, and text
keys and values still round-tripping in both positions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@lwshang lwshang changed the title fix: Scope the big-integer decode fast path to map values fix: Scope the map decode fast paths to their own half of an entry Sep 21, 2026
@lwshang
lwshang requested a review from marc0olo September 22, 2026 06:36

@marc0olo marc0olo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-checked — this addresses everything. The case that slipped through before is
now rejected, and the key/value matrix I ran earlier is still clean.

Both halves are real coverage, not vacuous: the extended test fails on master at
the bignum assertion and on a05b2fe (bignum fix only) at the blob assertion, so
neither half rides on the other's fix. Full suite green on --features all and
--no-default-features, clippy clean.

Nice outcome that both flags are now re-established at each half-boundary rather
than set once for the map — a nested compound inside a value can't disturb them
either. I also checked primitive_vec_fast_path for the same pattern: only set
in the seq path, never by deserialize_map, and cleared by Drop for Compound,
so "both fast paths" is complete.

Cost of clearing the text path, 200k-entry maps, release: text->text 43.1 -> 45.5 ms,
text->nat 42.8 -> 44.3 ms. In line with the bignum side.

LGTM.

@lwshang
lwshang merged commit a3ffaf5 into master Sep 22, 2026
17 checks passed
@lwshang
lwshang deleted the scope-bignum-fast-path-to-map-values branch September 22, 2026 09:40
@lwshang lwshang mentioned this pull request Sep 22, 2026
3 of 5 tasks
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants