Skip to content

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

Draft
NathanFlurry wants to merge 1 commit into04-12-perf_sqlite-native_gate_kv_operation_labelsfrom
04-12-perf_sqlite-native_avoid_cloning_cached_read_chunks
Draft

perf(sqlite-native): avoid cloning cached read chunks#4634
NathanFlurry wants to merge 1 commit into04-12-perf_sqlite-native_gate_kv_operation_labelsfrom
04-12-perf_sqlite-native_avoid_cloning_cached_read_chunks

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 13, 2026

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

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

@claude
Copy link
Copy Markdown

claude bot commented Apr 13, 2026

Review: perf(sqlite-native): avoid cloning cached read chunks\n\nThe optimization is correct and well-targeted. The core idea — storing &[u8] slices in buffered_chunks instead of owned Vec<u8> clones — eliminates allocations on read-cache and dirty-buffer hits.\n\n### Correctness\n\nRead-cache slices (cached.as_slice()): The borrow of state.read_cache through buffered_chunks ends when the chunk loop exits. The mutable borrow for read_cache.insert() comes after the loop. NLL handles this correctly, so there is no borrow conflict.\n\nDeferred cache population: Moving read_cache.insert() to after the chunk loop is semantically equivalent to the original. resp only contains keys that were not already in the cache (those keys were never added to chunk_keys_to_fetch), so the same entries are written either way.\n\n### Issues\n\nDouble lookup for dirty-buffer chunks (minor perf regression): The prep loop now does contains_key to skip KV fetching, and the retrieval loop does a second get to obtain the slice. Storing the slice directly in buffered_chunks from the prep loop would close the gap and let the retrieval loop drop the if state.batch_mode branch entirely.\n\nLifetime invariant not documented: buffered_chunks holds slices that borrow from state.read_cache. A short comment near the declaration would make this explicit and prevent confusion for future readers.\n\n### Nits\n\n- read_start is still shadowed twice later in the function (pre-existing).\n- The new comment about resp being empty is accurate and helpful.

@NathanFlurry NathanFlurry force-pushed the 04-12-perf_sqlite-native_gate_kv_operation_labels branch from 5482e7d to bde5c19 Compare April 13, 2026 05:38
@NathanFlurry NathanFlurry force-pushed the 04-12-perf_sqlite-native_avoid_cloning_cached_read_chunks branch 2 times, most recently from ff117f9 to e25c1b6 Compare April 13, 2026 05:50
@NathanFlurry NathanFlurry force-pushed the 04-12-perf_sqlite-native_gate_kv_operation_labels branch from 13c73bd to 768d9da Compare April 13, 2026 07:03
@NathanFlurry NathanFlurry force-pushed the 04-12-perf_sqlite-native_avoid_cloning_cached_read_chunks branch from e25c1b6 to 532364f Compare April 13, 2026 07:03
@NathanFlurry NathanFlurry force-pushed the 04-12-perf_sqlite-native_gate_kv_operation_labels branch from 768d9da to b37f244 Compare April 13, 2026 21:07
@NathanFlurry NathanFlurry force-pushed the 04-12-perf_sqlite-native_avoid_cloning_cached_read_chunks branch from 532364f to 22df032 Compare April 13, 2026 21:07
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.

1 participant