GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

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

Conversation

@erlend-aasland

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue42862

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@erlend-aasland
erlend-aasland requested review from a team and berkerpeksag as code owners January 12, 2021 23:24
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jan 12, 2021
@bedevere-bot bedevere-bot added the awaiting review label Jan 12, 2021
@erlend-aasland erlend-aasland mentioned this pull request Jan 12, 2021
bpo-40956: Convert _sqlite3.Cache to Argument Clinic #24135
Closed
@erlend-aasland

erlend-aasland commented Jan 12, 2021

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland marked this pull request as draft January 14, 2021 00:41
@erlend-aasland
erlend-aasland force-pushed the sqlite-cache branch from 9fa24b1 to 8f8694a Compare January 21, 2021 20:08
@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
bpo-40956: Convert sqlite3.connect and sqlite3.Connection.__init__ to AC #24421
Merged
@erlend-aasland

erlend-aasland commented Feb 9, 2021

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland mentioned this pull request Feb 10, 2021
bpo-40956: [regression] first argument in sqlite3.Connection.backup is mandatory #24503
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
erlend-aasland force-pushed the sqlite-cache branch from 6cf36b2 to c79c3a9 Compare March 22, 2021 21:08
@erlend-aasland

erlend-aasland commented Mar 22, 2021

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland force-pushed the sqlite-cache branch from c79c3a9 to 3d85a78 Compare April 15, 2021 07:19
@erlend-aasland
erlend-aasland force-pushed the sqlite-cache branch from 3d85a78 to e8478cf Compare May 2, 2021 20:11
@erlend-aasland

erlend-aasland commented May 2, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
erlend-aasland force-pushed the sqlite-cache branch from c561bab to 18a030c Compare May 14, 2021 22:01
@erlend-aasland

erlend-aasland commented May 18, 2021

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
berkerpeksag
berkerpeksag reviewed May 19, 2021
View reviewed changes
Comment thread Doc/library/sqlite3.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal

pablogsal commented May 19, 2021

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed May 19, 2021
View reviewed changes
Comment thread Modules/_sqlite/connection.c Outdated

@pablogsal pablogsal May 19, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland May 19, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@erlend-aasland erlend-aasland May 19, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pablogsal
pablogsal reviewed May 19, 2021
View reviewed changes
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed May 19, 2021
View reviewed changes
Comment thread Modules/_sqlite/connection.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed May 19, 2021
View reviewed changes
Comment thread Lib/sqlite3/test/hooks.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal

pablogsal commented May 19, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland added 3 commits May 26, 2021 00:17
Erlend E. Aasland
Address review: document Windows test workaround
8d1c138
Erlend E. Aasland
Address review: check PyLong_FromLong ret value
0067cef
Erlend E. Aasland
Address review: add global state and add LRU cache func to it
a95a386
@erlend-aasland
erlend-aasland force-pushed the sqlite-cache branch from 6337da4 to a95a386 Compare May 25, 2021 22:17
@erlend-aasland

erlend-aasland commented May 25, 2021

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland marked this pull request as ready for review May 25, 2021 22:18
Erlend E. Aasland added 2 commits May 26, 2021 00:20
Erlend E. Aasland
Fix rebase
9882ebd
Erlend E. Aasland
Merge branch 'main' into sqlite-cache
150b3e7
This was referenced May 31, 2021
bpo-42972: Fix sqlite3 traverse/clear #26452
Merged
bpo-42213: Remove redundant cyclic GC hack in sqlite3 #26462
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
Comment thread Lib/sqlite3/test/dbapi.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting review label Jun 2, 2021
@bedevere-bot

bedevere-bot commented Jun 2, 2021

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jun 2, 2021
Erlend E. Aasland added 4 commits June 2, 2021 14:18
Erlend E. Aasland
Revert test workaround
5a4aacc
Erlend E. Aasland
Merge branch 'main' into sqlite-cache
d1a233a
Erlend E. Aasland
Improve naming: get_statement => get_statement_from_cache
401b057
Erlend E. Aasland
Remove spurious change in connection_dealloc
dfb47a8
pablogsal
pablogsal approved these changes Jun 3, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Jun 3, 2021
@pablogsal

pablogsal commented Jun 3, 2021

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland
erlend-aasland marked this pull request as ready for review June 3, 2021 19:55
@erlend-aasland

erlend-aasland commented Jun 3, 2021

Copy link
Copy Markdown
Contributor Author

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal
pablogsal merged commit f461a7f into python:main Jun 3, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 3, 2021
@erlend-aasland
erlend-aasland deleted the sqlite-cache branch June 3, 2021 19:59
@pablogsal

pablogsal commented Jun 3, 2021

Copy link
Copy Markdown
Member

Great job @erlend-aasland! 🎉
erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Jun 3, 2021

Copy link
Copy Markdown
Contributor Author

Thanks, Pablo! 😃
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@erlend-aasland @pablogsal @bedevere-bot @berkerpeksag @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.