Repository navigation
Use FASTCALL in typeobject.c call_method() to avoid temporary tuple #73693
Description
Activity
Subset of the (almost) rejected issue bpo-29259 (tp_fastcall), attached patch adds _PyMethod_FastCall() and uses it in call_method() of typeobject.c. The change avoids the creation of a temporary tuple for Python functions and METH_FASTCALL C functions.
Currently, call_method() calls method_call() which calls _PyObject_Call_Prepend(), and calling method_call() requires a tuple for positional arguments.
Example of benchmark on __getitem__(): 1.3x faster (-22%).
$ ./python -m perf timeit -s 'class C:' -s ' def __getitem__(self, index): return index' -s 'c=C()' 'c[0]'
Median +- std dev: 130 ns +- 1 ns => 102 ns +- 1 ns
See also the issue bpo-29263 "Implement LOAD_METHOD/CALL_METHOD for C functions".
Maybe PyObject_Call(), _PyObject_FastCallDict(), etc. can also be modified to get the following fast-path:
+ if (Py_TYPE(func) == &PyMethod_Type) {
+ result = _PyMethod_FastCall(func, args, nargs);
+ }But I don't know how common it is to get a PyMethod_Type object in these functions, nor the code of the additional if.
Maybe, we can skip Method object entirely using _PyObject_GetMethod().
Currently it is used only in LOAD_METHOD.
But PyObject_CallMethod(), _PyObject_CallMethodId(), PyObject_CallMethodObjArgs(), _PyObject_CallMethodIdObjArgs() can use it too.But PyObject_CallMethod(), _PyObject_CallMethodId(), PyObject_CallMethodObjArgs(), _PyObject_CallMethodIdObjArgs() can use it too.
CallMethod[Id]ObjArgs() can use it easily.
But format support is not so easy.callmethod.patch:
+ ../python.default -m perf compare_to default.json patched2.json -G --min-speed=1 Slower (5): - logging_silent: 717 ns +- 9 ns -> 737 ns +- 8 ns: 1.03x slower (+3%) - fannkuch: 1.04 sec +- 0.01 sec -> 1.06 sec +- 0.02 sec: 1.02x slower (+2%) - call_method: 14.5 ms +- 0.1 ms -> 14.7 ms +- 0.1 ms: 1.02x slower (+2%) - call_method_slots: 14.3 ms +- 0.3 ms -> 14.6 ms +- 0.1 ms: 1.02x slower (+2%) - scimark_sparse_mat_mult: 8.66 ms +- 0.21 ms -> 8.76 ms +- 0.25 ms: 1.01x slower (+1%)
Faster (17):
- scimark_lu: 433 ms +- 28 ms -> 410 ms +- 24 ms: 1.06x faster (-5%)
- unpickle: 32.9 us +- 0.2 us -> 31.7 us +- 0.3 us: 1.04x faster (-4%)
- sqlite_synth: 10.0 us +- 0.2 us -> 9.77 us +- 0.24 us: 1.03x faster (-3%)
- telco: 21.1 ms +- 0.7 ms -> 20.6 ms +- 0.4 ms: 1.03x faster (-2%)
- unpickle_list: 8.22 us +- 0.18 us -> 8.02 us +- 0.17 us: 1.03x faster (-2%)
- json_dumps: 30.3 ms +- 0.8 ms -> 29.6 ms +- 0.4 ms: 1.02x faster (-2%)
- nbody: 245 ms +- 6 ms -> 240 ms +- 5 ms: 1.02x faster (-2%)
- meteor_contest: 207 ms +- 2 ms -> 203 ms +- 2 ms: 1.02x faster (-2%)
- scimark_fft: 738 ms +- 14 ms -> 727 ms +- 17 ms: 1.02x faster (-2%)
- pickle_pure_python: 1.27 ms +- 0.02 ms -> 1.25 ms +- 0.02 ms: 1.01x faster (-1%)
- django_template: 401 ms +- 4 ms -> 395 ms +- 5 ms: 1.01x faster (-1%)
- sqlalchemy_declarative: 317 ms +- 3 ms -> 313 ms +- 4 ms: 1.01x faster (-1%)
- json_loads: 64.2 us +- 1.0 us -> 63.4 us +- 1.0 us: 1.01x faster (-1%)
- nqueens: 270 ms +- 2 ms -> 267 ms +- 2 ms: 1.01x faster (-1%)
- crypto_pyaes: 234 ms +- 1 ms -> 231 ms +- 3 ms: 1.01x faster (-1%)
- chaos: 300 ms +- 2 ms -> 297 ms +- 4 ms: 1.01x faster (-1%)
- sympy_expand: 1.01 sec +- 0.01 sec -> 1.00 sec +- 0.01 sec: 1.01x faster (-1%)
Benchmark hidden because not significant (42)
I'm sorry, callmethod.patch is tuned other place, and causing SEGV.
method_fastcall2.patch is tuning same function (call_method() in typeobject.c), and uses trick to bypass temporary method object (same to _PyObject_GetMethod()).
$ ./python -m perf timeit --compare-to `pwd`/python.default -s 'class C:' -s ' def __getitem__(self, index): return index' -s 'c=C()' 'c[0]' python.default: ..................... 155 ns +- 4 ns python: ..................... 111 ns +- 1 ns
Median +- std dev: [python.default] 155 ns +- 4 ns -> [python] 111 ns +- 1 ns: 1.40x faster (-28%)
method_fastcall2.patch is tuning same function (call_method() in typeobject.c), and uses trick to bypass temporary method object (same to _PyObject_GetMethod()).
Oh, great idea! That's why I put you in the nosy list ;-) You know better than me this area of the code.
Median +- std dev: [python.default] 155 ns +- 4 ns -> [python] 111 ns +- 1 ns: 1.40x faster (-28%)
Wow, much better than my patch. Good job!
Can we implement the same optimization in callmethod() of Objects/abstract.c? Maybe add a "is_method" argument to the static function _PyObject_CallFunctionVa(), to only enable the optimization for callmehod().
method_fastcall3.patch implement the trick in more general way.
(I haven't ran test yet since it's midnight. I'll post result later.)method_fastcall4.patch: Based on method_fastcall3.patch, I just added call_unbound() and call_unbound_noarg() helper functions to factorize code. I also modified mro_invoke() to be able to remove lookup_method().
I confirm the speedup with attached bench.py:
Median +- std dev: [ref] 121 ns +- 5 ns -> [patch] 82.8 ns +- 1.0 ns: 1.46x faster (-31%)
method_fastcall4.patch looks clean enough, and performance benefit seems nice.
I don't know current test suite covers unusual special methods.
Maybe, we can extend test_class to cover !unbound (e.g. @classmethod) case.method_fastcall4.patch benchmark results. It's not the first time that I notice that fannkuch and nbody benchmarks become slower. I guess that it's effect of changing code placement because of unrelated change in the C code.
Results don't seem significant on such macro benchmarks (may be random performance changes due to code placement). IMHO the change is worth it! "1.46x faster (-31%)" on a microbenchmark is significant and the change is small.
$ python3 -m perf compare_to /home/haypo/benchmarks/2017-02-08_15-49-default-f507545ad22a.json method_fastcall4_ref_f507545ad22a.json -G --min-speed=5 Slower (2): - fannkuch: 900 ms +- 20 ms -> 994 ms +- 10 ms: 1.10x slower (+10%) - nbody: 215 ms +- 3 ms -> 228 ms +- 4 ms: 1.06x slower (+6%)
Faster (3):
- scimark_lu: 357 ms +- 23 ms -> 298 ms +- 8 ms: 1.19x faster (-16%)
- scimark_sor: 400 ms +- 11 ms -> 355 ms +- 12 ms: 1.12x faster (-11%)
- raytrace: 1.05 sec +- 0.01 sec -> 984 ms +- 15 ms: 1.07x faster (-6%)
Benchmark hidden because not significant (59): (...)
+1 Though this is a rather large and impactful patch, I think it is a great idea. It will be one of the highest payoff applications of FASTCALL, broadly benefitting a lot of code.
Let's be sure to be extra careful with this one because it is touching central parts of the language, so any errors or subtle behavior changes will be felt by a lot of code, some of which is sure to hit the rare corner cases and to rely on implementation details.
6 remaining items
I checked typeobject.c: there is a single case where we use the result of lookup_maybe_method()/lookup_method() for something else than calling the unbound method: slot_tp_finalize() calls PyErr_WriteUnraisable(del), the case discussed in my previous message which caused test_exceptions failure (now fixed).
Thanks for finishing my draft patch, Victor.
callmetohd2.patch is same trick for PyObject_CallMethod* APIs in abstract.c.
It fixes segv in callmethod.patch.
And APIs receiving format string can do same trick when format is empty too.As I grepping "PyObject_CallMethod", there are many format=NULL callers.
python-dev commented
on Feb 9, 2017 python-devmannequinMannequinNew changeset e5cd74868dfc by Victor Stinner in branch 'default':
Issue bpo-29507: Fix _PyObject_CallFunctionVa()
https://hg.python.org/cpython/rev/e5cd74868dfccallmethod2.patch: I like that change on object_vacall(), I'm not sure about the change on PyObject_CallMethod*() only for empty format string.
I suggest to split your patch into two parts, and first focus on object_vacall(). Do you have a benchmark for this one?
Note: I doesn't like the name I chose for object_vacall(). If we modify it, I would suggest to rename it objet_call_vargs() instead.
Anyway, before pushing anything more, I would like to take a decision on the repr()/test_exceptions issue. What do you think Naoki?
I'm not sure about the change on PyObject_CallMethod*() only for empty format string.
There are many place using _PyObject_CallMethodId() to call method without args.
Maybe, we should recommend to use _PyObject_CallMethodIdObjArgs() when no arguments, and replace all caller in cpython?performance benefit is small.
https://gist.github.com/methane/32fe57cd4aaac1c5c37f75cbbfbe7562> I'm not sure about the change on PyObject_CallMethod*() only for empty format string.
There are many place using _PyObject_CallMethodId() to call method without args.
I'm more interested by an optimization PyObject_CallMethod*() for any number of arguments, as done in typeobject.c ;-)
performance benefit is small.
https://gist.github.com/methane/32fe57cd4aaac1c5c37f75cbbfbe7562Are you using PGO+LTO compilation? Without PGO, the noise of code placement can be too high. In your "perf stat" comparisons, I see that "insn per cycle" is lower with the patch, which sounds like a code placement issue like a performance issue with the patch.
Yes, I used --enable-optimization this time.
But my patch is not good for branch prediction of my CPU in this time.
I'm willing Object/call.c solves such placement issue.BTW, since benefit of GetMethod is small, how about this?
- Add _PyMethod_FastCallKeywords
- Call it from _PyObject_FastCall*
_PyObject_FastCall* can use FASTCALL C function and method (PyCFunction),
and Python function (PyFunction).
Python method (PyMethod) is last common callable PyObject_FastCall* can't use FASTCALL.I'm willing Object/call.c solves such placement issue.
I also *hope* that a call.c file would *help* a little bit, but I'm not sure that it will fix *all* code placement issues.
I created the issue bpo-29524 with a patch creating Objects/call.c.
- changed the title
[-]Use FASTCALL in call_method() to avoid temporary tuple[/-][+]Use FASTCALL in typeobject.c call_method() to avoid temporary tuple[/+]on Mar 14, 2017
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields: