Integrate trashcan mechanism into _Py_Dealloc

[nascheme]

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]

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]

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]

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.

[nascheme]

Replaced by new PR GH-124716