Skip to content
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.

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

bpo-42064: Convert sqlite3 global state to module state #29073

Merged
merged 4 commits into from Oct 27, 2021
Merged

bpo-42064: Convert sqlite3 global state to module state #29073

merged 4 commits into from Oct 27, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Oct 19, 2021 •
edited by bedevere-bot

@erlend-aasland erlend-aasland requested a review from encukou Oct 19, 2021
@encukou
Copy link
Member

encukou commented Oct 20, 2021

Nice! The finish line is near!

pysqlite_get_state_by_cls is a bit dangerous: the cls must be exactly a _sqlite3 class, not a subclass (which would be defined in another module).
There are two uses of it, in pysqlite_row_richcompare (where it's incorrect since it uses Py_TYPE(self)), and in pysqlite_connection_close_impl (where it's correct since it uses the defining class).

I suggest removing pysqlite_get_state_by_cls and switching all uses to pysqlite_get_state_by_type. Avoiding the overhead of _PyType_GetModuleByDef is not worth keeping this trap. (And if you call it with the defining class, the overhead shouldn't be noticeable.)

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 20, 2021

Nice! The finish line is near!

Yes, it's been a long journey, but I've learned a thing or two while getting here :)

pysqlite_get_state_by_cls is a bit dangerous: the cls must be exactly a _sqlite3 class, not a subclass (which would be defined in another module). There are two uses of it, in pysqlite_row_richcompare (where it's incorrect since it uses Py_TYPE(self)), [...]

Hm, I was under the impression that the left operand was guaranteed to be an instance of the defining class:

https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_richcompare

@encukou
Copy link
Member

encukou commented Oct 20, 2021

"an instance of the defining class" roughly means isinstance(self, defining_cls). Here you need type(self) == defining_cls. The two are different if self is an instance of a subclass.

@encukou
Copy link
Member

encukou commented Oct 20, 2021

(And any subclass will necessarily be defined in another module, so PyType_GetModule won't return _sqlite3 for it.)

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 20, 2021

"an instance of the defining class" roughly means isinstance(self, defining_cls). Here you need type(self) == defining_cls. The two are different if self is an instance of a subclass.

Yep, I know :) I'm not sure where I got the misconception regarding rich compare from, but that does not matter; I'll fix it right away.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 20, 2021 •
edited

Also, I'm fine with removing pysqlite_get_state_by_cls in favour of pysqlite_get_state_by_type. It's only used twice, and both uses are slow paths (just before raising an exception).

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 22, 2021

PTAL, @encukou :)

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 26, 2021

FYI, I ran the sqlitesynth benchmark to see if this change introduces any performance regression:

$ python -m pyperf compare_to a.json b.json 
Mean +- std dev: [a] 5.22 us +- 0.08 us -> [b] 5.20 us +- 0.11 us: 1.00x faster
  • a: vanilla CPython
  • b: This PR

encukou
encukou reviewed Oct 26, 2021
View changes
Copy link
Member

@encukou encukou left a comment

Other than this, the change looks good:

Modules/_sqlite/module.h Outdated Show resolved Hide resolved
@encukou encukou merged commit 8f24b7d into python:main Oct 27, 2021
11 checks passed
@erlend-aasland erlend-aasland deleted the sqlite-module-state branch Oct 27, 2021
@bedevere-bot
Copy link

bedevere-bot commented Oct 27, 2021 •
edited

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 FreeBSD Non-Debug 3.x has failed when building commit 8f24b7d.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/172/builds/1015) and take a look at the build logs.
  4. Check if the failure is related to this commit (8f24b7d) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/172/builds/1015

Summary of the results of the build (if available):

Click to see traceback logs 
remote: Enumerating objects: 25, done.        
remote: Counting objects:   4% (1/25)        
remote: Counting objects:   8% (2/25)        
remote: Counting objects:  12% (3/25)        
remote: Counting objects:  16% (4/25)        
remote: Counting objects:  20% (5/25)        
remote: Counting objects:  24% (6/25)        
remote: Counting objects:  28% (7/25)        
remote: Counting objects:  32% (8/25)        
remote: Counting objects:  36% (9/25)        
remote: Counting objects:  40% (10/25)        
remote: Counting objects:  44% (11/25)        
remote: Counting objects:  48% (12/25)        
remote: Counting objects:  52% (13/25)        
remote: Counting objects:  56% (14/25)        
remote: Counting objects:  60% (15/25)        
remote: Counting objects:  64% (16/25)        
remote: Counting objects:  68% (17/25)        
remote: Counting objects:  72% (18/25)        
remote: Counting objects:  76% (19/25)        
remote: Counting objects:  80% (20/25)        
remote: Counting objects:  84% (21/25)        
remote: Counting objects:  88% (22/25)        
remote: Counting objects:  92% (23/25)        
remote: Counting objects:  96% (24/25)        
remote: Counting objects: 100% (25/25)        
remote: Counting objects: 100% (25/25), done.        
remote: Compressing objects:   9% (1/11)        
remote: Compressing objects:  18% (2/11)        
remote: Compressing objects:  27% (3/11)        
remote: Compressing objects:  36% (4/11)        
remote: Compressing objects:  45% (5/11)        
remote: Compressing objects:  54% (6/11)        
remote: Compressing objects:  63% (7/11)        
remote: Compressing objects:  72% (8/11)        
remote: Compressing objects:  81% (9/11)        
remote: Compressing objects:  90% (10/11)        
remote: Compressing objects: 100% (11/11)        
remote: Compressing objects: 100% (11/11), done.        
remote: Total 13 (delta 12), reused 2 (delta 2), pack-reused 0        
From https://github.com/python/cpython
 * branch                  main       -> FETCH_HEAD
Note: switching to '8f24b7dbcbd83311dad510863d8cb41f0e91b464'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 8f24b7dbcb [bpo-42064](https://bugs.python.org/issue42064): Convert `sqlite3` global state to module state (GH-29073)
Switched to and reset branch 'main'

renaming build/scripts-3.11/pydoc3 to build/scripts-3.11/pydoc3.11
renaming build/scripts-3.11/idle3 to build/scripts-3.11/idle3.11
renaming build/scripts-3.11/2to3 to build/scripts-3.11/2to3-3.11

renaming build/scripts-3.11/pydoc3 to build/scripts-3.11/pydoc3.11
renaming build/scripts-3.11/idle3 to build/scripts-3.11/idle3.11
renaming build/scripts-3.11/2to3 to build/scripts-3.11/2to3-3.11

Cannot open file '/usr/home/buildbot/python/3.x.koobs-freebsd-9e36.nondebug/build/test-results.xml' for upload

@encukou
Copy link
Member

encukou commented Oct 27, 2021

The buildbot failed with signal 9 (SIGKILL), before it got to sqlite tests. Most likely unrelated.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Oct 27, 2021

The buildbot failed with signal 9 (SIGKILL), before it got to sqlite tests. Most likely unrelated.

Yes, I've concluded with it being unrelated. Looks like the buildbot was shut down or restarted.

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

@encukou encukou

@berkerpeksag berkerpeksag

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@erlend-aasland @encukou @bedevere-bot @the-knights-who-say-ni