Skip to content

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

Merged

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Dec 9, 2021

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 and CALL_METHOD_KW independently.

Since the only real difference between between the CALL_FUNCTION and CALL_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 with LOAD_METHOD. Set the pre-call argument delta, and post-call stack shrink for a method call.
  • CALL_NO_KW: Combines CALL_FUNCTION and CALL_METHOD
  • CALL_KW: Combines CALL_FUNCTION_KW and CALL_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

@Fidget-Spinner
Copy link
Member

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.

@Fidget-Spinner Fidget-Spinner self-requested a review December 10, 2021 13:54
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a 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;
Copy link
Member

@Fidget-Spinner Fidget-Spinner Dec 13, 2021

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?

Copy link
Member Author

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);
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 14, 2021
@bedevere-bot
Copy link

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 14, 2021
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Awesome!

@markshannon markshannon merged commit 9f8f451 into python:main Dec 14, 2021
@markshannon markshannon deleted the split-calls-into-precall-and-call branch December 14, 2021 18:44
Copy link
Member

@brandtbucher brandtbucher left a 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:

Comment on lines +1 to +8
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``.
Copy link
Member

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants