⚡ Move scalar text out of events during decode, dropping the pre-pass by frenck · Pull Request #92 · frenck/YAMLRocks

frenck · 2026-06-23T08:43:17Z

Breaking change

None. Same decoded values, same zero-copy string ownership, byte-for-byte identical output.

Proposed change

The fast decoder copied every scalar's text into a parallel side table (Vec<Option<Cow>>) in a separate pass over the whole event stream, before decoding, then pulled each string from that table by position. The decoder is a single forward pass that only ever mutated the side table, never the events themselves, so the table was pure overhead: one extra walk of the stream and one allocation sized to it.

This borrows the events mutably and mem::takes each scalar's text straight out of its event as the decoder consumes it, handing the string to Value::String with the exact same zero-copy ownership transfer. Same result, one fewer pass and one fewer allocation.

The change is mechanical: the decoder already copied each bound pattern value (let flow = *flow;) before recursing, which releases the scrutinee borrow, so flipping the seven recursing methods from &[Event] to &mut [Event] and moving the single scalar-take inline was accepted by the borrow checker without restructuring. The two read-only helpers stay on &[Event].

Measured with callgrind (deterministic instruction counts, cache-sim=no branch-sim=no) over the real-world config corpus, loads pipeline:

before: 6,708,729,277 instructions
after: 6,633,513,011 instructions
−1.1% on the full loads pipeline (the Rust-relative share is higher; the workload denominator includes file I/O)

This is the safe, isolated first step toward eliminating the materialized event vector on the fast path. Full streaming (dropping Vec<Event> entirely) remains a possible follow-up, to be re-measured once this lands.

Type of change

Dependency or tooling upgrade
Bugfix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Deprecation (replaces or removes a feature, with a migration path)
Breaking change (a fix or feature that changes existing behavior)
Code quality, refactor, or test-only change
Documentation only

Additional information

This PR fixes or closes issue: None
This PR is related to:
Link to a separate documentation pull request:

Checklist

I have read the AI Policy, and this pull request was not created by an autonomous agent.
I fully understand the code in this pull request and can explain every line, including any AI-assisted changes.
The change is covered by tests, and uv run pytest passes locally. A pull request cannot be merged unless CI is green.
uv run ruff check . and uv run ruff format --check . pass.
cargo fmt --check and cargo clippy --all-targets -- -D warnings pass.
Round-trip fidelity is preserved: an unmodified document still re-emits byte-for-byte.
No commented-out or dead code is left in the pull request.

If the change is user-facing:

Documentation under docs/ is added or updated, and docs/verify_examples.py still passes.

The fast decoder copied every scalar's text into a parallel side table in a separate pass over the whole event stream before decoding, then pulled from that table by position. The decoder is a single forward pass that only ever mutated the side table, never the events, so the table was pure overhead: one extra walk of the stream and one Vec<Option<Cow>> allocation sized to it. Borrow the events mutably and mem::take each scalar's text straight out of its event as the decoder consumes it, handing the string to Value::String with the same zero-copy ownership transfer. Same result, one fewer pass and allocation. Measured with callgrind over the real-world config corpus: -1.1% instructions on the full loads pipeline.

coderabbitai · 2026-06-23T08:43:27Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: ca9268db-16cc-4638-a515-59a705ce57bc

📥 Commits

Reviewing files that changed from the base of the PR and between 73003bb and af4f65e.

📒 Files selected for processing (1)

src/decode/mod.rs

📝 Walkthrough

Walkthrough

The decoder in src/decode/mod.rs is refactored to eliminate the scalar string side table. Previously, decode_collecting ran take_scalar_strings as a pre-pass to extract scalar text into a position-indexed Vec stored in Decoder.scalars, and decoding retrieved text via self.take_scalar(pos). Now the scalars field and the pre-pass are removed; a free function take_scalar(events, pos) uses mem::take to move text directly out of the EventKind::Scalar variant. All decode functions (decode_stream, decode_node, decode_node_inner, decode_mapping, decode_sequence, decode_sequence_item, decode_block_sequence) update their events parameter to &mut [Event<'input>] to support this in-place consumption.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main refactoring: moving scalar text extraction from a pre-pass side table into the decode loop itself.
Description check	✅ Passed	The description comprehensively explains the refactoring, rationale, implementation details, performance impact (−1.1% instruction reduction), and testing verification.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

codspeed-hq · 2026-06-23T08:45:42Z

Merging this PR will not alter performance

✅ 16 untouched benchmarks

_{Comparing frenck/decode-own-scalar-text (af4f65e) with main (73003bb)}

Copilot

Pull request overview

This PR refactors the fast-path decoder (src/decode/mod.rs) to eliminate the scalar “pre-pass” side table by moving each scalar’s text out of its EventKind::Scalar on-demand as the decoder consumes events, preserving the same zero-copy ownership transfer into the decoded Value tree while removing an extra full-stream walk and allocation.

Changes:

Remove the up-front scalar extraction pass and the decoder’s scalars: Vec<Option<Cow<...>>> side table.
Switch the decoding entrypoints from &[Event] to &mut [Event] so scalar text can be mem::taken directly from events during decoding.
Introduce a small helper (take_scalar) to move scalar text out of an event at a given position.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The helper is only ever called right after the event was matched as a scalar, so the non-scalar arm is unreachable. Returning an empty Cow there would mask a future invariant break by silently decoding the wrong event as ""; unreachable! makes it fail loudly instead.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings June 23, 2026 08:43

frenck added the performance Improving performance, not introducing new features. label Jun 23, 2026

Copilot started reviewing on behalf of frenck June 23, 2026 08:43 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Comment thread src/decode/mod.rs

frenck added 2 commits June 23, 2026 08:51

Merge branch 'main' into frenck/decode-own-scalar-text

af4f65e

Copilot AI review requested due to automatic review settings June 23, 2026 11:24

Copilot started reviewing on behalf of frenck June 23, 2026 11:26 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

frenck merged commit 40950f8 into main Jun 23, 2026
62 checks passed

frenck deleted the frenck/decode-own-scalar-text branch June 23, 2026 13:05

frenck mentioned this pull request Jun 23, 2026

📝 Drop the stale scalar-string side table note from the decode fuzz docs #94

Merged

15 tasks

github-actions Bot locked as resolved and limited conversation to collaborators Jun 25, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

⚡ Move scalar text out of events during decode, dropping the pre-pass#92

⚡ Move scalar text out of events during decode, dropping the pre-pass#92
frenck merged 3 commits into
mainfrom
frenck/decode-own-scalar-text

frenck commented Jun 23, 2026

Uh oh!

coderabbitai Bot commented Jun 23, 2026 •

edited

Loading

Walkthrough

Uh oh!

codspeed-hq Bot commented Jun 23, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

frenck commented Jun 23, 2026

Breaking change

Proposed change

Type of change

Additional information

Checklist

Uh oh!

coderabbitai Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

codspeed-hq Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jun 23, 2026 •

edited

Loading

codspeed-hq Bot commented Jun 23, 2026 •

edited

Loading