Skip to content

perf(sqlite-native): gate kv operation labels#4633

Draft
NathanFlurry wants to merge 1 commit into04-12-perf_sqlite-native_reuse_read_cache_for_partial_writesfrom
04-12-perf_sqlite-native_gate_kv_operation_labels
Draft

perf(sqlite-native): gate kv operation labels#4633
NathanFlurry wants to merge 1 commit into04-12-perf_sqlite-native_reuse_read_cache_for_partial_writesfrom
04-12-perf_sqlite-native_gate_kv_operation_labels

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

PR Review: perf(sqlite-native): gate kv operation labels

This is a focused, well-targeted performance improvement. The changes are correct and align with the codebase conventions.

Strengths

Removing std::env::var(RIVET_TRACE_SQL) calls on the hot path is the most impactful change. These were called on every KV operation and involve an OS syscall + string comparison each time. Eliminating them is a meaningful reduction in overhead.

Using format_args!() instead of format!() correctly avoids heap-allocating a String for the op label when DEBUG tracing is disabled. format_args! is lazy and only formats if the subscriber actually processes the event.

Removing eprintln! calls aligns with the CLAUDE.md guideline: never use eprintln!/println! for logging in Rust, always use tracing macros.

Suggestions

  1. Timing overhead is still unconditional. Instant::now() and .elapsed() are called on every KV operation even when debug tracing is disabled. For zero overhead when debug is off, gate behind tracing::enabled!(tracing::Level::DEBUG). Instant::now is cheap so this is a micro-optimization, but worth noting.

  2. Loss of RIVET_TRACE_SQL escape hatch. The env-var trace was useful for quick diagnostics without a configured tracing subscriber. Removing it means debug output now requires RUST_LOG=debug. This is the right direction per conventions, but worth noting in a dev guide.

  3. Pre-existing: format string missing space. get({key_count}keys) produces output like get(5keys) - same as original. Out of scope here, but a minor follow-up cleanup would be get({key_count} keys).

Minor

The cosmetic reformatting of read_cache_enabled, existing_chunk, and flushed improves readability with no logic changes.

Overall this is a clean, correct perf fix. The main actionable suggestion is optionally gating the Instant calls, though low priority. LGTM.

@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_reuse_read_cache_for_partial_writes branch from 18d65ab to 349925c Compare April 13, 2026 05:50
@NathanFlurry NathanFlurry force-pushed the 04-12-perf_sqlite-native_gate_kv_operation_labels branch from bde5c19 to 13c73bd Compare April 13, 2026 05:50
@NathanFlurry NathanFlurry force-pushed the 04-12-perf_sqlite-native_reuse_read_cache_for_partial_writes branch from 349925c to c7ac8f9 Compare April 13, 2026 07:03
@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
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