bpo-9303: Migrate sqlite3 module to _v2 API to enhance performance by palaviv · Pull Request #359 · python/cpython

palaviv · 2017-02-28T20:39:42Z

Use the _v2 API when possible.

…pare_v2

berkerpeksag

I think we can also avoid calling sqlite3_reset in _pysqlite_seterror if the v2 API is available.

berkerpeksag · 2017-02-28T21:50:21Z

I haven't checked yet, but I also think that the check for SQLITE_SCHEMA can also be eliminated if the v2 API is available:

cpython/Modules/_sqlite/cursor.c

Line 553 in c92e604

if (rc==SQLITE_SCHEMA) {

berkerpeksag · 2017-02-28T22:11:18Z

 Py_BEGIN_ALLOW_THREADS
+#ifdef HAVE_SQLITE3_OPEN_V2
+ rc = sqlite3_open_v2(database, &self->db,
+ SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL);


Style nitpick: FOO|BAR is more common than FOO | BAR in sqlite3 code base.

berkerpeksag · 2017-02-28T22:14:26Z

 }
 Py_BEGIN_ALLOW_THREADS
+#ifdef HAVE_SQLITE3_OPEN_V2
+ rc = sqlite3_open_v2(database, &self->db,


I think that there is an opportunity to simplify this whole SQLITE_OPEN_URI and HAVE_SQLITE3_OPEN_V2 dance.
(The use of Py_BEGIN_ALLOW_THREADS is a bit weird in line 111 and line 120.)

…re_v2

palaviv · 2017-03-01T19:41:20Z

I think we can also avoid calling sqlite3_reset in _pysqlite_seterror if the v2 API is available.

done

I haven't checked yet, but I also think that the check for SQLITE_SCHEMA can also be eliminated if the v2 API is available:
cpython/Modules/_sqlite/cursor.c
Line 553 in c92e604
if (rc==SQLITE_SCHEMA) {

I don't think we should do this because we can get SQLITE_SCHEMA if SQLITE_MAX_SCHEMA_RETRY tries to recompile will pass.

berkerpeksag

This looks pretty good to me. We just need an entry in Misc/NEWS (please add "Patch by Aviv Palivoda." too)

berkerpeksag · 2017-03-02T11:30:27Z

 (void)sqlite3_reset(st);
 }
+#endif
+


Nitpick: Please remove extra empty line.

berkerpeksag · 2017-03-02T11:32:41Z

 }
 Py_BEGIN_ALLOW_THREADS
+ /* No need to use sqlite3_open_v2 as sqlite3_open(filename, db) is the
+ same as sqlite3_open_v2(filename, db, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE, NULL)*/


Nitpick: FOO|BAR syntax instead of FOO | BAR.
Also, finish the line with a full stop and space:
/* [...] NULL). */

berkerpeksag · 2017-03-02T12:08:54Z

I don't think we should do this because we can get SQLITE_SCHEMA if SQLITE_MAX_SCHEMA_RETRY tries to recompile will pass.

Right, we can still get SQLITE_SCHEMA from sqlite3_step() (in legacy API we need to call sqlite3_reset() to get it), but in this case rc is the return value of sqlite3_reset(). The latter call is not needed when we have the new API since sqlite3_step() calls sqlite3_reset() for us.

palaviv · 2017-03-02T18:52:22Z

I have removed the reset but I left the recompile because we should recompile before calling sqlite3_step again.

berkerpeksag · 2017-03-02T22:17:26Z

 int errorcode;

- /* SQLite often doesn't report anything useful, unless you reset the statement first */
+#if SQLITE_VERSION_NUMBER >= 3003009


Shouldn't >= be < here?

Good catch!

serhiy-storchaka

Fix an error found by @berkerpeksag.

berkerpeksag

Thanks, Aviv! I will wait for Serhiy's approval and merge this.

Closes#358

