bpo-42862: Use functools.lru_cache iso. _sqlite.Cache in sqlite3 module by erlend-aasland · Pull Request #24203

bpo-42862: Use functools.lru_cache iso. _sqlite.Cache in sqlite3 module by erlend-aasland · Pull Request #24203 · python/cpython

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

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 :)

@pablogsal, two questions:

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.
Is it ok to request Raymonds review here? functools is his domain.

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?

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.

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.

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

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

Erlend E. Aasland added 3 commits

May 26, 2021 00:17

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

Erlend E. Aasland added 2 commits

May 26, 2021 00:20

This was referenced

May 31, 2021

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.

Erlend E. Aasland added 4 commits

June 2, 2021 14:18

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

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