| msg399939 - (view) |
Author: Erlend E. Aasland (erlendaasland) *  |
Date: 2021-08-20 07:33 |
| Ref. Serhiy's in bpo-43350: "Maybe the code could be rewritten in more explicit way and call pysqlite_statement_reset() only when it is necessary [...]" Currently, we try to reset statements in all "statement exit" routes. IMO, it would be cleaner to just reset statements when we really need to: 1. before the first sqlite3_step() every time we (re)execute a query 2. at cursor exit, if there's an active statement (3. in pysqlite_do_all_statements() ... see bpo-44092) This will make the code easier to follow, and it will minimise the number of resets. The current patch is pretty small: 7 insertions(+), 33 deletions(-) Pro: - less lines of code, less maintenance - cleaner exit paths - optimise SQLite API usage Con: - code churn If this is accepted, PR 25984 of bpo-44073 will be easier to land and review :) |
|
|
| msg399984 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-08-20 18:05 |
| I did a quick count of sqlite3_reset()s in the sqlite3 test suite: - main: 2976 calls - PR 27844: 1730 calls Since we never call sqlite3_reset() with a NULL pointer, all sqlite3_reset() calls we execute hold the SQLite db mutex; reducing the number of sqlite3_reset() calls reduces the number of times we need to hold the mutex. |
|
|
| msg401186 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-09-06 22:35 |
| In , item 2 lacks one more "reset path": > 2. at cursor exit, if there's an active statement Rewording this to: 2. when a statement is removed from a cursor; that is either at cursor dealloc, or when the current statement is replaced. |
|
|
| msg402312 - (view) |
Author: Pablo Galindo Salgado (pablogsal) *  |
Date: 2021-09-21 11:20 |
| New changeset 050d1035957379d70e8601e6f5636637716a264b by Erlend Egeberg Aasland in branch 'main': bpo-44958: Only reset `sqlite3` statements when needed (GH-27844) https://github.com/python/cpython/commit/050d1035957379d70e8601e6f5636637716a264b |
|
|
| msg402317 - (view) |
Author: miss-islington (miss-islington) |
Date: 2021-09-21 13:16 |
| New changeset 3e3ff09058a71b92c32d1d7dc32470c0cf49300c by Erlend Egeberg Aasland in branch 'main': bpo-44958: Fix ref. leak introduced in GH-27844 (GH-28490) https://github.com/python/cpython/commit/3e3ff09058a71b92c32d1d7dc32470c0cf49300c |
|
|
| msg402319 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-09-21 14:25 |
| Now that pysqlite_statement_reset() is only used in cursor.c, I suggest to move it to cursor.c and make it a static function. |
|
|
| msg402402 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-09-21 22:33 |
| > Now that pysqlite_statement_reset() is only used in cursor.c, I suggest to move it to cursor.c and make it a static function. I'll wait until PR 25984 is merged before opening a PR for relocating pysqlite_statement_reset(). |
|
|
| msg402423 - (view) |
Author: Ken Jin (kj) * |
Date: 2021-09-22 12:03 |
| Erlend, I suspect that 050d1035957379d70e8601e6f5636637716a264b may have introduced a perf regression in pyperformance's sqlite_synth benchmark: https://speed.python.org/timeline/?exe=12&base=&ben=sqlite_synth&env=1&revs=50&equid=off&quarts=on&extr=on The benchmark code is here https://github.com/python/pyperformance/blob/main/pyperformance/benchmarks/bm_sqlite_synth.py. |
|
|
| msg402424 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-09-22 12:11 |
| Ouch, that's quite a regression! Thanks for the heads up! I'll have a look at it right away. |
|
|
| msg402425 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-09-22 12:19 |
| I'm unable to reproduce this regression on my machine (macOS, debug build, no optimisations). Are you able to reproduce, Ken? |
|
|
| msg402427 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-09-22 12:27 |
| > I'm unable to reproduce this regression on my machine (macOS, debug build, no optimisations) [...] Correction: I _am_ able to reproduce this. |
|
|
| msg402490 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-09-23 10:51 |
| Explicitly resetting statements when we're done with them removes the performance regression; SQLite works more efficient when we keep the number of non-reset statements low. |
|
|
| msg402677 - (view) |
Author: Erlend E. Aasland (erlendaasland) * |
Date: 2021-09-26 20:11 |
| I'll revert PR 27844 for now (except the tests). Since SQLite works better when we keep the number of non-reset statements to a minimum, we need to ensure that we reset statements when we're done with them (sqlite3_step() returns SQLITE_DONE or an error). Before doing such a change, we should clean up _pysqlite_query_execute() so we don't need to sprinkle that function with pysqlite_statement_reset's. I plan to do this before attempting to clean up reset usage again. |
|
|
| msg402683 - (view) |
Author: Pablo Galindo Salgado (pablogsal) * |
Date: 2021-09-26 21:24 |
| New changeset 7b88f63e1dd4006b1a08b9c9f087dd13449ecc76 by Erlend Egeberg Aasland in branch 'main': bpo-44958: Revert GH-27844 (GH-28574) https://github.com/python/cpython/commit/7b88f63e1dd4006b1a08b9c9f087dd13449ecc76 |
|
|