Converts 12 BasicBlock-related extern "C" wrapper functions in lir_c_api.cpp to static inline definitions in lir_c_api.h. Each wrapper was a thin reinterpret_cast + field access; layout-pin static_asserts in lir_instr_c_verify.cpp guarantee LirBasicBlock (C) and BasicBlock (C++) field offsets match, making direct field access via cast safe across both languages. Files: M Python/jit/lir/lir_c_api.h +82 / -19 (12 static inline defs added) M Python/jit/lir/lir_c_api.cpp +6 / -68 (12 wrapper bodies removed) Net: -75 / +81 = +6 LOC (header inline definitions slightly larger than collapsed C++ bodies due to per-function comment + cast pattern). Inlined functions (all static inline in lir_c_api.h): jit_lir_block_num_preds -> ((const LirBasicBlock*)b)->num_preds_ jit_lir_block_get_pred -> ((const LirBasicBlock*)b)->predecessors_[i] jit_lir_block_num_succs -> ((const LirBasicBlock*)b)->num_succs_ jit_lir_block_get_succ -> ((const LirBasicBlock*)b)->successors_[i] jit_lir_block_get_last_instr -> ((const LirBasicBlock*)b)->instr_tail_ jit_lir_block_get_first_instr -> ((const LirBasicBlock*)b)->instr_head_ jit_lir_block_num_instrs -> ((const LirBasicBlock*)b)->num_instrs_ jit_lir_block_get_false_succ -> ((const LirBasicBlock*)b)->successors_[1] jit_lir_block_get_section -> (int)((const LirBasicBlock*)b)->section_ jit_lir_block_set_section -> ((LirBasicBlock*)b)->section_ = ... jit_lir_block_get_id -> ((const LirBasicBlock*)b)->id_ jit_lir_block_get_instr_at -> linked-list traversal via instr_head_/next_ Caller audit (preserved by header-inline): cold_block_marker.c: 14 sites dce.c: 4 sites blocksorter.c: 4 sites printer_c.c: 11 sites verify.c: 1 site TOTAL: 34 caller sites, ALL preserved by inline (same function names, same signatures, same return semantics). Behavior preservation: - Field offsets: guaranteed by static_asserts in lir_instr_c_verify.cpp (specifically: id_, successors_, instr_head_, section_ offsets all cross-validated) - num_succs_ / num_preds_ : direct field reads vs C++ predecessors().size() — C++ BlockSpan::size() returns the same num_preds_ field (block.h:24) - get_false_succ : preserves original no-bounds-check semantic (original used successors_[1] without check; matches inline version) - set_section : original called BasicBlock::setSection() which is just section_ = section (block.h:183 confirmed); inline writes field directly - get_instr_at : C inline traverses instr_head_->next_ chain identically to C++ instructions().front() + cur->next_ loop .h-decl <-> .cpp-defn enumeration: COMPREHENSIVE (per c4 lesson). All 12 extern declarations in lir_c_api.h:40-51 mapped to definitions in lir_c_api.cpp:63-129; all 12 inlined; 0 missed; 0 dead methods to delete this round (all 12 have active callers per audit above). .h-inline <-> decl-visibility check: lir_c_api.h is self-contained for the inlined functions — uses LirBasicBlock + LirInstruction (declared in lir_types_c.h, included by lir_c_api.h:15) + JitLirBlock/JitLirInstr typedefs (defined later in same file). LirCodeSection used in set_section is also from lir_types_c.h. No new include needed. PRE-COMMIT BUILD VERIFIED (per supervisor 16:12:20Z compile-before-commit directive, first real test of the workflow per pythia python#359): Build command: cd Python/jit_build/build && cmake --build . --target jit -j 16 (then) cd cpython && make -j16 python Build trailer (verbatim, x86_64, 2026-05-12): [ 24%] Linking CXX static library libjit.a [100%] Built target jit clang++ ... -o python ... -Wl,--start-group Python/jit_build/build/libjit.a ... (link successful, exit 0) Warnings: 5 pre-existing (block-comment in reader_c.h:22, unused-private- field in phoenix_asm_wrapper.h:865 etc.); ZERO new warnings introduced. Binary post-build: stat: 2026-05-12 10:03:36 python sys.version: 3.12.13 (heads/phoenix-asm-integration:6aedfa8d1e, ...) NOTE: version string reports c5 (6aedfa8) because git HEAD at compile-time was c5; binary CONTAINS the c6 inline code (in libjit.a) but the embedded commit-string lags by one commit. Testkeeper Tier 1 rebuild post-c6-commit will produce a binary with c6 commit string. Working tree clean post-build (verified: only the 2 staged c6 files M). Pre-ARM64 APPROVE pending; ABBA waived (inline conversion of byte-equivalent field accessors, no executable-path semantic change beyond removed function- call overhead which the optimizer would have eliminated anyway via LTO).

Add optional use of sqlite3_open_v2, sqlite3_close_v2 and sqlite3_pre…
c92e604
…pare_v2

berkerpeksag reviewed Feb 28, 2017
View reviewed changes

brettcannon added CLA not signed and removed CLA not signed labels Feb 28, 2017

the-knights-who-say-ni added the CLA signed label Feb 28, 2017

berkerpeksag mentioned this pull request Mar 1, 2017
bpo-27645: Supporting native backup facility of SQLite #377
Closed

palaviv added 3 commits March 1, 2017 21:09

Use SQLITE_VERSION_NUMBER
8eccb84

Don't call sqlite3_reset in _pysqlite_seterror in using sqlite3_prepa…
5360795
…re_v2

Revert sqlite3_open change
9185cb9

berkerpeksag reviewed Mar 2, 2017
View reviewed changes

palaviv added 3 commits March 2, 2017 20:44

Don't reset the statment if using sqlite3_prepare_v2
f7dcd02

CR fixes
d4d9c4e

Add NEWS entry
64c3bb7

Merge remote-tracking branch 'upstream/master' into sqlite-v2-api
645d959

serhiy-storchaka approved these changes Mar 2, 2017
View reviewed changes

berkerpeksag reviewed Mar 2, 2017
View reviewed changes

serhiy-storchaka requested changes Mar 3, 2017
View reviewed changes

palaviv added 2 commits March 3, 2017 10:11

Fix ifdef in _pysqlite_seterror
f1ddaba

Merge remote-tracking branch 'upstream/master' into sqlite-v2-api
3157f4c

berkerpeksag approved these changes Mar 3, 2017
View reviewed changes

serhiy-storchaka approved these changes Mar 3, 2017
View reviewed changes

berkerpeksag merged commit 86a6705 into python:masterMar 3, 2017

palaviv mentioned this pull request Mar 4, 2017
bpo-26187: Tests for sqlite3 trace callback #434
Closed

coleifer mentioned this pull request Jul 22, 2019
Is sweepea using sqlite3_close_v2 ? coleifer/sweepea#1
Closed

jaraco pushed a commit that referenced this pull request Dec 2, 2022
Use Python 3.7.8 (#359)
5f46257
Closes#358

Uh oh!

Uh oh!

Conversation

palaviv commented Feb 28, 2017

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Feb 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksagFeb 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palaviv commented Mar 1, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Mar 2, 2017

Uh oh!

palaviv commented Mar 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

berkerpeksagFeb 28, 2017•
edited
Loading

palaviv commented Mar 1, 2017•
edited
Loading