gosqlite: replace uintptr handles with Go pointers #120
Conversation
raggi
requested review from
bradfitz and
Copilot
and removed request for
Copilot
What regular tooling? function again how? what's not currently functioning? |
bradfitz
left a comment
bradfitz
left a comment
There was a problem hiding this comment.
No memprofile alloc differences?
There was a problem hiding this comment.
Pull request overview
This PR removes the uintptr indirection layer for sqlite3_stmt handles, replacing them with direct Go pointers. The change was motivated by improvements in the Go toolchain that eliminated the allocation overhead that originally necessitated the uintptr workaround. Benchmark results confirm that the refactoring maintains the same allocation profile while simplifying the code and enabling better tooling support.
Key Changes
- Removed the
cStmtwrapper struct and associated helper methods that managed uintptr-based statement handles - Replaced direct calls to wrapper functions with calls to the underlying SQLite C API functions
- Updated nil checks to use proper pointer comparisons instead of zero-value checks
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cgosqlite/cgosqlite.h | Removed uintptr typedefs and updated all C helper functions to accept sqlite3_stmt* pointers directly; removed now-redundant wrapper functions for bind operations |
| cgosqlite/cgosqlite.go | Removed cStmt abstraction, changed Stmt.stmt field to *C.sqlite3_stmt, and updated all 40+ method implementations to use direct pointer access and native SQLite API calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AIUI the race detector is blinded by uintptr reads. |
full benchstat output added to the commit |
Removed the uintptr indirection for sqlite3_stmt, replacing the uintptr with a plain Go pointer. Since 34bef2d was authored the toolchain has changed such that the indirection is no longer necessary to avoid allocations. Restoring regular pointers will enable regular tooling to function again and simplifies the code. ``` goos: darwin goarch: arm64 pkg: github.com/tailscale/sqlite cpu: Apple M4 Max │ /tmp/uintptr-bench.txt │ /tmp/no-cstmt-bench.txt │ │ sec/op │ sec/op vs base │ WALHookAndExec-16 1.362µ ± 1% 1.345µ ± 1% -1.25% (p=0.000 n=10) Persist-16 1.186µ ± 3% 1.145µ ± 1% -3.46% (p=0.000 n=10) QueryRows100MixedTypes-16 20.05µ ± 1% 20.60µ ± 0% +2.73% (p=0.000 n=10) EmptyExec-16 219.8n ± 0% 222.8n ± 1% +1.39% (p=0.000 n=10) BeginTxNoop-16 5.226µ ± 0% 5.289µ ± 1% +1.21% (p=0.000 n=10) geomean 2.061µ 2.063µ +0.10% │ /tmp/uintptr-bench.txt │ /tmp/no-cstmt-bench.txt │ │ B/op │ B/op vs base │ WALHookAndExec-16 32.00 ± 0% 32.00 ± 0% ~ (p=1.000 n=10) ¹ Persist-16 504.0 ± 0% 504.0 ± 0% ~ (p=1.000 n=10) ¹ QueryRows100MixedTypes-16 2.766Ki ± 0% 2.766Ki ± 0% ~ (p=1.000 n=10) ¹ EmptyExec-16 32.00 ± 0% 32.00 ± 0% ~ (p=1.000 n=10) ¹ BeginTxNoop-16 640.0 ± 0% 640.0 ± 0% ~ (p=1.000 n=10) ¹ geomean 247.9 247.9 +0.00% ¹ all samples are equal │ /tmp/uintptr-bench.txt │ /tmp/no-cstmt-bench.txt │ │ allocs/op │ allocs/op vs base │ WALHookAndExec-16 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹ Persist-16 11.00 ± 0% 11.00 ± 0% ~ (p=1.000 n=10) ¹ QueryRows100MixedTypes-16 107.0 ± 0% 107.0 ± 0% ~ (p=1.000 n=10) ¹ EmptyExec-16 1.000 ± 0% 1.000 ± 0% ~ (p=1.000 n=10) ¹ BeginTxNoop-16 13.00 ± 0% 13.00 ± 0% ~ (p=1.000 n=10) ¹ geomean 6.870 6.870 +0.00% ¹ all samples are equal ``` Updates tailscale/corp#9919
2 participants
Removed the uintptr indirection for sqlite3_stmt, replacing the uintptr with a plain Go pointer. Since 34bef2d was authored the toolchain has changed such that the indirection is no longer necessary to avoid allocations. Restoring regular pointers will enable regular tooling to function again and simplifies the code.
Updates tailscale/corp#9919