Skip to content

gh-87613: Argument Clinic vectorcall decorator - #145381

Open
cmaloney wants to merge 27 commits into
python:mainfrom
cmaloney:ac_add_vectorcall
Open

gh-87613: Argument Clinic vectorcall decorator#145381
cmaloney wants to merge 27 commits into
python:mainfrom
cmaloney:ac_add_vectorcall

Conversation

@cmaloney

@cmaloneycmaloney commented Mar 1, 2026

Copy link
Copy Markdown
Contributor

Add @vectorcall as a decorator to Argument Clinic (AC) which emits a Vectorcall Protocol argument parsing C function named {type}_vectorcall. This is only supported for __new__ and __init__ currently to simplify implementation.

The generated code has similar or better performance to existing hand-written cases for list, float, str, tuple, enumerate, reversed, and int. Using the decorator on bytearray, which has no handwritten case, construction got 1.09x faster. For more benchmark details see #87613 (comment).

The @vectorcall decorator has two options:

  • zero_arg={C_FUNC}: Some types, like int, can be called with zero arguments and return an immortal object in that case. Adding a shortcut is needed to match existing hand-written performance; provides an over 10% performance change for those cases. -- removed in refactoring, delegated to the new/init implementation.
  • exact_only: If the type is not an exact match delegate to the existing non-vectorcall implementation. Needed for str to get matching performance while ensuring correct behavior.

Implementation details:

  • Adds support for the new decorator with arguments in the AC DSL Parser
  • Move keyword argument parsing generation from inline to a function so both vectorcall, vc_, and existing can share code generation.
  • Adds an emit helper to simplify code a bit from existing AC cases

Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com

Add `@vectorcall` as a decorator to Argument Clinic (AC) which generates a new
[Vectorcall Protocol](https://docs.python.org/3/c-api/call.html#the-vectorcall-protocol)
argument parsing C function named `{}_vectorcall`. This is only supported for
`__new__` and `__init__` currently to simplify implementation.
The generated code has similar or better performance to existing hand-written
cases for `list`, `float`, `str`, `tuple`, `enumerate`, `reversed`, and `int`.
Using the decorator added vectorcall to `bytearray` and construction got
1.09x faster. For more details see the comments in pythongh-87613.
The `@vectorcall` decorator has two options:
- **zero_arg={C_FUNC}**: Some types, like `int`, can be called with zero
arguments and return an immortal object in that case. Adding a shortcut is
needed to match existing hand-written performance; provides an over 10%
performance change for those cases.
- **exact_only**: If the type is not an exact match delegate to the existing
non-vectorcall implementation. NEeded for `str` to get matching performance
while ensuring correct behavior.
Implementation details:
- Adds support for the new decorator with arguments in the AC DSL Parser
- Move keyword argument parsing generation from inline to a function so both
vectorcall, `vc_`, and existing can share code generation.
- Adds an `emit` helper to simplify code a bit from existing AC cases
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cmaloneycmaloney changed the title gh-87613: Argument Cliic @vectorcall decoratorgh-87613: Argument Clinic @vectorcall decoratorMar 1, 2026
@cmaloneycmaloney added performance Performance or resource usage and removed performance Performance or resource usage labels Mar 1, 2026
@cmaloneycmaloney changed the title gh-87613: Argument Clinic @vectorcall decoratorgh-87613: Argument Clinic vectorcall decoratorMar 1, 2026

@corona10corona10 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you replace current hand-written with your new DSL.

Let's see how handle them.

@cmaloney

Copy link
Copy Markdown
ContributorAuthor

I have commits to do that in my draft branch (https://github.com/python/cpython/compare/main...cmaloney:cpython:ac_vectorcall_v1?expand=0); can pull them into this branch if that would be easier / better to review. This generally produces code that is as fast or faster than the hand-written ones currently (full benchmarking in: #87613 (comment))

@cmaloney

cmaloney commented Mar 1, 2026

Copy link
Copy Markdown
ContributorAuthor

Added commits moving enum.c (reversed, enumerate) and tuple to the new decorator. enum.c had comments pointing to this issue and covers positional + keyword arguments. tuple uses the "zero arg" optimization and has no keyword args. None of those cases use the __init__ code; only cases of that are the new bytearray or list which is otherwise very similar to tuple. Hoping those serve as a good sample for what the code generation looks like relative to the handwritten while iterating; happy to include more in this PR if desired.

Comment threadObjects/clinic/enumobject.c.h Outdated
@corona10

Copy link
Copy Markdown
Member

I will take a look at this PR til end of this week.

@skirpichev

Copy link
Copy Markdown
Member

happy to include more in this PR if desired.

I would like to suggest you at least to think about complexobject.c. Based on benchmarks for the float pr (#22432) I would expect a good performance boost (maybe not 1.5x, but more than from freelist addition).

Yes, this case seems to be already covered by the enum.c example (kwargs).

On another hand, the complex class has special hacks to support multiple signatures (complex('123') is allowed, while complex(real='123') - not). Maybe it's not the only case, but I can't find quickly others across the CPython codebase. I suspect that AC magic will not work in this case and we will need some workarounds somewhere (well, maybe just one hand-written case). Though, it would be great if you disprove this hypothesis.

This generally produces code that is as fast or faster than the hand-written ones currently (full benchmarking in: #87613 (comment))

Still, there are some regressions, e.g. int(str). Could you explain this difference?

I also suggest you to try pyperformance on this.

@cmaloney

Copy link
Copy Markdown
ContributorAuthor

I would like to suggest you at least to think about complexobject.c. Based on benchmarks for the float pr (#22432) I would expect a good performance boost (maybe not 1.5x, but more than from freelist addition).

Will implemnt it in my draft branch this week. As part of developing this PR I added Vectorcall Protocol support to bytes (cmaloney@f5c7b7c) and bytearray (cmaloney@7de2ab7). With just two small changes: 1. add @vectorcall, 2. set .tp_vectorcall construction is 1.09x to 1.23x faster. Multiply that speedup across the many AC implemented types without vectorcall construction and I definitely get excited.

Still, there are some regressions, e.g. int(str). Could you explain this difference?

The int hand written vectorcall implementation, long_vectorcall, is a particularly elegant switch:

long_vectorcall(PyObject*type, PyObject*const*args,
size_tnargsf, PyObject*kwnames)
{
Py_ssize_tnargs=PyVectorcall_NARGS(nargsf);
if (kwnames!=NULL) {
PyThreadState*tstate=PyThreadState_GET();
return_PyObject_MakeTpCall(tstate, type, args, nargs, kwnames);
}
switch (nargs) {
case0:
return_PyLong_GetZero();
case1:
returnPyNumber_Long(args[0]);
case2:
returnlong_new_impl(_PyType_CAST(type), args[0], args[1]);
default:
returnPyErr_Format(PyExc_TypeError,
"int expected at most 2 arguments, got %zd",
nargs);
}
}

That switch specializes 1-argument to call PyNumber_Long instead of long_new_impl which matches a very similar performance delta I investigated yesterday in the hand written vectorcall for str. Worried the hand written is faster because the compiler optimizer is doing clever things around the switch form. Adding support for a one_arg special case will need more code in the AC implementation to handle. Overall not sure it's actually worth replacing the hand written int vectorcall with an AC generated version for.

I'm comparing to the handwritten because I want @vectorcall when people try it out on a type they care about to be as good as I can get it. Adding to two types without vectorcall construction, bytes and bytearray, it provides a measurable speedup for a two-line code change. I think correctness of generated code, maintainability of the new decorator, and providing a speedup for types with no vectorcall today is a lot of benefit even if it's not quite as fast as hand written expert code. If adopting the new decorator on an AC type is really low-cost for a significant performance gain that will lead to speedy adoption and a speedier CPython.

I also suggest you to try pyperformance on this.

Will run on this PR as it exists currently.

I can also run on my draft branch but not sure that will give a clear signal as it migrates every hand-written vectorcall even if it makes them slower. Ideally to me would be able to figure out what types are commonly constructed in pyperformance benchmarks so I can make a draft branch adding vectorcall support to those. Not sure what would be the most important set of types to migrate before running pyperformance.

@erlend-aasland

Copy link
Copy Markdown
Contributor

Did you consider adding this implicitly if supported, instead of making it opt-in? Disclaimer: I didn't take a look at the implementation yet.

Comment threadTools/clinic/libclinic/dsl_parser.py Outdated
Comment on lines +305 to +307
self.vectorcall = False
self.vectorcall_exact_only = False
self.vectorcall_zero_arg = ''

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if we should collect these in a "vectorcall config dataclass". The stuff in this file is already so cluttered with tons of class members and local variables.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm not sure it would help readability much for introducing a new to this bit of code pattern.

Would be really nice to refactor the decorator parsing -> argument functions (at_*), they are really repetative to me at the moment; would be nice to not have to do custom key=value parsing in the new at_vectorcall.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Updated to use a dataclass for this. None now indicates not decorated, then can check individual elements if non-None

@skirpichev

skirpichev commented Mar 2, 2026

Copy link
Copy Markdown
Member

Multiply that speedup across the many AC implemented types without vectorcall construction and I definitely get excited.

Yes, I hope that can mitigate speed regression with Decimal's since v3.13. Edit: no, this doesn't help too much.

Overall not sure it's actually worth replacing the hand written int vectorcall with an AC generated version for.

In any case, this will happen on case-by-case basis (this pr should include minimal set of such examples). BTW, here my results for this pr + c14c173.

Benchmark code: cmaloney@ddcd3b6, run with --rigorous
Configure options: ./configure --enable-optimizations --with-lto --with-static-libpython
Host: 64-bit Debian (Trixie)
Compiler: GCC 14.2.0

Benchmarkref-lto-pr-benchpatch-lto-pr-bench
list(tuple)2.23 us2.20 us: 1.01x faster
list(range)9.69 us9.65 us: 1.00x faster
float(int)1.04 us1.04 us: 1.00x faster
str()557 ns563 ns: 1.01x slower
str(int)2.16 us2.13 us: 1.01x faster
str(bytes,enc)2.19 us2.21 us: 1.01x slower
bytes()832 ns910 ns: 1.09x slower
bytes(int)2.59 us2.58 us: 1.00x faster
bytearray(bytes)3.46 us3.44 us: 1.01x faster
bytearray(int)1.73 us1.76 us: 1.02x slower
tuple()541 ns531 ns: 1.02x faster
int(str)1.35 us1.37 us: 1.02x slower
int(str,base)1.78 us1.77 us: 1.01x faster
enumerate(list,start)2.70 us2.79 us: 1.03x slower
Geometric mean(ref)1.00x slower

Benchmark hidden because not significant (9): list(), list_subclass, float(), float(str), bytearray(), tuple(list), int(), reversed(list), enumerate(list)

With default ./configure:

Benchmarkref-pr-benchpatch-pr-bench
list(tuple)2.74 us2.66 us: 1.03x faster
list_subclass4.42 us4.22 us: 1.05x faster
float(str)2.61 us2.57 us: 1.02x faster
str(int)3.05 us2.98 us: 1.02x faster
bytes()991 ns992 ns: 1.00x slower
bytes(int)3.08 us3.13 us: 1.02x slower
bytearray()1.93 us1.88 us: 1.03x faster
bytearray(bytes)4.52 us4.24 us: 1.07x faster
bytearray(int)2.20 us2.08 us: 1.06x faster
tuple()559 ns544 ns: 1.03x faster
int()556 ns538 ns: 1.03x faster
int(str)1.80 us1.78 us: 1.01x faster
int(str,base)2.44 us2.45 us: 1.00x slower
enumerate(list)2.96 us2.97 us: 1.00x slower
enumerate(list,start)3.32 us3.27 us: 1.01x faster
Geometric mean(ref)1.01x faster
BTW, I wonder how noisy your benchmarks, here an alternative approach with bench_func().
# vectorcall-bench.pyimportpyperfrunner=pyperf.Runner()
bench_cases= ['1<<7', '1<<38', '1<<300', '1<<3000']
forcinbench_cases: # XXX: bigger samplei=eval(c)
bn=f'int({c})'runner.bench_func(bn, int, i)
forcinbench_cases:
i=eval(c)
s=str(i)
bn=f'int({c!r})'runner.bench_func(bn, int, s)

As before, all optimizations:

Benchmarkref-ltopatch-lto
int(1<<7)122 ns125 ns: 1.03x slower
int(1<<38)123 ns126 ns: 1.02x slower
int(1<<300)123 ns126 ns: 1.02x slower
int(1<<3000)123 ns126 ns: 1.02x slower
int('1<<7')197 ns199 ns: 1.01x slower
int('1<<38')319 ns301 ns: 1.06x faster
int('1<<300')922 ns902 ns: 1.02x faster
int('1<<3000')18.5 us18.5 us: 1.00x faster
Geometric mean(ref)1.00x slower

Default:

Benchmarkrefpatch
int(1<<7)121 ns128 ns: 1.05x slower
int(1<<38)124 ns129 ns: 1.04x slower
int(1<<300)123 ns129 ns: 1.06x slower
int(1<<3000)123 ns129 ns: 1.05x slower
int('1<<7')229 ns246 ns: 1.08x slower
int('1<<38')364 ns368 ns: 1.01x slower
Geometric mean(ref)1.04x slower

I think correctness of generated code, maintainability of the new decorator, and providing a speedup for types with no vectorcall today is a lot of benefit even if it's not quite as fast as hand written expert code.

I agreed. But if auto-generated code catch major patterns for current hand-written functions - it will be great.

I can also run on my draft branch

No, I don't think it does make much sense with a lot of conversions to AC magic in one shot.

@cmaloney

cmaloney commented Mar 4, 2026

Copy link
Copy Markdown
ContributorAuthor

Did you consider adding this implicitly if supported, instead of making it opt-in? Disclaimer: I didn't take a look at the implementation yet.

My leaning is explicit at least to start. I think that provides a good path for gradual adoption / testing / rollout (hopefully in the 3.15 timeframe). I'd really like at least an alpha which reaches wider testing with a couple common types (ex. bytes) moved to make sure there aren't unanticipated tradeoffs or issues.

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it be possible to add a @vectorcall test to Modules/_testclinic.c?

@vstinner

Copy link
Copy Markdown
Member

This PR is very promising! Great work.

@cmaloney

Copy link
Copy Markdown
ContributorAuthor

Added a vectorcall test to the _testclinic module and a new VectorcallFunctionalTest which exercises the emitted __init__ and __new__

Debating if a hypothesis test of "does this parse the right args + kwargs" would provide value for the complexity.

@cmaloney

Copy link
Copy Markdown
ContributorAuthor

Full pyperformance numbers on the current PR below. No cases stand out to me / all seems to be within noise for my machine. That is enumerate, reversed, and tuple don't seem to change performance going from hand written to the auto-generated as expected.

Not sure what will make this easier to review. I am happy to resolve the merge conflict anytime but also don't want to disturb in-process reviews. I am thinking of scoping this down to just _testclinic + the core implementation to reduce risk. Then there should be no performance (or behavior) changes in existing code as this just adds support and tests to AC but no user visible usage. With that setup can add to one type at a time in very focused PRs with micro benchmarks + pyperformance.

pyperformance comparison: 3484ef6 (just before) vs HEAD (vectorcall clinic)

Platform: Linux-6.19.9-arch1-1-x86_64-with-glibc2.43 | 32 logical CPUs
Baseline:3484ef60 (2026-03-21 19:46–20:31) | Changed: HEAD 3698a32d (2026-03-21 20:34–21:20)

BenchmarkBaselineHEADChangeSignificance
2to3136 ms135 ms1.01x fasterNot significant
async_generators195 ms193 ms1.01x fasterNot significant
async_tree_cpu_io_mixed273 ms275 ms1.01x slowerNot significant
async_tree_cpu_io_mixed_tg279 ms278 ms1.00x fasterNot significant
async_tree_eager50.3 ms50.1 ms1.00x fasterNot significant
async_tree_eager_cpu_io_mixed217 ms218 ms1.00x slowerNot significant
async_tree_eager_cpu_io_mixed_tg255 ms255 ms1.00x slowerNot significant
async_tree_eager_io337 ms331 ms1.02x fasterNot significant
async_tree_eager_io_tg358 ms356 ms1.00x fasterNot significant
async_tree_eager_memoization123 ms123 ms1.00x slowerNot significant
async_tree_eager_memoization_tg166 ms165 ms1.01x fasterNot significant
async_tree_eager_tg120 ms122 ms1.01x slowerNot significant
async_tree_io347 ms356 ms1.03x slowerNot significant
async_tree_io_tg358 ms361 ms1.01x slowerNot significant
async_tree_memoization180 ms180 ms1.00x slowerNot significant
async_tree_memoization_tg187 ms189 ms1.01x slowerNot significant
async_tree_none146 ms145 ms1.00x fasterNot significant
async_tree_none_tg147 ms150 ms1.02x slowerSignificant (t=-2.03)
asyncio_tcp162 ms162 ms1.00x fasterNot significant
asyncio_tcp_ssl573 ms574 ms1.00x slowerNot significant
asyncio_websockets344 ms343 ms1.00x fasterNot significant
bench_mp_pool6.27 ms6.30 ms1.00x slowerNot significant
bench_thread_pool835 us840 us1.01x slowerNot significant
bpe_tokeniser2.10 sec2.10 sec1.00x slowerNot significant
chameleon6.97 ms6.96 ms1.00x fasterNot significant
chaos27.0 ms26.6 ms1.02x fasterNot significant
comprehensions7.35 us7.30 us1.01x fasterNot significant
connected_components308 ms311 ms1.01x slowerNot significant
coroutines12.0 ms12.1 ms1.01x slowerNot significant
coverage33.2 ms33.6 ms1.01x slowerNot significant
create_gc_cycles1.18 ms1.17 ms1.01x fasterNot significant
crypto_pyaes35.0 ms34.9 ms1.00x fasterNot significant
dask417 ms418 ms1.00x slowerNot significant
deepcopy109 us108 us1.00x fasterNot significant
deepcopy_memo12.6 us12.6 us1.00x slowerNot significant
deepcopy_reduce1.28 us1.29 us1.01x slowerNot significant
deltablue1.58 ms1.57 ms1.01x fasterNot significant
django_template17.4 ms17.4 ms1.00x slowerNot significant
docutils1.18 sec1.18 sec1.00x fasterNot significant
dulwich_log19.5 ms19.4 ms1.00x fasterNot significant
fannkuch180 ms178 ms1.01x fasterNot significant
float34.7 ms34.4 ms1.01x fasterNot significant
gc_traversal3.00 ms2.84 ms1.06x fasterSignificant (t=7.11)
generators14.3 ms14.1 ms1.02x fasterNot significant
genshi_text11.1 ms11.0 ms1.00x fasterNot significant
genshi_xml24.6 ms24.6 ms1.00x slowerNot significant
go55.2 ms54.3 ms1.02x fasterNot significant
hexiom2.83 ms2.86 ms1.01x slowerNot significant
html5lib23.4 ms23.4 ms1.00x slowerNot significant
json_dumps4.53 ms4.48 ms1.01x fasterNot significant
json_loads11.5 us11.4 us1.01x fasterNot significant
k_core1.38 sec1.38 sec1.00x fasterNot significant
logging_format3.30 us3.25 us1.02x fasterNot significant
logging_silent45.0 ns45.0 ns1.00x fasterNot significant
logging_simple3.03 us3.01 us1.01x fasterNot significant
mako6.18 ms6.29 ms1.02x slowerNot significant
many_optionals347 us343 us1.01x fasterNot significant
mdp570 ms566 ms1.01x fasterNot significant
meteor_contest48.1 ms47.8 ms1.01x fasterNot significant
nbody47.1 ms46.4 ms1.01x fasterNot significant
nqueens39.8 ms39.9 ms1.00x slowerNot significant
pathlib9.32 ms9.41 ms1.01x slowerNot significant
pickle5.72 us5.67 us1.01x fasterNot significant
pickle_dict12.4 us12.4 us1.00x slowerNot significant
pickle_list1.92 us1.87 us1.03x fasterSignificant (t=7.32)
pickle_pure_python144 us143 us1.01x fasterNot significant
pidigits112 ms112 ms1.00x slowerNot significant
pprint_pformat722 ms722 ms1.00x slowerNot significant
pprint_safe_repr357 ms356 ms1.00x fasterNot significant
pyflate210 ms212 ms1.01x slowerNot significant
python_startup7.63 ms7.65 ms1.00x slowerNot significant
python_startup_no_site4.66 ms4.66 ms1.00x slowerNot significant
raytrace125 ms123 ms1.02x fasterNot significant
regex_compile49.5 ms49.1 ms1.01x fasterNot significant
regex_dna94.0 ms93.0 ms1.01x fasterNot significant
regex_effbot1.72 ms1.58 ms1.08x fasterSignificant (t=19.11)
regex_v812.2 ms11.6 ms1.06x fasterSignificant (t=8.31)
richards20.9 ms20.9 ms1.00x slowerNot significant
richards_super24.1 ms24.3 ms1.01x slowerNot significant
scimark_fft154 ms150 ms1.02x fasterSignificant (t=3.69)
scimark_lu55.5 ms54.4 ms1.02x fasterSignificant (t=4.09)
scimark_monte_carlo31.2 ms31.5 ms1.01x slowerNot significant
scimark_sor54.8 ms54.8 ms1.00x slowerNot significant
scimark_sparse_mat_mult2.63 ms2.58 ms1.02x fasterNot significant
shortest_path322 ms324 ms1.01x slowerNot significant
spectral_norm44.8 ms45.0 ms1.00x slowerNot significant
sphinx454 ms454 ms1.00x slowerNot significant
sqlalchemy_declarative51.3 ms52.5 ms1.02x slowerSignificant (t=-6.55)
sqlalchemy_imperative5.07 ms5.09 ms1.00x slowerNot significant
sqlglot_v2_normalize49.7 ms49.4 ms1.01x fasterNot significant
sqlglot_v2_optimize24.2 ms24.2 ms1.00x slowerNot significant
sqlglot_v2_parse565 us559 us1.01x fasterNot significant
sqlglot_v2_transpile706 us704 us1.00x fasterNot significant
sqlite_synth1.12 us1.12 us1.00x slowerNot significant
subparsers4.96 ms4.91 ms1.01x fasterNot significant
sympy_expand186 ms184 ms1.01x fasterNot significant
sympy_integrate8.64 ms8.64 ms1.00x slowerNot significant
sympy_str106 ms105 ms1.00x fasterNot significant
sympy_sum56.4 ms57.0 ms1.01x slowerNot significant
telco3.34 ms3.44 ms1.03x slowerSignificant (t=-5.11)
tomli_loads976 ms981 ms1.00x slowerNot significant
tornado_http58.2 ms58.2 ms1.00x fasterNot significant
typing_runtime_protocols73.5 us74.9 us1.02x slowerNot significant
unpack_sequence24.6 ns25.0 ns1.02x slowerNot significant
unpickle7.23 us7.26 us1.00x slowerNot significant
unpickle_list2.16 us2.24 us1.03x slowerSignificant (t=-8.00)
unpickle_pure_python105 us102 us1.04x fasterSignificant (t=12.01)
xdsl_constant_fold17.2 ms17.3 ms1.01x slowerNot significant
xml_etree_generate38.0 ms38.6 ms1.02x slowerNot significant
xml_etree_iterparse45.4 ms46.0 ms1.01x slowerNot significant
xml_etree_parse76.3 ms78.3 ms1.03x slowerSignificant (t=-7.06)
xml_etree_process28.1 ms28.2 ms1.00x slowerNot significant

@cmaloneycmaloney removed the stale Stale PR or inactive for long period of time. label May 19, 2026
Comment threadModules/_testclinic.c

/* Forward declarations for vectorcall exemplar types, needed because
* clinic/_testclinic.c.h is included before the type definitions. */
static PyTypeObject VcNew_Type;

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Created gh-150097 to track that we shouldn't need to do this

Comment threadTools/clinic/libclinic/parse_args.py

@eendebakpteendebakpt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Three minor nits, but this looks like a nice improvement.

Comment threadTools/c-analyzer/cpython/globals-to-fix.tsv Outdated
Comment threadObjects/clinic/enumobject.c.h Outdated
Comment threadTools/clinic/libclinic/parse_args.py Outdated
int _result = {c_basename}_impl({vc_impl_arguments});
{unlock}
if (_result != 0) {{
Py_DECREF(self);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This path is not tested (claude reports some more missing coverage). I checked the generated output and it looks good.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Coverage of this got dropped when I removed the zero_arg case. Adding a new test class for keyword only type which will cover

Comment threadTools/clinic/libclinic/parse_args.py Outdated
 - kwnames != NULL to remove need for label
- Unify prototypes
- Remove unused parameter
- Restructure functions which contained ifs to be two functions or
preferrably use a common unified prototype.
- Make it so vectorcall requires fast converters
- Add test for unsupported converter
- Link to more things in NEWS
- Replace fast and slow with more descriptive terminology
""",
indent=4))
elif self.min_pos or max_args != NO_VARARG:
self.codegen.add_include('pycore_modsupport.h',

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is hoisted to _check_positional which both non-vectorcall and vectorcall use

@cmaloney

Copy link
Copy Markdown
ContributorAuthor

Substantialy reworked The parse_args.py code generation code to be much closer to existing non-vectorcall structure and a lot less weirdly intertwined. In the process also cleaned up comments and simplified the generated code a bit (slightly less labels and gotos)

PyObject *a;
PyObject *b = Py_None;

if (_PyType_CAST(type) != &VcNewExact_Type) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This does not seem to happen in any of the paths in this PR. Could we change this into an assert?

VECTORCALL_FINALE_MARKERS_INIT: Final[dict[str, str]] = {
"init_declarations": "PyObject *self;\nint _result;",
"self_alloc": libclinic.normalize_snippet("""
self = _PyType_CAST(type)->tp_alloc(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
self=_PyType_CAST(type)->tp_alloc(
assert(_PyType_CAST(type)->tp_new==PyType_GenericNew);
self=_PyType_CAST(type)->tp_alloc(

}
VECTORCALL_DELEGATE_MARKERS_INIT: Final[dict[str, str]] = {
"self_alloc": libclinic.normalize_snippet("""
self = _PyType_CAST(type)->tp_alloc(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
self=_PyType_CAST(type)->tp_alloc(
assert(_PyType_CAST(type)->tp_new==PyType_GenericNew);
self=_PyType_CAST(type)->tp_alloc(

Generated vectorcall replaces tp_new with a bare tp_alloc, which is only correct when tp_new is PyType_GenericNew.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@cmaloney@corona10@skirpichev@erlend-aasland@vstinner@eendebakpt@kumaraditya303