-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-44525: Split calls into PRECALL and CALL #30011
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
bpo-44525: Split calls into PRECALL and CALL #30011
Conversation
…CALL_FUNCTION_KW.
This is really exciting work, deltablue, hexiom and mako are pretty CALL_X heavy programs. So the pyperformance results looks promising! I'll try to review within the next week unless someone else beats me to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea looks good in general. The dis
docs and their opcodes need eventual updating too. That can probably come in another PR.
return -1; | ||
} | ||
|
||
static PyMethodDescrObject *_list_append = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something tells me Victor (and the subinterpreter folks) won't be happy seeing this 😄 . We can probably move it to interpreter state in another PR. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PyList_Type
is a static global and part of the ABI: https://github.com/python/cpython/blob/main/Misc/stable_abi.txt#L864
So the list.append
method descriptor might as well be a static global as well (at least for now).
return -1; | ||
} | ||
if (_list_append == NULL) { | ||
_list_append = (PyMethodDescrObject *)_PyType_LookupId(&PyList_Type, &PyId_append); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note: If we're going this route to make specialization attempts cheaper, we could consider a unified way to cache these types of lookups. E.g. len
and isinstance
specializations need looking into builtins()
dict every time. Caching the len
and isinstance
objects plus the builtins()
dict tag would make them much cheaper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
…tment around calls a bit more maintainable.
🤖 New build scheduled with the buildbot fleet by @markshannon for commit 4c855e2 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wasn't able to get to this yesterday (got bogged down in benchmarking issues). Just one fairly minor doc comment that could be followed up on:
Replace the four call bytecode instructions which one pre-call instruction | ||
and two call instructions. | ||
|
||
Removes ``CALL_FUNCTION``, ``CALL_FUNCTION_KW``, ``CALL_METHOD`` and | ||
``CALL_METHOD_KW``. | ||
|
||
Adds ``CALL_NO_KW`` and ``CALL_KW`` call instructions, and | ||
``PRECALL_METHOD`` prefix for pairing with ``LOAD_METHOD``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've started keeping another log of bytecode changes over in What's New. That section should probably be updated with these changes (especially since CALL_METHOD
and CALL_METHOD_KW
are mentioned there already).
Specializing calls is going to need a lot of specialized bytecode if we want to specialize all four of
CALL_FUNCTION
,CALL_FUNCTION_KW
,CALL_METHOD
andCALL_METHOD_KW
independently.Since the only real difference between between the
CALL_FUNCTION
andCALL_METHOD
forms is shifting the start of the arguments by one or two, it makes sense to move that into a pre-call instruction.The above four opcodes are replaced by:
PRECALL_METHOD
: Pairs withLOAD_METHOD
. Set the pre-call argument delta, and post-call stack shrink for a method call.CALL_NO_KW
: CombinesCALL_FUNCTION
andCALL_METHOD
CALL_KW
: CombinesCALL_FUNCTION_KW
andCALL_METHOD_KW
This means that we can specialize calls to methods with much extra effort.
Specialization of calls to Python methods happens almost for free (we need an additional check that the number of args is as expected).
Specialization of method descriptors needs a few more instructions, but no extra
ADAPTIVE
form is needed.1% speedup
I think that the slowdowns are calls to bound-methods. So they are next on the list.
https://bugs.python.org/issue44525