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

Py_LIMITED_API needs a PyType_GenericDealloc #61364

Closed
bfroehle mannequin opened this issue Feb 8, 2013 · 14 comments
Closed

Py_LIMITED_API needs a PyType_GenericDealloc #61364

bfroehle mannequin opened this issue Feb 8, 2013 · 14 comments
Labels
type-feature A feature request or enhancement

Comments

@bfroehle
Copy link
Mannequin

bfroehle mannequin commented Feb 8, 2013

BPO 17162
Nosy @loewis, @vstinner, @larryhastings
Files
  • getslot.diff
  • getslot2.diff
  • 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:

    assignee = None
    closed_at = <Date 2014-02-04.08:33:50.798>
    created_at = <Date 2013-02-08.18:27:15.139>
    labels = ['type-feature']
    title = 'Py_LIMITED_API needs a PyType_GenericDealloc'
    updated_at = <Date 2014-02-04.08:49:29.132>
    user = 'https://bugs.python.org/bfroehle'

    bugs.python.org fields:

    activity = <Date 2014-02-04.08:49:29.132>
    actor = 'python-dev'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-02-04.08:33:50.798>
    closer = 'loewis'
    components = []
    creation = <Date 2013-02-08.18:27:15.139>
    creator = 'bfroehle'
    dependencies = []
    files = ['33762', '33811']
    hgrepos = []
    issue_num = 17162
    keywords = ['patch']
    message_count = 14.0
    messages = ['181689', '181690', '209507', '209514', '209719', '209722', '209725', '209769', '209770', '209771', '209813', '210182', '210183', '210184']
    nosy_count = 5.0
    nosy_names = ['loewis', 'vstinner', 'larry', 'python-dev', 'bfroehle']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue17162'
    versions = ['Python 3.4', 'Python 3.5']

    @bfroehle
    Copy link
    Mannequin Author

    bfroehle mannequin commented Feb 8, 2013

    I tried to implement a custom extension type using PyType_FromSpec and Py_LIMITED_API but couldn't implement tp_dealloc:

    static void mytype_dealloc(mytypeobject *self)
    {
        // free some fields in mytypeobject
        Py_TYPE(self)->tp_free((PyObject *) self); // XXX
    }

    The line marked XXX will not compile in Py_LIMITED_API because there is no access to the fields of PyTypeObject. There doesn't seem to be any function in the API which just calls tp_free.

    I suggest the addition of an API function (to be included in Py_LIMITED_API):

    void
    PyType_GenericDealloc(PyObject *self)
    {
        Py_TYPE(self)->tp_free(self);
    }

    @bfroehle bfroehle mannequin added the type-feature A feature request or enhancement label Feb 8, 2013
    @bfroehle
    Copy link
    Mannequin Author

    bfroehle mannequin commented Feb 8, 2013

    I should mention that essentially what I'm advocating is renaming and exposing object_dealloc in Objects/typeobject.c.

    The proper name is not obvious to me... should it be PyObject_GenericDealloc since it acts on objects? Or PyType_GenericDealloc since it will be stuck into one of the PyTypeObject slots?

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 28, 2014

    I propose a more general solution: add a function PyType_GetSlot.

    @vstinner
    Copy link
    Member

    + return *(void**)(((char*)type) + slotoffsets[slot]);

    New Python versions may add new slots. What do you think of returning NULL if the slot number is higher than the maximum slot?

    It looks like "#define Py_tp_free 74" is the highest slot number since Python 3.2.

    For example, Python 3.4 has a new "tp_finalize" slot, but I don't see it in typeslots.h.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 30, 2014

    If the set of slots gets extended, extensions would have to opt-in to use the newer slots, i.e. the availability of slot numbers should be conditional at compile-time.

    Returning 0 for slots not supported in a version seems fine to me as well (after reconsideration), as this is also what you would get if you just recompiled the old type with the new Python headers (i.e. all fields added at the end are 0-initialized).

    As for slots added to 3.4: it would certainly possible to add them to the stable ABI, if we indeed trust that they are stable (i.e. won't go away until 4.0). That would have to be decided on a slot-by-slot case, preferably in individual roundup issues.

    @larryhastings
    Copy link
    Contributor

    I thought I replied to this... weird.

    Do I understand correctly that it's basically impossible to write a proper custom deallocator in the limited API right now, because you can't get access to your base class's tp_free?

    (If so, why didn't anybody notice?)

    @larryhastings
    Copy link
    Contributor

    Also, isn't it reasonable to pass in non-heap type objects? I realize supporting this would complicate the implementation a great deal.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 31, 2014

    It's certainly possible to write a "proper" dealloc - just call PyObject_Del directly. This is correct if the type isn't subclassable, and works if it actually isn't subclassed, or if the subclass object can also be released through PyObject_Del.

    That this has *nobody* noticed yet isn't the case - the OP certainly noticed a year ago. Else, the limited API isn't in wide use yet, probably partly because it is too limited still for certain extension modules (but it is by design that it is limited at all, so that code might require active porting to use it, and may not be portable at all in certain cases).

    If the typical use case is PyTYPE(self)->tp_free, then the type ought to be a heap type, so limiting the implementation to heap types should be sufficient. It would be feasible to extend it to non-heaptypes, although I do wonder what use case this would allow for.

    @larryhastings
    Copy link
    Contributor

    I was thinking of a subclass of an existing class, both implemented in C. collections.Counter does that, but it uses _PyType_LookupId() which is in the limited API.

    Would it be possible to achieve equivalent functionality by using _PyType_LookupId()? If so, maybe instead of the proposed new function PyType_GetSlot, we should make _PyType_Lookup and _PyType_LookupId part of the public API.

    @larryhastings
    Copy link
    Contributor

    (And, I freely admit I don't understand all this that well and that could be a very dumb question.)

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jan 31, 2014

    I'm not sure we are looking at the same code base, I look at

    http://hg.python.org/cpython/file/b56ce3410ca6/Lib/collections/__init__.py#l401

    and ISTM that collections.Counter is *not* implemented in C. Also, according to

    http://hg.python.org/cpython/file/b56ce3410ca6/Include/object.h#l486

    I see that _PyType_LookupId is *not* in the limited API (and it really shouldn't).

    In any case, _PyType_LookupId cannot replace PyType_GetSlot, since it returns a PyObject* from the namespace of the type. Many of the slots don't actually have a Python name (including tp_free, which is the OP's original use case), plus the value returned ought to be a function pointer, not a PyObject*.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 4, 2014

    New changeset 655d7a55c165 by Martin v. Löwis in branch 'default':
    Issue bpo-17162: Add PyType_GetSlot.
    http://hg.python.org/cpython/rev/655d7a55c165

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Feb 4, 2014

    Thanks for the reviews; this is now committed.

    @loewis loewis mannequin closed this as completed Feb 4, 2014
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 4, 2014

    New changeset eaae4008327d by Victor Stinner in branch 'default':
    Issue bpo-17162: Fix compilation, replace non-breaking space with an ASCII space
    http://hg.python.org/cpython/rev/eaae4008327d

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants