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

Integrate trashcan mechanism into _Py_Dealloc #89060

Open
nascheme opened this issue Aug 12, 2021 · 3 comments
Open

Integrate trashcan mechanism into _Py_Dealloc #89060

nascheme opened this issue Aug 12, 2021 · 3 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement

Comments

@nascheme
Copy link
Member

BPO 44897
Nosy @tim-one, @nascheme, @vstinner, @pablogsal
PRs
  • bpo-44897: WIP: Integrate trashcan into _Py_Dealloc #27738
  • Files
  • pyperf-trashcan.txt
  • perf-annotate-trash.txt
  • pyperf-trashcan2.txt
  • 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 = None
    created_at = <Date 2021-08-12.06:12:33.199>
    labels = ['interpreter-core', 'type-feature']
    title = 'Integrate trashcan mechanism into _Py_Dealloc'
    updated_at = <Date 2021-08-16.19:06:19.211>
    user = 'https://github.com/nascheme'

    bugs.python.org fields:

    activity = <Date 2021-08-16.19:06:19.211>
    actor = 'nascheme'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Interpreter Core']
    creation = <Date 2021-08-12.06:12:33.199>
    creator = 'nascheme'
    dependencies = []
    files = ['50219', '50220', '50223']
    hgrepos = []
    issue_num = 44897
    keywords = ['patch']
    message_count = 3.0
    messages = ['399433', '399626', '399669']
    nosy_count = 4.0
    nosy_names = ['tim.peters', 'nascheme', 'vstinner', 'pablogsal']
    pr_nums = ['27738']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue44897'
    versions = []

    @nascheme
    Copy link
    Member Author

    This is a WIP/proof-of-concept of doing away with Py_TRASHCAN_BEGIN and Py_TRASHCAN_END and instead integrating the functionality into _Py_Dealloc. There are a few advantages:

    • all container objects have the risk of overflowing the C stack if a long reference chain of them is created and then deallocated. So, to be safe, the tp_dealloc methods for those objects should be protected from overflowing the stack.

    • the Py_TRASHCAN_BEGIN and Py_TRASHCAN_END macros are hard to understand and a bit hard to use correctly. Making the mechanism internal avoids confusion. The code can be slightly simplified as well.

    This proof-of-concept seems to pass tests but it will need some careful review. The exact rules related to calling GC Track/Untrack are subtle and this changes things a bit. I.e. tp_dealloc is called with GC objects already untracked. For 3rd party extensions, they are calling PyObject_GC_UnTrack() and so I believe they should still work.

    The fact that PyObject_CallFinalizerFromDealloc() wants GC objects to definitely be tracked is a bit of a mystery to me (there is an assert to check that). I changed the code to track objects if they are not already tracked but I'm not sure that's correct.

    There could be a performance hit, due to the _PyType_IS_GC() test that was added to the _Py_Dealloc() function. For non-GC objects, that's going to be a new branch and I'm worried it might hurt a bit. OTOH, maybe it's just in the noise. Profiling will need to be done.

    @nascheme nascheme added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement labels Aug 12, 2021
    @nascheme
    Copy link
    Member Author

    As I suspected, the performance impact is significant (although pretty small). Based on Linux perf, it looks like the extra test+branch in _Py_Dealloc adds about 1% overhead. pyperformance shows something similar, see attached reports (pypref-trashcan.txt and perf-annotate-trash.txt).

    An idea I've been trying is to add another type slot, e.g. tp_call_dealloc. It could be set by PyType_Ready(). It would be called by the Py_DECREF macro on refcnt going to zero. If the object is non-GC and Py_TRACE_REFS is off, can make tp_call_dealloc actually be the tp_dealloc pointer. If the type has the GC flag, point tp_call_dealloc to a _Py_Dealloc version that checks tp_is_gc and does the trashcan stuff.

    Unfortunately, all types don't have PyType_Ready() called on them. So, we cannot rely on tp_call_dealloc being set correctly.

    @nascheme
    Copy link
    Member Author

    Based on some testing, I think an extra type slot is not worth the extra complication. I made some small improvements to _Py_Dealloc and now the performance seems a bit better. Basically, I expanded _PyObject_IS_GC() to put the most common branches first. See pypref-trashcan2.txt for benchmark. Overall I think might be a bit slower (less than 1%?) but we have gained trashcan protection for the dealloc of all GC objects.

    If we are okay with the performance, here are other questions to answer:

    A) is it safe to untrack GC objects before calling tp_dealloc?

    B) is it safe to have PyObject_CallFinalizerFromDealloc() track objects that are resurrected?

    We might have to look at 3rd party extensions to decide on those. I.e. maybe in theory it is not safe but in practice there is no extension code that would actually break.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant