GeezerVison is showing github.com — bpo-41861: Convert _sqlite3 CursorType and ConnectionType to heap types by erlend-aasland · Pull Request #22478 · python/cpython
Conversation
erlend-aasland
commented
Oct 1, 2020
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Merged, thanks. There are two remaining static types?
|
erlend-aasland reacted with hooray emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Merged, thanks. There are two remaining static types?
Thanks! No more remaining types.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Merged, thanks. There are two remaining static types?
BTW, should I add traverse/clear/free methods to the module def before closing the issue?
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
vstinner
commented
Oct 1, 2020
•
edited by bedevere-bot
Loading
Uh oh!
There was an error while loading. Please reload this page.
BTW, should I add traverse/clear/free methods to the module def before closing the issue?
Currently, it's possible to have more than once instance of the _sqlite3 module. If a second instance is created and then cleared, it would clear the shared heap types of the first instance: not good.
It would be better to add a module state and retrieve these types from the module state.
The problem is to retrieve the module state. I suggest you to convert all methods which use these 4 types to Argument Clinic, so later you will be able to use "defining_class" parameter which is a reliable way to get the module state from a type. That's why I asked you to use PyType_FromModuleAndSpec().
Example: _sqlite3.Connection.backup() Python method is implemented as the pysqlite_connection_backup() C function. If you modify the function to use Argument Clinic, you can then get the defining class: then PyType_GetModuleState(cls) gives you the module state.
pysqlite_connection_backup() -> defining_class (cls) -> PyType_GetModuleState(cls) -> module state -> your heap types
Converting _sqlite3 to use a module state will make it safe to be used in Python subinterpreters!
See:
|
erlend-aasland reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
BTW, should I add traverse/clear/free methods to the module def before closing the issue?
Currently, it's possible to have more than once instance of the _sqlite3 module. If a second instance is created and then cleared, it would clear the shared heap types of the first instance: not good.
It would be better to add a module state and retrieve these types from the module state.
I already started experimenting with this. Should I put up what I've got as a draft PR? I guess this goes as a separate issue anyways.
The problem is to retrieve the module state.
I'm painfully aware of this :)
I suggest you to convert all methods which use these 4 types to Argument Clinic, so later you will be able to use "defining_class" parameter which is a reliable way to get the module state from a type. That's why I asked you to use PyType_FromModuleAndSpec().
I think I already put up a PR that converts _sqlite3 to Argument Clinic. I'll rebase onto master and ping you.
Thanks!
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
a PR that converts _sqlite3 to Argument Clinic
That sounds like a good start! I prefer a PR to convert to Argument Clinic, and then a second one to add a module state.
|
erlend-aasland reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
a PR that converts _sqlite3 to Argument Clinic
That sounds like a good start! I prefer a PR to convert to Argument Clinic, and then a second one to add a module state.
While my other PR's are on hold, I've started with the post-agrument-clinic-step: heap module state (PEP 573). I'm currently basing this branch on top off my work with bpo-40956. Using defining_class makes this very easy, however I need help with resolving two issues:
-
What to do with the Argument Clinic class definition? We need the module pointer to get to the instances and the types. Quoting from the Argument Clinic docs:
"When you declare a class, you must also specify two aspects of its type in C: the type declaration you’d use for a pointer to an instance of this class, and a pointer to the PyTypeObject for this class."
-
What to do with __init__ and __new__? Providing a pointer to the module in the class/type state, would solve it, but that feels backwards and hacky.
UPDATE: Solved like this (excerpt from Argument Clinic for _sqlite3.Connection.__init__):
factory: object(c_default='(PyObject*)((struct pysqlite_state *)PyType_GetModuleState(Py_TYPE(self)))->ConnectionType') = ConnectionType
Is this an accepted solution?
Also, defining_class is not documented. I'll open an issue for that. https://docs.python.org/3.10/howto/clinic.html
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I wrote #22712 to "explain my idea". It's a minimum change just to pass a "state" to functions which access the UCD_Type.
|
erlend-aasland reacted with thumbs up emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
I wrote #22712 to "explain my idea". It's a minimum change just to pass a "state" to functions which access the UCD_Type.
Thanks, I'll have a look.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
xzy3
pushed a commit
to xzy3/cpython
that referenced
this pull request
Oct 18, 2020
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
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.
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.
|