perf(sqlite-native): avoid cloning cached read chunks

[NathanFlurry]

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore(engine): publish engine bases in ci #4649 : 3 dependent PRs (#4650 , #4652 , #4666 )
• chore: lockfile lefthook #4647
• fix(rivetkit): surface native sqlite kv errors to callers #4645
• fix(rivetkit): prevent sleep races during disconnect and db work #4644
• fix(rivetkit): isolate engine envoys and propagate startup failures #4643
• test(rivetkit): remove wasm sqlite-only fixtures and coverage #4642
• chore(sqlite-vfs): remove wasm sqlite packages and workspace wiring #4641
• chore(rivetkit): remove wasm sqlite runtime #4614
• fix(sqlite-native): restore native startup kv preload #4639
• fix(sqlite-native): delete metadata before chunk range #4638
• fix(sqlite-native): keep truncate cache coherent #4637
• fix(sqlite-vfs): use delete range for truncate cleanup #4636
• fix(sqlite-native): restore kv error hook #4635
• perf(sqlite-native): avoid cloning cached read chunks #4634 👈 (View in Graphite)
• perf(sqlite-native): gate kv operation labels #4633
• perf(sqlite-native): reuse read cache for partial writes #4632
• perf(sqlite-native): remove delete file existence probe #4631
• fix(sqlite-native): gate native read cache behind env var #4630
• fix(pegboard): isolate runner config dc lookup failures #4625 : 1 other dependent PR (#4626 )
• docs(pegboard): note runner config upsert split write risk #4624
• fix(pegboard): unpack global runner config keys correctly #4623
• fix(pegboard-gateway): enforce tunnel message state #4622
• fix(pegboard-runner): clear terminal tunnel routes #4621
• fix(pegboard): skip protocol version keys in runner pool backfill #4620
• fix(pegboard): create missing normal runner configs #4619
• fix(epoxy): reuse cached optimistic value #4618
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

---

PR Review: perf(sqlite-native): avoid cloning cached read chunks

Overview

This PR optimizes kv_io_read in the native SQLite VFS by eliminating heap allocations when serving chunks from the read cache or dirty buffer. The change is well-targeted and correctness-preserving.

---

What Changed

1. Avoids cloning read-cached chunks: buffered_chunks changes from HashMap<usize, Vec<u8>> to HashMap<usize, &[u8]>, so chunks already in read_cache are referenced in place instead of cloned. For 4 KiB chunks this is a meaningful allocation reduction on cache-heavy read workloads.

2. Avoids cloning dirty-buffer chunks: Previously, a batch-mode hit also cloned the chunk into buffered_chunks. Now the dirty buffer is accessed directly in the read loop via a second get, trading an O(log n) BTreeMap lookup for a 4 KiB clone. This is a sound tradeoff.

3. Moves read-cache population after the copy loop: Functionally equivalent since the newly fetched data is only needed for subsequent reads. The added comment explaining the empty-resp no-op case is clear and helpful.

---

Issues / Observations

Potential soundness concern in the unsafe context (minor)

buffered_chunks holds &[u8] slices pointing into the Vec<u8> heap data owned by read_cache. When read_cache.insert later causes the HashMap to rehash, it moves the Vec<u8> fat-pointers but not their heap-allocated data, so the &[u8] slices remain valid. This is safe in practice, but relies on implementation details of how Vec<u8> and HashMap interact.

Since this is inside unsafe extern "C" code with raw-pointer-derived references (the borrow checker cannot enforce the lifetime relationship), a brief safety comment at the mutable read_cache insertion site would help future maintainers understand why this is sound. Something like:

// Safety: buffered_chunks is fully consumed above before this mutable borrow of read_cache.
if let Some(read_cache) = state.read_cache.as_mut() {

Double lookup of dirty buffer in batch mode (acceptable tradeoff)

For chunks in dirty_buffer, the new code does two lookups: contains_key in the setup loop and get in the read loop. The prior code did one get but then cloned the chunk data. Avoiding a 4 KiB clone is worth an extra O(log n) BTreeMap lookup, so the tradeoff is sound.

Comment style nit

The new comment uses a parenthetical: // Skip fetching chunks already present in the dirty buffer (batch mode) or read cache. Per project CLAUDE.md conventions, parenthetical structures should be avoided. A cleaner form: // Skip fetching chunks already present in the dirty buffer or read cache.

No tests added

The checklist item for tests is unchecked. Since this is a pure refactor with no behavioural change, existing driver test coverage for reads in batch mode and read-cache hits should be sufficient. Worth confirming those paths are exercised before merge.

---

Summary

The optimization is correct and well-scoped. The main thing worth addressing before merge is a Safety: comment at the mutable read_cache insertion site to make the unsafe invariant explicit. Everything else is minor.