Skip to content

bpo-46541: Replace _Py_IDENTIFIER with _Py_ID in sqlite3 #31351

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 8 commits into from
Feb 16, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Feb 15, 2022

@erlend-aasland
Copy link
Contributor Author

Assuming we skip news, since this is an internal change only.

@ericsnowcurrently
Copy link
Member

The main reason I didn't do this earlier is because our (non-builtin) extension modules probably shouldn't be relying on internal API. Instead we should try to use only public API (and sometimes "private", AKA public API with a leading underscore).

How bad would it be to replace uses of _Py_IDENTIFIER() with PyUnicode_InternFromString()?

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Feb 15, 2022

The main reason I didn't do this earlier is because our (non-builtin) extension modules probably shouldn't be relying on internal API.

It's fully possible to build any stdlib extension as a built-in, even sqlite3.

How bad would it be to replace uses of _Py_IDENTIFIER() with PyUnicode_InternFromString()?

Most users will call execute and friend on a cursor object, not on a connection object, so they won't notice neither performance improvements nor regressions. I'm not sure about the adapter case though; I don't know how widely used they actually are.

@erlend-aasland
Copy link
Contributor Author

The main reason I didn't do this earlier is because our (non-builtin) extension modules probably shouldn't be relying on internal API. Instead we should try to use only public API (and sometimes "private", AKA public API with a leading underscore).

OTOH, I'm totally fine with mainly using public APIs in the sqlite3 module. I'll rewrite it to use public APIs instead.

Erlend E. Aasland added 2 commits February 15, 2022 20:55
@ericsnowcurrently
Copy link
Member

BTW, thanks for working on this!

@erlend-aasland
Copy link
Contributor Author

BTW, thanks for working on this!

Likewise :)

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Feb 16, 2022

PTAL. This should result in a small speedup compared to main.

The sqlite3.Connection.cursor() calls are now made directly. Ideally we should do the same with the Cursor.execute* calls, but sqlite3 is inconveniently implemented across multiple files1, so we can't just call the cursor impl methods directly. I changed the scope of _pysqlite_query_execute, so we can at least speed up the Connection.execute() and Connection.executemany() calls. The remaining methods now use interned strings, with the module always keeping a strong ref to __adapt__, __conform__, and upper to speed up adapters/converters, and finalize to speed up user-defined functions. I did the same for executemany; it is probably not used in hot code, but following the same pattern simplified the code a bit.

Footnotes

  1. Consolidating all the Module/_sqlite/*.[ch] files into Module/_sqlite3.[ch] would be very helpful.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

@corona10 corona10 merged commit b207711 into python:main Feb 16, 2022
@erlend-aasland erlend-aasland deleted the sqlite-pyid branch February 16, 2022 16:49
@erlend-aasland
Copy link
Contributor Author

Thanks for your review and thoughts, @corona10 & @ericsnowcurrently

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