◐ Shell
reader mode source ↗
Skip to content

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

Merged
encukou merged 4 commits into
python:mainfrom
erlend-aasland:sqlite-module-state
Oct 27, 2021
Merged

bpo-42064: Convert sqlite3 global state to module state#29073
encukou merged 4 commits into
python:mainfrom
erlend-aasland:sqlite-module-state

Conversation

@erlend-aasland

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

Copy link
Copy Markdown
Contributor

@encukou

encukou commented Oct 20, 2021

Copy link
Copy Markdown
Member

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

erlend-aasland commented Oct 20, 2021

Copy link
Copy Markdown
Contributor Author

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

encukou commented Oct 20, 2021

Copy link
Copy Markdown
Member

"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

encukou commented Oct 20, 2021

Copy link
Copy Markdown
Member

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

@erlend-aasland

erlend-aasland commented Oct 20, 2021

Copy link
Copy Markdown
Contributor Author

"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

erlend-aasland commented Oct 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

erlend-aasland commented Oct 22, 2021

Copy link
Copy Markdown
Contributor Author

PTAL, @encukou :)

@erlend-aasland

erlend-aasland commented Oct 26, 2021

Copy link
Copy Markdown
Contributor Author

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 reviewed changes

@encukou encukou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Other than this, the change looks good:

@encukou encukou merged commit 8f24b7d into python:main Oct 27, 2021
@erlend-aasland erlend-aasland deleted the sqlite-module-state branch October 27, 2021 11:18
@bedevere-bot

bedevere-bot commented Oct 27, 2021 •
edited
Loading

Copy link
Copy Markdown

⚠️⚠️⚠️ 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

encukou commented Oct 27, 2021

Copy link
Copy Markdown
Member

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

@erlend-aasland

erlend-aasland commented Oct 27, 2021

Copy link
Copy Markdown
Contributor Author

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 encukou left review comments

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland @encukou @bedevere-bot @the-knights-who-say-ni