The Wayback Machine - https://web.archive.org/web/20211027233000/https://github.com/python/cpython/pull/27931
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: Offset arguments for PyObject_Vectorcall in the _sqlite module #27931

Merged
merged 3 commits into from Aug 31, 2021
Merged

bpo-42064: Offset arguments for PyObject_Vectorcall in the _sqlite module #27931

merged 3 commits into from Aug 31, 2021

Conversation

@encukou
Copy link
Member

@encukou encukou commented Aug 24, 2021 •
edited by bedevere-bot

This weird mechanism allows e.g. methods to be called efficiently by providing space for a "self" argument; see PY_VECTORCALL_ARGUMENTS_OFFSET docs.

https://bugs.python.org/issue42064
@encukou
Offset arguments for PyObject_Vectorcall in the _sqlite module
dad0c57 
This allows e.g. methods to be called efficiently by providing
space for a "self" argument; see PY_VECTORCALL_ARGUMENTS_OFFSET docs.
@encukou encukou requested a review from erlend-aasland Aug 24, 2021
@encukou encukou requested a review from berkerpeksag as a code owner Aug 24, 2021
@corona10
Copy link
Member

@corona10 corona10 commented Aug 24, 2021

Out of curiosity, How efficient? is there any benchmark?

@encukou
Copy link
Member Author

@encukou encukou commented Aug 24, 2021

Out of curiosity, How efficient? is there any benchmark?

I'm not aware of one, but the docs say this is encouraged :)

shihai1991
shihai1991 reviewed Aug 24, 2021
View changes
Modules/_sqlite/cursor.c Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Aug 24, 2021 •
edited

LGTM, thanks for spotting this! This makes sense for the collation callback, but I can't see how the calls in new_statement_cache and get_statement_from_cache will benefit from this. OTOH, it won't hurt to use the encouraged calling convention :)

erlend-aasland
erlend-aasland suggested changes Aug 24, 2021
View changes
Modules/_sqlite/connection.c Outdated
PyObject *inner = PyObject_Vectorcall(
lru_cache, args + 1, 1 | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL);
Copy link
Contributor

@erlend-aasland erlend-aasland Aug 24, 2021

Could you please break lines according to PEP 7? I'm striving to keep all sqlite3 changes PEP 7 compliant :)
Ditto for the other calls.

Copy link
Contributor

@erlend-aasland erlend-aasland Aug 24, 2021

That is, either:

Suggested change
PyObject *inner = PyObject_Vectorcall(
lru_cache, args + 1, 1 | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL);
PyObject *inner = PyObject_Vectorcall(lru_cache, args + 1,
1 | PY_VECTORCALL_ARGUMENTS_OFFSET,
NULL);

or:

Suggested change
PyObject *inner = PyObject_Vectorcall(
lru_cache, args + 1, 1 | PY_VECTORCALL_ARGUMENTS_OFFSET, NULL);
size_t nargs = 1 | PY_VECTORCALL_ARGUMENTS_OFFSET;
PyObject *inner = PyObject_Vectorcall(lru_cache, args + 1, nargs, NULL);

@erlend-aasland erlend-aasland mentioned this pull request Aug 25, 2021
Merged
encukou added 2 commits Aug 31, 2021
@encukou
Note about borrowed ref
2cbaf4d
@encukou
Avoid long lines
3a2e3fa
@encukou encukou merged commit 01dea5f into python:main Aug 31, 2021
11 checks passed
@encukou encukou deleted the sqlite-vectorcall-offset branch Aug 31, 2021
@encukou
Copy link
Member Author

@encukou encukou commented Aug 31, 2021

Thanks for the reviews!

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

@shihai1991 shihai1991

@erlend-aasland erlend-aasland

@berkerpeksag berkerpeksag

Assignees
No one assigned
Labels
CLA signed skip news
Projects
None yet
Milestone
No milestone
6 participants
@encukou @corona10 @erlend-aasland @shihai1991 @the-knights-who-say-ni @bedevere-bot