_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking

[pablogsal]

PR #30872 added _Py_DECREF_SPECIALIZED which calls destruct over the object. When destruct is PyObject_Free this skips tracemalloc counting reporting that the memory is alive. This also makes debuggers segfault because they think that the object is alive because they did not get a notification. This is the code that's not getting called:

cpython/Objects/object.c

Lines 2924 to 2928 in 2e950e3

	struct _reftracer_runtime_state *tracer = &_PyRuntime.ref_tracer;
	if (tracer->tracer_func != NULL) {
	void* data = tracer->tracer_data;
	tracer->tracer_func(op, PyRefTracer_DESTROY, data);
	}

This may also qualify a regression as makes tracemalloc detect incorrect memory usage for these objects

Linked PRs

• gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths #125704
• [3.13] gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths (GH-125704) #125705
• [3.11] gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths (GH-125704) #125706
• [3.12] gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths (GH-125704) #125707
• gh-125703: Correctly honour tracemalloc hooks on more PyDECREF specialized paths #125712
• [3.13] gh-125703: Correctly honour tracemalloc hooks on more PyDECREF specialized paths (GH-125712) #125791

[pablogsal]

CC: @sweeneyde @Yhg1s

[pablogsal]

CC: @mdboom @markshannon

[pablogsal]

I suspect that we are missing more calls to tracemalloc hooks when the memory is reused. Basically every call to _Py_ForgetReference needs to have a call to tracemalloc hooks but the ones left happen in reallocations, so I would tackle those later when this is fixed.

[pablogsal]

3.11 and 3.12 are not affected because there is no tracemalloc_remove_trace

[sweeneyde]

The macro version in ceval.c may also be affected:

cpython/Python/ceval.c

Line 93 in f8ba9fb

#define _Py_DECREF_SPECIALIZED(arg, dealloc) \

I might be misunderstanding how the macros expand, but I believe this macro is unused anyway.

[markshannon]

@pablogsal You seem to be claiming that #30872 caused a regression in a feature added in #115945.
How can an earlier change cause a regression in a feature added later?

Combined with such comments as this, it feels like you are trying to shift the blame for this issue.

[pablogsal]

@pablogsal You seem to be claiming that #30872 caused a regression in a feature added in #115945.

How can an earlier change cause a regression in a feature added later?

Combined with such comments as this, it feels like you are trying to shift the blame for this issue.

I am not shifting blame for anything. Among other things because there is no need to blame anyone. And by fixing a bug I am not blaming anyone. I am trying to fix a bug in a feature we released in 3.13. What I am complaining in that comment is that I am trying to fix a bug in 3.13 (in a new feature or in a old feature, whatever you prefer) and you are complaining about a PR that already landed, with a feature that was already decided with comments such as #125704 (comment) where you say things like "Can we please stop doing this" in a PR that's trying to fix a bug.

#30872 affects for tracemalloc AND therefore also affect the hooks I added #115945.

The regression of the feature is for tracemalloc which was added many releases ago. Tracemalloc tracks object creation and destruction to be able to call get_object_traceback(): https://docs.python.org/3/library/tracemalloc.html#tracemalloc.get_object_traceback:

Tracemalloc in 3.13 has been unified to use the hooks we added in #115945 but the regression would still be there if the hooks were not added.

---

Even if you don't want to call this a "regression", it's a bug in a feature we added to 3.13 and start complaining about the feature in the bug fix PR is not productive and that's what I am trying to say in #125704 (comment)

[vstinner]

I think that we should first make the code correct (fix tracemalloc/the callback), then make the code fast.

[markshannon]

@vstinner I don't see any value in making trite comments like "first make the code correct, then make the code fast"?

It suggests that correctness and performance are totally unrelated. Often considering performance will lead to a more streamlined implementation that can be more maintainable.

For example, if tracemalloc was entirely built on PEP 445 (as PEP 454 states it would), there would be no issue in the first place as all allocations and frees would be done through the PEP 445 function pointer(s).

[pablogsal]

Ok, so @markshannon and I had a catch up call about this. We discussed our views and we clarified our positions a bit better. Here is what we have agreed:

• Mark is concerned about how these hooks can impact performance, specially in the future once we have a JIT. One of the thinks he is concerned is that if we increase the size of _Py_DECREF_SPECIALIZED then the JIT will have a lot of repeated code to optimise and that will be bad.
• Mark thinks there is a better way to place these hooks that will have less performance impact. I will open a new issue where we can track these improvements for 3.14.
• We have agreed that for 3.13 as the JIT is experimental and pyperformance didn't show any regression above noise (noise at least defined with ~0.5% change) for the fix or the original PR then we will go with the combo of gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths #125704 and gh-125703: Correctly honour tracemalloc hooks on more PyDECREF specialized paths #125712 since this fixes the issue without introducing a lot of trouble.

[pablogsal]

I will open a new issue where we can track these improvements for 3.14.

Opened #125790

[pablogsal]

I'm closing this as the issue is fixed and the performance discussion can be moved to the new issue. Thanks a lot everyone for your comments and help.