Skip to content
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

Receiving vector calls in the Py_LIMITED_API #93274

Closed
wjakob opened this issue May 26, 2022 · 15 comments · Fixed by #95717
Closed

Receiving vector calls in the Py_LIMITED_API #93274

wjakob opened this issue May 26, 2022 · 15 comments · Fixed by #95717
Assignees
Labels
3.12 bugs and security fixes topic-C-API type-feature A feature request or enhancement

Comments

@wjakob
Copy link
Contributor

wjakob commented May 26, 2022

Feature or enhancement

The PyType_FromSpec family of functions provides a convenient and forward-compatible way mechanism for creating new types. It could in principle also be used to create callables providing a PEP-590 vector call interface, which has significant performance benefits for binding libraries (see the related discussion here).

One can already specify a member named __vectorcalloffset__ in PyType_FromSpec. This isn't fully working in the limited API, however. I encountered the following problems.

  1. The Py_TPFLAGS_HAVE_VECTORCALL flag is not part of the limited API.
  2. The PyVectorcall_NARGS() helper function is not part of the limited API.
  3. One would normally set tp_call to the compatibility dispatch routine PyVectorcall_Call. It is, however, also not part of the public ABI.
  4. Leaving tp_call unspecified is not an option. PyType_Ready() even throws an exception in type_ready_pre_checks() when tp_call is unspecified.

Pitch

I propose the following changes:

  1. Adding Py_TPFLAGS_HAVE_VECTORCALL, PyVectorcall_NARGS(), and PyVectorcall_Call() to the limited API.
  2. Redundant, but nice: Setting tp_call to PyVectorcall_Call when PyType_Ready encounters a type that doesn't have this field set.

Note that vector calls can be received and performed. This issue is just about the receiving end.

Previous discussion

See the discord thread https://discuss.python.org/t/ideas-for-forward-compatible-and-fast-extension-libraries-in-python-3-12/15993/12.

@wjakob wjakob added the type-feature A feature request or enhancement label May 26, 2022
@AA-Turner AA-Turner added topic-C-API 3.12 bugs and security fixes labels May 26, 2022
@encukou encukou self-assigned this Jun 7, 2022
@encukou
Copy link
Member

encukou commented Jun 7, 2022

Found one more wrinkle: we probably should disable setting __call__ from Python code for classes with vectorcall. See https://docs.python.org/3/c-api/typeobj.html#c.PyTypeObject.tp_vectorcall_offset.

@encukou
Copy link
Member

encukou commented Jun 7, 2022

@encukou encukou changed the title Receiving vector calls in the Py_LIMITED_API PEP: Receiving vector calls in the Py_LIMITED_API Jun 7, 2022
@encukou encukou changed the title PEP: Receiving vector calls in the Py_LIMITED_API Receiving vector calls in the Py_LIMITED_API Jun 7, 2022
@encukou
Copy link
Member

encukou commented Jun 7, 2022

Combined with exposing the call side of vectorcall (PyObject_Vectorcall, PyObject_VectorcallDict, PyObject_VectorcallMethod), it looks like there are enough changes here for a small PEP.

@markshannon
Copy link
Member

PEP 590 makes it clear that tp_call and the vectorcall function must be consistent.
That can be achieved two ways:

  • Make the class immutable, so that __call__ cannot be set.
  • If __call__ is set, then set the vectorcall function pointer to NULL.

@encukou
Copy link
Member

encukou commented Jun 17, 2022

The issue is that if the class is mutable, __call__ can be set from arbitrary Python code.

@markshannon
Copy link
Member

Not a problem.
Somewhere in the bowels of typeobject.c there is code that sets cls->tp_call, so

    cls->tp_call = ...

becomes

    cls->tp_call = ...
    cls->tp_vectorcall = NULL;

@encukou
Copy link
Member

encukou commented Jun 17, 2022

Setting __call__ on a class defines how the instances of that class are called.
tp_vectorcall defines how a class object is called (the tp_vectorcall_offset of a metaclass is usually set to offsetof(PyTypeObject, tp_vectorcall)).

It would need to become something like:

cls->tp_call = ...
for inst in all instances of cls:
    if isinstance(inst, type):
        inst->tp_vectorcall = NULL; 
    else:
        ???

@markshannon
Copy link
Member

Indeed, as Victor points out, it isn't tp_vectorcall that needs to change, but whether it is valid.
The code above should read:

    cls->tp_call = ...
    cls->tp_flags &= ~Py_TPFLAGS_HAVE_VECTORCALL;
    cls->tp_version = 0; /* Just to be safe? */

@markshannon
Copy link
Member

We might need to invalidate sub-classes as well, it depends on whether PyType_FromSpec allows sub-classing.

@encukou
Copy link
Member

encukou commented Jun 17, 2022

Yes, the flag needs to be unset.
Invalidation is already handled, type_setattro calls PyType_Modified.

@encukou
Copy link
Member

encukou commented Aug 5, 2022

PR: #95717

As for setting tp_call automatically, after reviewing the past discussions I prefer leaving this as is: it needs to be set it explicitly.

@erlend-aasland
Copy link
Contributor

AFAICS, this is now implemented. Unless you have further changes planned, let's close this issue.

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Aug 8, 2022
@encukou encukou closed this as completed Aug 8, 2022
@encukou
Copy link
Member

encukou commented Aug 8, 2022

Next up in this bucket is making vectorcalls. Or see Discourse for a bigger bucket.

@encukou
Copy link
Member

encukou commented Aug 8, 2022

The new tests broke buildbots. I'll revert & test more thoroughly.

@encukou
Copy link
Member

encukou commented Aug 9, 2022

Fixed in #95796

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes topic-C-API type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants