bpo-44525: Specialize CALL_FUNCTION for C function calls

[Fidget-Spinner]

Initial infrastructure code for specializing CALL_FUNCTION. Also added specialization for calling METH_O PyCFunctions because it's the easiest to implement. Along with METH_FASTCALL.

Measured up to 20% faster calls for METH_O on microbenchmarks.

https://bugs.python.org/issue44525

[Fidget-Spinner]

Initially I also planned to specialize things like list() and normal python functions, but the diff was getting rather long, so I'll try those out some other time. This code can also specialize CALL_FUNCTION_KW, and maybe even CALL_METHOD.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit 0e0a3a4 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[pablogsal]

I think this is unfortunate far too complex for the benefit this is giving. I think at the very least this needs some macro benchmarks but if this already is just 20% on a micro benchmark it doesn't look very promising, unfortunately

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[pablogsal]

Oh, I just saw the macro benchmarks in https://gist.github.com/Fidget-Spinner/b2f05e0f6b8851d41e09412f5189ce8f.

Indeed, it shows almost no improvement.

I propose to close this because this is far too complex for no global benefit, and even the micro benchmark is too small in my humble opinion.

Maybe other core Devs have another view on this, thought.

[pablogsal]

@markshannon @serhiy-storchaka @vstinner @isidentical @ericsnowcurrently

[isidentical]

I agree with @pablogsal that as is, it is too much of complexity with no real gains. Though if this is most of an infrastructure PR, then I think the optimizations (6 of them?) @Fidget-Spinner spoke of should also be implemented in this as well, and if they show promising returns on macro benchmarks we could re-evaluate the complexity/performance pair.

[pablogsal]

Notice that this also has a memory cost for caching the pointers, which should also be taken into account when evaluating the benefits.

[Fidget-Spinner]

@pablogsal and @isidentical your concerns are valid and shared by me. I was worried about the code maintenance while writing this PR too.

Roughly 1/4th of it is boilerplate infrastructure for adaptive instructions. The entire diff by CALL_FUNCTION_ADAPTIVE and some parts of the specialize functions fall into that. Maybe I can look into refactoring the code elsewhere to reduce the boilerplate and diff.

I'm hesitant to implement the other specializations too as they require a lot more time (and complexity!). So I sent this PR first to get a sanity check on what I'm doing.

Ultimately, I'll let the core devs decide on this and whether y'all find it useful. I don't mind too much if this gets rejected -- I already had a lot of fun writing this :). But I'm interested to hear what the others have to say.

[Fidget-Spinner]

Oh I forgot to mention that the pyperformance numbers are out of date. They were taken when I only optimized for __builtins__. I'm also waiting for @isidentical 's PR GH-26677 which will cause more CALL_FUNCTION to be emitted correctly. Once that PR is merged I'll re-bench again.

In the meantime, I'll explore the other specializations and see if there's a difference. Thanks everyone!

[markshannon]

Specialized instructions need to be quite specialized 🙂

Looking forward to seeing benchmark results once the specialization is more refined and #26677 is merged.

[markshannon]

One other thing, take a look at #26954

[markshannon]

Looks good. A few minor tweaks needed.

[markshannon]

Currently, just a little bit faster but that's good as this PR takes the hit of the adaptive machinery for all other calls.

[Fidget-Spinner]

Please ignore a message I posted earlier, I just realized it was a different bytecode altogether 🤦‍♂️ .

[markshannon]

A few minor things I missed earlier

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @markshannon for commit f191720 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

[markshannon]

Buildbot failures are the usual trio plus a timeout.

[bedevere-bot]

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL7 LTO 3.x has failed when building commit 3163e68.

What do you need to do:

1. Don't panic.
2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/402/builds/1002) and take a look at the build logs.
4. Check if the failure is related to this commit (3163e68) or if it is a false positive.
5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/402/builds/1002

Failed tests:

• test_pickle
• test_pickletools

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
    ^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: '/psm_52734b07'


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
    ^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: '/psm_b71e5584'


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.x.edelsohn-rhel-z.lto/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
    ^^^^^^^^^^^^^^^^^^^^^^^^^
KeyError: '/psm_d663bdb0'

[pablogsal]

This commit has broken thes 390x RHEL7 LTO 3.x buildbot as you can see before. As per the buildbot maintenance procedures, we will need to revert this PR unless is fixed in 24 hours.

@markshannon @Fidget-Spinner

[Fidget-Spinner]

@pablogsal indeed it seems this PR is guilty. I'll send a PR to revert this tomorrow. Unfortunately I don't have the bandwidth to investigate this right now. Sorry.

[Fidget-Spinner]

@pablogsal it seems that the buildbot is now green thanks to Dennis' patch #29048.

For clarity, the C function optimizations assume all C functions called already do their own recursion checking. This should already be the case for all vectorcall-abiding functions, but it seems that isinstance did not properly abide by our own rules 😉 .

Unfortunately the rule doesn't apply to tp_call functions. So I will have to update CALL_FUNCTION_BUILTIN_O (METH_O) to be safer too.

[pablogsal]

For clarity, the C function optimizations assume all C functions called already do their own recursion checking.

Right, but this is not true in all functions as we saw. Doesn't also need to be done in the function call site? What is the contact exactly here? Because if this means that all the C functions need to do the recursion check on the function body, then the change is backwards incompatible

[Fidget-Spinner]

For clarity, the C function optimizations assume all C functions called already do their own recursion checking.

Right, but this is not true in all functions as we saw. Doesn't also need to be done in the function call site? What is the contact exactly here? Because if this means that all the C functions need to do the recursion check on the function body, then the change is backwards incompatible

The contract is that for vectorcall functions, the callee must do their own recursion checking https://docs.python.org/3/c-api/call.html#recursion-control. For tp_call functions, CPython will check for them.

then the change is backwards incompatible

Right. Like I mentioned above, the only incompatible opcode is CALL_FUNCTION_BUILTIN_O, the other opcodes all call vectorcall functions, or builtin functions that we know have vectorcall, so they must already do their own checking. I'll add recursion checking in the caller for just that one opcode.

[pablogsal]

. I'll add recursion checking in the caller for just that one opcode.

Fantastic! Thanks for the explanation 👍