Skip to content

bpo-42862: Use functools.lru_cache iso. _sqlite.Cache in sqlite3 module #24203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 18 commits into from
Jun 3, 2021
Merged

bpo-42862: Use functools.lru_cache iso. _sqlite.Cache in sqlite3 module #24203

merged 18 commits into from
Jun 3, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jan 12, 2021 •
edited by bedevere-bot
Loading

@erlend-aasland erlend-aasland requested review from berkerpeksag and a team as code owners January 12, 2021 23:24
@erlend-aasland erlend-aasland mentioned this pull request Jan 12, 2021
Closed
@erlend-aasland
Copy link
Contributor Author

cc. @berkerpeksag, @rhettinger, @serhiy-storchaka.

@erlend-aasland erlend-aasland marked this pull request as draft January 14, 2021 00:41
@erlend-aasland erlend-aasland marked this pull request as ready for review January 21, 2021 20:43
@erlend-aasland erlend-aasland mentioned this pull request Feb 2, 2021
Merged
@erlend-aasland
Copy link
Contributor Author

@rhettinger Would you mind reviewing the use of functools.lru_cache in this PR?

@erlend-aasland erlend-aasland mentioned this pull request Feb 10, 2021
Merged
@erlend-aasland erlend-aasland force-pushed the sqlite-cache branch 2 times, most recently from 2cceb7f to ec6485f Compare February 21, 2021 19:56
@erlend-aasland
Copy link
Contributor Author

FYI @berkerpeksag, I've rebased onto master again, bco. #24801.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented May 2, 2021 •
edited
Loading

Rebased onto master to resolve conflicts. Also added a tiny optimisation: increase the default cache size from 100 to 128. I'm guessing it is way to late to get this into 3.10 now :)

@erlend-aasland erlend-aasland force-pushed the sqlite-cache branch 2 times, most recently from 6ceadcd to c561bab Compare May 2, 2021 20:49
@erlend-aasland
Copy link
Contributor Author

@pablogsal, two questions:

  1. There's some issues with the Windows tests. I've added a workaround in f1f023b, but I don't understand why this is needed. One possible reason could be that SQLite is holding on to TESTFN after the test is finished. I haven't devoted too much time to investigate this yet.
  2. Is it ok to request Raymonds review here? functools is his domain.

@pablogsal
Copy link
Member

2. Is it ok to request Raymonds review here? functools is his domain.

Unless you are changing the module itself, is not very relevant, but you are always free to ask for his advice :)

  • There's some issues with the Windows tests. I've added a workaround in f1f023b, but I don't understand why this is needed. One possible reason could be that SQLite is holding on to TESTFN after the test is finished. I haven't devoted too much time to investigate this yet.

That seems like some genuine problem. Maybe @vstinner has seen this before?

pablogsal
pablogsal reviewed May 19, 2021
View reviewed changes
Modules/_sqlite/connection.c Outdated
@@ -57,6 +56,39 @@ static const char * const begin_statements[] = {
static int pysqlite_connection_set_isolation_level(pysqlite_Connection* self, PyObject* isolation_level, void *Py_UNUSED(ignored));
static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self);

static PyObject *_lru_cache = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful with static globals as this is not subinterpreter-friendly. @vstinner or @ericsnowcurrently can explain further.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, there's no module state in sqlite3. Converting the sqlite3 state to heap types has been the first step on the path to sqlite3 multi-phase initialisation and module state. I can use this PR to establish a static global state, and put _lru_cache there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a global state struct and put _lru_cache there. I can add the rest of the static data to it in a separate PR. One step closer to multi-phase init and sub-interpreter support.

See bb186dca837c805cf5d78ae7675d8a43a0b217cd

@pablogsal
Copy link
Member

When you are done with the fixes, could you test with the buildbots using the buildbot label?

@erlend-aasland
Copy link
Contributor Author

I messed up the previous merge so I had to rebase and force push. Sorry 'bout the mess.

@erlend-aasland erlend-aasland marked this pull request as ready for review May 25, 2021 22:18
This was referenced May 31, 2021
Merged
Closed
@erlend-aasland erlend-aasland marked this pull request as draft June 2, 2021 08:01
pablogsal
pablogsal requested changes Jun 2, 2021
View reviewed changes
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@pablogsal
Copy link
Member

@erlend-aasland is this still a draft (I assume not)? :)

@erlend-aasland erlend-aasland marked this pull request as ready for review June 3, 2021 19:55
@erlend-aasland
Copy link
Contributor Author

is this still a draft (I assume not)? :)

Nope, I was waiting for the CI, just to be sure :)

@pablogsal pablogsal merged commit f461a7f into python:main Jun 3, 2021
@erlend-aasland erlend-aasland deleted the sqlite-cache branch June 3, 2021 19:59
@pablogsal
Copy link
Member

Great job @erlend-aasland! 🎉

@erlend-aasland
Copy link
Contributor Author

Thanks, Pablo! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@berkerpeksag berkerpeksag berkerpeksag left review comments

@pablogsal pablogsal pablogsal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erlend-aasland @pablogsal @bedevere-bot @berkerpeksag @the-knights-who-say-ni