Skip to content

bpo-42064: Optimise sqlite3 state access, part 1 #27273

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

Merged
merged 6 commits into from
Jul 29, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jul 20, 2021

Prepare for module state:

  • Add "get state by defining class" and "get state by module def" stubs
  • Add AC defining class when needed
  • Add state pointer to connection context
  • Pass state as argument to utility functions

https://bugs.python.org/issue42064

Automerge-Triggered-By: GH:encukou

@encukou
Copy link
Member

encukou commented Jul 27, 2021

Have you thought about caching the state on Connection objects? That way you could get the state from a cursor by following two pointers. (And similarly for other cases where you have a Connection, which is most of pysqlite).
Connections are already quite heavy, so adding one more pointer might be a good trade-off.

pysqlite_get_state_by_cls looks like it'll eventually use _PyType_GetModuleByDef, which isn't as fast as following two pointers.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 27, 2021

Have you thought about caching the state on Connection objects? That way you could get the state from a cursor by following two pointers. (And similarly for other cases where you have a Connection, which is most of pysqlite).
Connections are already quite heavy, so adding one more pointer might be a good trade-off.

Yes, I have though about it, but I don't remember why I didn't follow that idea :) As you say, it's probably a very good trade-off. I'll have a go at it.

pysqlite_get_state_by_cls looks like it'll eventually use _PyType_GetModuleByDef, which isn't as fast as following two pointers.

Both true. No, it was meant to use PyType_GetModuleState.

@erlend-aasland erlend-aasland marked this pull request as draft July 27, 2021 21:29
@erlend-aasland erlend-aasland marked this pull request as ready for review July 27, 2021 23:41
@erlend-aasland
Copy link
Contributor Author

PTAL!

@encukou
Copy link
Member

encukou commented Jul 29, 2021

Looks good, I just did a trivial merge to resolve a conflict.

@miss-islington
Copy link
Contributor

@erlend-aasland: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit d542742 into python:main Jul 29, 2021
@erlend-aasland
Copy link
Contributor Author

Looks good, I just did a trivial merge to resolve a conflict.

Great, thanks and thank you for reviewing.

@erlend-aasland erlend-aasland deleted the sqlite-ac-defining-class branch July 29, 2021 09:52
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Jul 29, 2021
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants