Improve wrappers#52
Merged
Merged
Conversation
Instead of `mdb_put(txn.handle, dbi.handle, ...)`, define
unsafe_convert/cconvert for the handle wrapper types and write
`mdb_put(txn, dbi, ...)`. Routing centralizes in four lines:
Base.unsafe_convert(::Type{Ptr{MDB_env}}, e::Environment) = e.handle
Base.unsafe_convert(::Type{Ptr{MDB_txn}}, t::Transaction) = t.handle
Base.unsafe_convert(::Type{Ptr{MDB_cursor}}, c::Cursor) = c.handle
Base.cconvert(::Type{MDB_dbi}, d::DBI) = d.handle
The first three follow the CUDA.jl pattern: cconvert isn't needed because
Base's default `cconvert(::Type{<:Ptr}, x) = x` already returns the
wrapper for ccall to GC-preserve, then dispatches to our unsafe_convert.
DBI needs cconvert because MDB_dbi is Cuint (by-value), not a pointer,
so the default falls through to convert which isn't defined for DBI.
No allocation regression: cconvert/unsafe_convert just hand back the
existing wrapper. Refs around MDB_val structs (the local-scope boxes for
key/val args) and their GC.@preserve guards stay explicit at the use
site, since the ptr inside an MDB_val is opaque to GC and can't be
rooted by ccall.
Drop two pieces of dead code: const MBDValue (unused typo'd alias) and
toref(::Ptr{Nothing}) (unreachable — delete! short-circuits on
val === C_NULL before calling toref).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two related cleanups:
1. Drop `toref` and the `[v]` one-element-array trick. Bitstype scalars
are wrapped in `Ref(v)` at the call site (smaller box, honest type);
strings/arrays pass through (already heap-rooted with stable data
pointers). `MDBValue` now dispatches on `String` / `AbstractArray{T}`
/ `Base.RefValue{T}` and pulls the data pointer via `Base.unsafe_convert`
— the same primitive ccall uses to lower `Ref{T}`/`Array{T}`/`String`
arguments.
2. Add `Base.cconvert(::Type{Ptr{MDB_val}}, x::MDB_val) = Ref(x)`. Clang.jl
has no codegen option to emit `Ref{T}` instead of `Ptr{T}` (verified
exhaustively against the [codegen] block in Clang's generator.toml),
but the BLAS-style auto-box convenience that `Ref{T}` sigs provide
is just a cconvert default — overriding cconvert for `Ptr{MDB_val}`
gives the same lowering. This is the documented Base extension idiom
(see refpointer.jl:178 for the same pattern on Ptr{Ptr} args).
Net effect: callers write `mdb_put(..., MDBValue(rkey), ...)` instead
of `mdb_put(..., Ref(MDBValue(rkey)), ...)`. The compiler can now
stack-allocate the box because it doesn't escape past the ccall:
`put!`/`get` go from 128/64 bytes per call to 0 bytes per call.
Test: ErrorException → MethodError on `MDBValue(::TestType)` since the
generic bitstype-error fallback is gone (callers must now wrap in Ref);
MethodError is the more idiomatic signal anyway.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Push the per-call-site `isbitstype(typeof(v)) ? Ref(v) : v` conditional
plus the explicit `Ref(MDBValue(...))` boxing and `GC.@preserve` rkey/rval
guards into a single `cconvert(::Type{Ptr{MDB_val}}, x)` dispatch table
in common.jl. Each method returns an `MDBArg{D}` wrapper that holds both
the `Ref{MDB_val}` box and a reference to the data buffer the box's
mv_data field aliases — ccall's automatic GC.@preserve over the cconvert
result roots both transitively, so callers don't need any manual rooting.
Dispatch:
String / Array / Base.RefValue → wrap in MDBArg
bare bitstype (Int, Float, ...) → Ref-wrap, then MDBArg
MDB_val (e.g. delete!'s empty val) → Ref(x) auto-box
Ref{MDB_val} (iterator/get out-param) → passthrough
Net effect on callers — `put!`, `get`, `delete!` (transaction and cursor
variants) and the `LMDBDict` haskey/get/get!/get(f)/get!(f) functions all
collapse to a single line that just hands the user value straight to
mdb_put/mdb_get/mdb_del. No conditional, no explicit box, no preserve.
Allocations measured at 0 bytes/call across put/get/delete for int,
string, array, and Ref inputs — Julia's escape analysis stack-allocates
the MDBArg + Ref{MDB_val} since they don't escape past the ccall.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Status-returning bindings in liblmdb.jl now auto-throw LMDBError on a non-zero return; an `unchecked_*` companion returns the raw status for callers that need to branch on it (e.g. MDB_NOTFOUND). Tier-2 wrappers drop the now-redundant `check(...)` calls, and dicts.jl switches its NOTFOUND-aware paths to `unchecked_mdb_get`. `res/wrap.jl` post-processes the Clang-generated bindings to apply `@checked` automatically on regen, with a denylist for the few `Cint`- returning functions (mdb_cmp, mdb_dcmp, mdb_env_get_maxkeysize) whose return value is not a status code. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Each handle wrapper now:
- has a single inner constructor that attaches a finalizer
- closes / aborts idempotently (safe to call after explicit cleanup
or on a never-opened handle)
- keeps its parent reference in the struct (Transaction.env,
Cursor.txn) — both to expose via env(txn) / transaction(cur) and
to ensure parents outlive children under GC
Closes a real bug: an abandoned write txn (e.g. from a `for`-`break`
out of a future LMDBDict iterator, or any error path that doesn't
hit `try`/`finally`) used to hold LMDB's exclusive write mutex until
process exit, deadlocking subsequent write txns. The finalizer aborts
it.
The view-handle ctors (Environment(::Ptr), Transaction(::Ptr) without
parent) are gone: env(txn) / transaction(cur) now return the stored
parent reference instead of wrapping the C-side `mdb_*_env` /
`mdb_cursor_txn` pointers — same answer, no double-ownership risk.
Pattern follows CUDA.jl's CuStream (single inner ctor, parent ref in
struct).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Removes the duplicate `show` import and the unused `keys` import; drops `import Base.Iterators: drop` so the `drop(::Transaction, ::DBI)` method is a fresh name in the LMDB module instead of (accidentally) extending `Iterators.drop`. Tightens `Transaction.env`, `Cursor.txn`, `Cursor.dbi` to non-optional fields. Every constructor in the package already passes a real wrapper; the `Nothing` half was vestigial from upstream's pointer-only round-trip and only added field-access dispatch cost. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per `lmdb.h`, write-txn cursors are freed by `mdb_txn_commit` / `mdb_txn_abort`, and `mdb_cursor_close` afterwards is undefined. Block-style usage (`open(txn, dbi) do cur ... end`) and `LMDBDict` iteration both close cursors before the txn ends, but a user who explicitly commits while a cursor is still alive would otherwise trigger UB once the cursor's finalizer ran. The Cursor already holds a reference to its parent Transaction; check the wrapper's handle state and skip the LMDB call if the parent is gone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mdb_txn_commit frees the txn handle whether it returns success or an error (per lmdb.h), so when the @checked wrapper threw on a non-zero status, txn.handle still pointed at the freed handle and the finalizer would re-abort it. Null the wrapper out before letting the check throw. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
- Coverage 77.31% 73.42% -3.90%
==========================================
Files 8 9 +1
Lines 529 538 +9
==========================================
- Hits 409 395 -14
- Misses 120 143 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The old `MDBValue(::String)` / `MDBValue(::AbstractArray)` /
`MDBValue(::Base.RefValue)` constructors called `Base.unsafe_convert`
to take a raw data pointer eagerly, then stored it in the returned
`MDB_val` struct. The pointer was correctly rooted *in practice*
because the caller's `MDBArg` wrapper held the buffer, but the call
to `unsafe_convert` happened outside of any preservation scope —
violating the Julia GC contract that `unsafe_convert` should only
produce a pointer while ccall is preserving the source.
Push the extraction into a small set of `unsafe_convert(::Ptr{MDB_val},
::MDBArg{D})` methods (one per `D`). These run while ccall is
preserving the `MDBArg`, so the buffer is provably alive at the
moment we ask it for a pointer.
Drop the unsafe `MDBValue(::String)` etc. constructors; the iterator's
buffer-to-MDB_val needs are served by a private `_mdb_val_for(::Vector)`
that's only called inside an explicit `GC.@preserve`. The cursor
iterator now fills `mdb_key_ref` lazily inside the same preserve
block that wraps the ccall.
Rewrites `test/common.jl` to test the cconvert / unsafe_convert
chain — and along the way demonstrates the unsafety by example: the
test helper has to pass the loaded `MDB_val` to a callback inside
`GC.@preserve`, because reading through `mv_data` after leaving the
preserve scope is exactly the bug the carrier exists to prevent.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Collaborator
Author
|
For anybody following along, I'm doing things a little more loosely than I normally would, just to get the package in a better state. I'll do a proper pass cleaning up LLMisms as soon as all the fixes have landed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@checkedmacro auto-throws on non-zero status;unchecked_*companion forMDB_NOTFOUND-tolerant call sites (mimicking CUDA.jl)unsafe_convert/cconvertplumbing for the handle wrappers andMDB_val; call sites passEnvironment/Transaction/Cursordirectly, and PR Fix correctness bugs #50'sRef(MDBValue(toref(v)))+GC.@preservetriplets collapse into a single argument. 0 bytes per put/get/delete.Environment/Transaction/Cursor. Fixes a bug where an abandoned write txn held LMDB's exclusive write mutex until process exit. Idempotent close; safe across an explicit txn commit/abort; safe across a failedmdb_txn_commit(which frees the handle per lmdb.h).Base.showfor the handle types.