Movatterモバイル変換

📜🤖 Added by blurb_it.

edcdcf1

Copy link

ContributorAuthor

adphrost commentedMay 3, 2022

@markshannon Can you let me know if there should be any other checks inceval.c besides the one added in this PR?

For context, this check fixes an edge case where calling function with a set vectorfield only worked with a unpacked tuple, e.g. f(1) failed butf(*(1,)) succeeded (now both succeed)

itamaro reviewed

Copy link

Contributor

itamaro left a comment

Choose a reason for hiding this comment

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

thanks for working on this@adphrost !

Lib/test/test_call.py OutdatedShow resolvedHide resolved

Python/ceval.c Outdated

		intpositional_args=total_args-KWNAMES_LEN();
		// Check if the call can be inlined or not
		if (Py_TYPE(function)==&PyFunction_Type&&tstate->interp->eval_frame==NULL) {
		if (Py_TYPE(function)==&PyFunction_Type&&tstate->interp->eval_frame==NULL&& ((PyFunctionObject*)function)->vectorcall==_PyFunction_Vectorcall) {

Copy link

Contributor

Choose a reason for hiding this comment

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

this probably exceeds line length

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

What is the max line length allowed? And is there a linter I can run to handle this for me?

vstinner reviewed

Misc/NEWS.d/next/C API/2022-05-03-19-35-37.gh-issue-92193.61VoFL.rst Outdated


		This PR introduces an API call where vectorcall field can be modified like so:

		`void PyFunction_SetVectorcall(PyFunctionObject *func, vectorcallfunc vectorcall);`

Copy link

Member

Choose a reason for hiding this comment

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

Please document the function in Doc/c-api/function.rst and in the NEWS entry, refer to use with:

:c:func:`PyFunction_SetVectorcall`

New public functions should also be documented in What's New in Python 3.11 > C API > New features: Doc/whatsnew/3.11.rst.

markshannon reviewed

Copy link

Member

markshannon left a comment

Choose a reason for hiding this comment

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

A few comments.
Have you attempted to measure the performance impact of this? (I'd expect it to be small or negligible).

Objects/funcobject.c OutdatedShow resolvedHide resolved

Modules/_testcapimodule.c OutdatedShow resolvedHide resolved

Apply suggestions from code review

debf5a9

Change how assertion is doneCo-authored-by: Itamar Ostricher <itamarost@gmail.com>

Copy link

ghost commentedMay 3, 2022•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.

itamaro mentioned this pull request

gh-92193: Check for overridden vectorcall field on PyFunctionObject#92194

Closed

addressed comments

473d18d

markshannon mentioned this pull request

GH-92239: Respect PEP 523#92245

Merged

added to docs

76e8c9d

Copy link

Contributor

itamaro commentedMay 4, 2022

Have you attempted to measure the performance impact of this? (I'd expect it to be small or negligible).

@markshannon is there a simple way to ask a buildbot to run pyperformance comparing this against the base revision? we can do that manually, but a buildbot doing it for us would be nice :)

Copy link

Contributor

itamaro commentedMay 19, 2022

circling back to@markshannon 's performance impact question. -- tldrGeometric mean: 1.00x faster

some more details:

rebased this PR ontoa4460f2 and ran pyperformance ona4460f2 and on the rebased branch

both baseline and branch were built using./configure --enable-optimizations --with-lto && make -j

used pyperformance 1.0.5 (installed directly from GitHub) on bare metal AWS machine (Linux-5.13.0-1023-aws-x86_64-with-glibc2.31 with 72 logical CPUs), and excluded the sqlalchemy benchmarks because they tried building greenlet and failed

pyperformance run --benchmarks=-sqlalchemy_declarative,-sqlalchemy_imperative --python builds/3.12-opt/python -o main.json --rigorouspyperformance run --benchmarks=-sqlalchemy_declarative,-sqlalchemy_imperative --python builds/3.12-gh-91049-vectorcall-opt/python -o gh-91049.json --rigorous

compare results:

pyperf compare_to main.json gh-91049.json -GSlower (20):- regex_effbot: 3.02 ms +- 0.01 ms -> 3.17 ms +- 0.01 ms: 1.05x slower- meteor_contest: 110 ms +- 2 ms -> 114 ms +- 4 ms: 1.04x slower- pidigits: 200 ms +- 0 ms -> 208 ms +- 0 ms: 1.04x slower- telco: 7.17 ms +- 0.21 ms -> 7.39 ms +- 0.34 ms: 1.03x slower- html5lib: 64.0 ms +- 2.7 ms -> 65.6 ms +- 2.6 ms: 1.02x slower- unpickle_list: 5.30 us +- 0.08 us -> 5.40 us +- 0.05 us: 1.02x slower- pyflate: 430 ms +- 3 ms -> 438 ms +- 4 ms: 1.02x slower- sympy_sum: 168 ms +- 2 ms -> 171 ms +- 2 ms: 1.02x slower- xml_etree_iterparse: 110 ms +- 1 ms -> 112 ms +- 3 ms: 1.02x slower- async_tree_memoization: 640 ms +- 25 ms -> 650 ms +- 25 ms: 1.01x slower- chaos: 71.8 ms +- 0.7 ms -> 72.8 ms +- 0.6 ms: 1.01x slower- hexiom: 6.71 ms +- 0.06 ms -> 6.80 ms +- 0.06 ms: 1.01x slower- sympy_str: 299 ms +- 3 ms -> 302 ms +- 4 ms: 1.01x slower- spectral_norm: 101 ms +- 1 ms -> 103 ms +- 1 ms: 1.01x slower- sympy_integrate: 21.5 ms +- 0.2 ms -> 21.8 ms +- 0.2 ms: 1.01x slower- logging_silent: 105 ns +- 1 ns -> 107 ns +- 3 ns: 1.01x slower- raytrace: 303 ms +- 2 ms -> 306 ms +- 4 ms: 1.01x slower- regex_v8: 23.1 ms +- 0.3 ms -> 23.3 ms +- 0.1 ms: 1.01x slower- unpickle_pure_python: 239 us +- 2 us -> 240 us +- 2 us: 1.01x slower- 2to3: 282 ms +- 2 ms -> 283 ms +- 1 ms: 1.00x slowerFaster (25):- genshi_xml: 57.1 ms +- 2.7 ms -> 54.3 ms +- 1.8 ms: 1.05x faster- scimark_sparse_mat_mult: 4.93 ms +- 0.06 ms -> 4.70 ms +- 0.10 ms: 1.05x faster- scimark_lu: 112 ms +- 2 ms -> 109 ms +- 2 ms: 1.03x faster- json_dumps: 13.3 ms +- 0.2 ms -> 12.9 ms +- 0.1 ms: 1.03x faster- scimark_fft: 337 ms +- 3 ms -> 331 ms +- 3 ms: 1.02x faster- nbody: 97.6 ms +- 1.8 ms -> 95.9 ms +- 1.8 ms: 1.02x faster- pathlib: 23.0 ms +- 1.2 ms -> 22.6 ms +- 0.8 ms: 1.02x faster- pickle_pure_python: 324 us +- 3 us -> 318 us +- 3 us: 1.02x faster- genshi_text: 23.2 ms +- 0.4 ms -> 22.8 ms +- 0.3 ms: 1.02x faster- unpack_sequence: 46.5 ns +- 0.6 ns -> 45.7 ns +- 0.7 ns: 1.02x faster- fannkuch: 401 ms +- 6 ms -> 396 ms +- 4 ms: 1.01x faster- chameleon: 7.06 ms +- 0.06 ms -> 6.98 ms +- 0.08 ms: 1.01x faster- pickle_dict: 27.5 us +- 0.1 us -> 27.2 us +- 1.0 us: 1.01x faster- pickle: 10.2 us +- 0.1 us -> 10.1 us +- 0.1 us: 1.01x faster- xml_etree_parse: 170 ms +- 2 ms -> 168 ms +- 3 ms: 1.01x faster- xml_etree_process: 56.8 ms +- 0.5 ms -> 56.3 ms +- 0.5 ms: 1.01x faster- nqueens: 88.0 ms +- 1.1 ms -> 87.2 ms +- 1.1 ms: 1.01x faster- json_loads: 26.3 us +- 0.3 us -> 26.1 us +- 0.2 us: 1.01x faster- async_tree_none: 549 ms +- 15 ms -> 544 ms +- 12 ms: 1.01x faster- async_tree_cpu_io_mixed: 771 ms +- 13 ms -> 766 ms +- 10 ms: 1.01x faster- mako: 10.5 ms +- 0.0 ms -> 10.4 ms +- 0.1 ms: 1.01x faster- crypto_pyaes: 77.2 ms +- 0.8 ms -> 76.8 ms +- 0.9 ms: 1.01x faster- xml_etree_generate: 81.2 ms +- 0.6 ms -> 80.8 ms +- 0.5 ms: 1.00x faster- regex_compile: 142 ms +- 1 ms -> 141 ms +- 1 ms: 1.00x faster- scimark_sor: 121 ms +- 1 ms -> 120 ms +- 1 ms: 1.00x fasterBenchmark hidden because not significant (18): async_tree_io, deltablue, django_template, dulwich_log, float, go, logging_format, logging_simple, pickle_list, python_startup, python_startup_no_site, regex_dna, richards, scimark_monte_carlo, sqlite_synth, sympy_expand, tornado_http, unpickleGeometric mean: 1.00x faster

thanks@ansley for doing the perf analysis on this :)

itamaro mentioned this pull request

May 25, 2022

Allow extensions to set the vectorcall field on instances of PyFunctionObject#91049

Closed

Andrew Frost added5 commits

June 15, 2022 18:05

Merge branch 'main' into pyfunctionobject-set-vectorcall-field

3337459

updated doc with fix by itamaro

1543f44

formatting

08082bd

removed doc from 3.11

ed93327

remove lines from 3.11

24ffd30

vstinner reviewed

Jun 16, 2022

Copy link

Member

vstinner left a comment

Choose a reason for hiding this comment

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

Oh. This change reminds me my oldhttps://peps.python.org/pep-0510/ nice :-)

Doc/c-api/function.rst OutdatedShow resolvedHide resolved

Modules/_testcapimodule.c Outdated

		staticPyObject*
		override_vectorcall(
		PyObjectcallable,PyObjectconstargs,size_tnargsf,PyObjectkwnames) {
		Py_RETURN_NONE;

Copy link

Member

vstinnerJun 16, 2022

Choose a reason for hiding this comment

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

Could you modify the test to return the string "overridden" instead of just returning None?

Doc/whatsnew/3.12.rst Outdated

		(Contributed by Wenzel Jakob in:gh:`93012`.)

		* Add new function:c:func:`PyFunction_SetVectorcall` to the C API
		which sets the vectorcall field of a given PyFunctionObject

Copy link

Member

vstinnerJun 16, 2022

Choose a reason for hiding this comment

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

You may explain the purpose of the function here, like mention Cinder JIT and Pyjion.

Copy link

Member

markshannonJun 17, 2022

Choose a reason for hiding this comment

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

I'd rather not have a "purpose". We should state clearly what the function does, not bless some particular use case.

@adphrost
Could you add a warning that the new function pointer must haveexactly the same behavior as the unaltered function.
I am a little concerned that C extensions will abuse this, and we will have deal with the bug reports.

Misc/NEWS.d/next/C API/2022-05-03-19-35-37.gh-issue-92193.61VoFL.rst Outdated

		CPython extensions providing optimized execution of Python bytecode (like Cinder JIT and Pyjion)
		can benefit from being able to modify the vectorcall field on instances of PyFunctionObject to allow calling the optimized path (e.g. JIT-compiled) directly.

		This PR introduces an API call where vectorcall field can be modified like so:

Copy link

Member

vstinnerJun 16, 2022

Choose a reason for hiding this comment

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

This text lands intohttps://docs.python.org/dev/whatsnew/changelog.html#changelog which is a changlog, you should not mention "a PR" there.

I suggest to just copy/paste what you wrote in whatsnew.

Doc/whatsnew/3.12.rst Outdated

		(Contributed by Wenzel Jakob in:gh:`93012`.)

		* Add new function:c:func:`PyFunction_SetVectorcall` to the C API
		which sets the vectorcall field of a given PyFunctionObject

Copy link

Member

vstinnerJun 16, 2022

Choose a reason for hiding this comment

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

Suggested change

	which sets the vectorcall field of a given PyFunctionObject
	which sets the vectorcall field of a given:c:type:`PyFunctionObject`.

Apply review feedback from vstinner and markshannon

89cb4b2

Copy link

Contributor

itamaro commentedJun 25, 2022

@vstinner @markshannon I think all comments were addressed - any further feedback?

vstinner reviewed

Jun 28, 2022

Doc/c-api/function.rst Outdated


		..c:function::voidPyFunction_SetVectorcall(PyFunctionObject *func, vectorcallfunc vectorcall)

		Set the vectorcall field of a given function object func

Copy link

Member

vstinnerJun 28, 2022

Choose a reason for hiding this comment

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

Suggested change

	Set the vectorcall field of a given function object func
	Set the vectorcall field of a given function object func.

Doc/whatsnew/3.12.rst Outdated

		(Contributed by Wenzel Jakob in:gh:`93012`.)

		* Add new function:c:func:`PyFunction_SetVectorcall` to the C API
		which sets the vectorcall field of a given:c:type:`PyFunctionObject`

Copy link

Member

vstinnerJun 28, 2022

Choose a reason for hiding this comment

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

Suggested change

	which sets the vectorcall field of a given:c:type:`PyFunctionObject`
	which sets the vectorcall field of a given:c:type:`PyFunctionObject`.

Doc/whatsnew/3.12.rst Outdated

		* Add new function:c:func:`PyFunction_SetVectorcall` to the C API
		which sets the vectorcall field of a given:c:type:`PyFunctionObject`
		Warning: extensions using this API must preserve the behavior
		of the unaltered function!

Copy link

Member

vstinnerJun 28, 2022

Choose a reason for hiding this comment

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

I don't think that this document is the right place to put a warning. Why this warning is not in the documentation of the function?

Misc/NEWS.d/next/C API/2022-05-03-19-35-37.gh-issue-92193.61VoFL.rst Outdated

		which sets the vectorcall field of a given:c:type:`PyFunctionObject`

		Warning: extensions using this API must preserve the behavior
		of the unaltered function!

Copy link

Member

vstinnerJun 28, 2022

Choose a reason for hiding this comment

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

Please remove this warning.

Misc/NEWS.d/next/C API/2022-05-03-19-35-37.gh-issue-92193.61VoFL.rst Outdated

		@@ -0,0 +1,5 @@
		Add new function:c:func:`PyFunction_SetVectorcall` to the C API
		which sets the vectorcall field of a given:c:type:`PyFunctionObject`

Copy link

Member

vstinnerJun 28, 2022

Choose a reason for hiding this comment

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

Suggested change

	which sets the vectorcall field of a given:c:type:`PyFunctionObject`
	which sets the vectorcall field of a given:c:type:`PyFunctionObject`.

Python/ceval.c OutdatedShow resolvedHide resolved

vstinner reviewed

Jun 28, 2022

Objects/funcobject.cShow resolvedHide resolved

Copy link

Contributor

itamaro commentedJul 27, 2022

@vstinner @markshannon thanks for the feedback. with@markshannon 's last response I think the remaining feedback is minor styling and phrasing. if that's all that left for this to be merged, I'm happy to perform these changes, but also totally ok with a core dev making these last changes when merging 😄

itamaro added5 commits

September 6, 2022 12:58

remove redundant argcount check in LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN …

60f7769

…(replace with assert) - func version check is sufficient

Add missing periods in docs

376ee75

move warning comment to the function C API docs

56ffc70

Merge branch 'main' into pyfunctionobject-set-vectorcall-field

5340e87

fix race withpythonGH-96519(removed func_version in LOAD_ATTR_GETAT…

a5e9d13

…TRIBUTE_OVERRIDDEN specialization)

Copy link

Contributor

itamaro commentedSep 6, 2022

pushed a fix toLOAD_ATTR_GETATTRIBUTE_OVERRIDDEN to deopt on function version instead of argcount

@markshannon I also reviewed the interpreter loop and didn't found any other cases that inline calls without deopting on function version. the methodology I used is searching for occurrences ofinlined_py_calls - let me know if this should cover everything!

(btw, I noticed thatBINARY_SUBSCR_GETITEM containsCALL_STAT_INC(inlined_py_calls) twice - is that intentional?)

Fidget-Spinner reviewed

Copy link

Member

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 parts touching the specializer and specialized opcodes LGTM.

Copy link

Member

Fidget-Spinner commentedSep 7, 2022

@itamaro

(btw, I noticed that BINARY_SUBSCR_GETITEM contains CALL_STAT_INC(inlined_py_calls) twice - is that intentional?)

No that's wrong.

markshannon reviewed

Copy link

Member

markshannon left a comment

Choose a reason for hiding this comment

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

One minor formatting nit. Otherwise looks good.

Python/ceval.c OutdatedShow resolvedHide resolved

PEP-7 formatting

12faf52

Copy link

Contributor

itamaro commentedSep 7, 2022

(btw, I noticed that BINARY_SUBSCR_GETITEM contains CALL_STAT_INC(inlined_py_calls) twice - is that intentional?)
No that's wrong.

I'll file a separate issue for that

itamaro mentioned this pull request

BINARY_SUBSCR_GETITEM has duplicate CALL_STAT_INC(inlined_py_calls)#96653

Closed

Copy link

Member

markshannon commentedSep 7, 2022•
edited
Loading

All looks good to me.
@vstinner anything else before I merge this?

erlend-aasland reviewed

Modules/_testcapi/vectorcall.c OutdatedShow resolvedHide resolved

Objects/funcobject.cShow resolvedHide resolved

Python/ceval.cShow resolvedHide resolved

itamaro added2 commits

September 7, 2022 13:26

Address review feedback

6623470

- PEP-7 braces- add assert- raise TypeError

Add test for LOAD_ATTR specialization when overriding vectorcall of _…

9149f14

…_getattribute__

Fidget-Spinner reviewed

Sep 8, 2022

Copy link

Member

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 test LGTM in general but I have a few comments. Thanks!

Lib/test/test_call.py OutdatedShow resolvedHide resolved

Improve setvectorcall + specialization testing

e732d7e

1. test specialization doesn't trigger when vectorcall is already overridden2. test specialization gets deopted when vectorcall is overridden after specialization triggered

erlend-aasland approved these changes

Sep 9, 2022