Skip to content

perf: leak names in BtreeError InvalidRootPage/MasterEntryNotFound (#679) - #680

Merged
iheitlager merged 1 commit into
mainfrom
fix/679-btree-error-shrink-needs-drop
Aug 31, 2026
Merged

perf: leak names in BtreeError InvalidRootPage/MasterEntryNotFound (#679)#680
iheitlager merged 1 commit into
mainfrom
fix/679-btree-error-shrink-needs-drop

Conversation

@iheitlager

Copy link
Copy Markdown
Member

Summary

Follow-up to #677/#679: BtreeError::InvalidRootPage/MasterEntryNotFound
owned a String, making BtreeError non-trivially-droppable and forcing an
out-of-line drop_in_place::<BtreeError> on every Result<_, BtreeError>
produced inside TableCursor::seek's hot per-page binary search, paid
log2(N) times per row.

Both variants only fire on rare sqlite_master corruption/lookup paths
(bad rootpage, missing entry on delete), so the fix leaks the name
(Box::leak(name.to_string().into_boxed_str())&'static str) instead of
owning a String. A few dozen leaked bytes on an error path that typically
propagates straight to the caller is a fine trade for removing drop glue
from the hot-path type.

Bench results (make -C tests/performance crud, filtered to read_join, bench_1mb.db, 16700 rows)

ns/row
before ~210.7
after ~166.9
recovered ~43.9 ns/row (~56% of the ~78ns/row gap from #679)

Criterion reported a -20.8% mean-time improvement for read_join (p < 0.05).

Note on the needs_drop acceptance criterion

std::mem::needs_drop::<BtreeError>() is still true after this
change — not false as the ticket's acceptance criteria literally ask for.
Root cause: BtreeError::Pager(PagerError) transitively owns a
path: String via PagerError::HotJournal/PagerError::Wal, so the whole
enum keeps drop glue regardless of what's done to InvalidRootPage/
MasterEntryNotFound. Boxing/leaking String fields specifically in
BtreeError's own two variants was the only thing in scope for this
ticket; fully eliminating needs_drop would require the same treatment on
PagerError (touches pager.rs in ~4 places + pager/checkpoint.rs),
which is a materially larger, separate change.

Per the ticket's own guidance ("if a measurable gap remains ... note it
rather than chasing it further"), I'm not expanding scope here. The bench
result above shows this fix alone recovers a solid majority of the
regression even though needs_drop stays true — LLVM's actual codegen
decision (drop shim size/complexity, not just the boolean) is what mattered
in practice. Happy to file a follow-up ticket for the PagerError side if
the remaining ~59ns/row-to-oracle gap on read_join is worth chasing.

Test plan

  • cargo test — full suite passes
  • cargo clippy --all-targets -- -D warnings — clean
  • cargo fmt --check — clean
  • btree::error/btree::master unit tests updated for the new field
    types and pass
  • Bench comparison run and documented above

Spend: matched estimate (small, mechanical).

Closes #679

Replaces the owned `String` in these two cold sqlite_master
corruption/lookup variants with a leaked `&'static str`, removing their
drop glue. This is intentional: both variants are only constructed on
rare error paths (corrupt rootpage, missing schema entry during
delete), so leaking the handful of bytes is a deliberate trade against
the drop cost these variants otherwise impose on every
Result<_, BtreeError> in TableCursor::seek's hot per-page binary
search.

std::mem::needs_drop::<BtreeError>() still returns true overall
because BtreeError::Pager(PagerError) transitively owns a
`path: String` (PagerError::HotJournal/Wal) -- fixing that is a
separate, out-of-scope change (see PR description).

Refs: follow-up to #677, closes #679
@iheitlager
iheitlager force-pushed the fix/679-btree-error-shrink-needs-drop branch from 13576f3 to 1c958e8 Compare August 31, 2026 09:48
@iheitlager
iheitlager merged commit 179ed1a into main Aug 31, 2026
6 checks passed
@iheitlager
iheitlager deleted the fix/679-btree-error-shrink-needs-drop branch August 31, 2026 09:52
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.

perf: shrink BtreeError to eliminate needs_drop overhead in TableCursor::seek's hot path

1 participant