gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths

[pablogsal]

• Issue: _Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703

[vstinner]

Python 3.13 is also affected, I add the "needs backport to 3.13" label.

It's unclear to me if Python 3.12 is also affected or not. Your issue has labels up to Python 3.11?

[vstinner]

LGTM.

This change makes _Py_DECREF_SPECIALIZED() behaves as _Py_Dealloc() again.

[pablogsal]

Python 3.13 is also affected, I add the "needs backport to 3.13" label.

It's unclear to me if Python 3.12 is also affected or not. Your issue has labels up to Python 3.11?

All versions are affected from 3.11 onwards

[miss-islington-app]

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

[bedevere-app]

GH-125705 is a backport of this pull request to the 3.13 branch.

[miss-islington-app]

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

[miss-islington-app]

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[bedevere-app]

GH-125706 is a backport of this pull request to the 3.11 branch.

[bedevere-app]

GH-125707 is a backport of this pull request to the 3.12 branch.

[pablogsal]

It's unclear to me if Python 3.12 is also affected or not. Your issue has labels up to Python 3.11?

Ah, I was wrong, 3.11 and 3.12 are not affected because there is no tracemalloc_remove_trace

[markshannon]

Did anyone benchmark this?
And please leave sufficient time for reviews before merging. 1 hour is clearly not enough.

According to PEP 454, tracemalloc "has no overhead when the module is not tracing memory allocations."
Can we ensure that is true, please.

[markshannon]

There seems to be a trend of adding tracing hooks without proper assessment of the performance impact, or wider discussion.
We can add hooks without performance impact, for example PEP 669.

Can we please stop doing this.
There is a discussion forum and PEP process for these changes.

[pablogsal]

There seems to be a trend of adding tracing hooks without proper assessment of the performance impact, or wider discussion.

We can add hooks without performance impact, for example PEP 669.

Can we please stop doing this.

There is a discussion forum and PEP process for these

@markshannon tracemalloc was added many release ago and these hooks use the same technology as tracemallocs do, so this but exists independently on the hooks. This functionality and therefore the restrictions exist independently of the hooks. The hooks just expose THE SAME THING to other tools other than tracemalloc with exactly the same limitations. The hooks were exposed to 3.13 following the normal process and there was no measured performance impact (we run pyperformance). The code was reviewed by other core devs and an entire relies cycle has passed.

Mark, this disrespect for debugging and profiling has to stop. It's not productive and it makes interactions much harder. You must respect that there are other people interested in things other than optimisations.

[pablogsal]

Did anyone benchmark this?

I did and saw no difference in performance. Is a predictable branch when tracemalloc is not active. This is also a bug fix so correctness takes precedence over speed. The tracemalloc functionality has been there much before we started to optimise things.

According to PEP 454, tracemalloc "has no overhead when the module is not tracing memory allocations."

Can we ensure that is true, please.

Mark this is a bigfix not a place to consider features that were added many releases ago.

[markshannon]

I am asking that you consider the performance impact of your changes and give me time for review.
I really don't see how that is "disrespect for debugging and profiling".

If anything is disrespectful it is the casual disregard for our work which these sorts of changes show.
It is immensely frustrating to put in weeks of work to streamline some key path of the VM to have extra parts casually added on that undo that effort.

I am asking that we find a way to add the features you need without unnecessary performance impacts. That is all.

This should be a collaborative effort. If debuggers and profilers need a feature, then we're happy to help find a way to add that without undue performance impact. But you need to say what you need, not just make changes then get upset when I complain.

[pablogsal]

I am asking that you consider the performance impact of your changes and give me time for review.
I really don't see how that is "disrespect for debugging and profiling".

If anything is disrespectful it is the casual disregard for our work which these sorts of changes show.

What changes? This PR is fixing a bug in an optimisation that was incorrect and was breaking an existing feature, the feature being tracemalloc.

It is immensely frustrating to put in weeks of work to streamline some key path of the VM to have extra parts casually added on that undo that effort.

That makes two of us. It is terrible frustrating to make an uphill battle to keep profilers and debuggers working when we make optimisations that don't take them into account.

I am asking that we find a way to add the features you need without unnecessary performance impacts. That is all.

Yet again I tell you that tracemalloc was added many releases ago before we started to work on the specialising interpreter. There is no new features that change how this bug must be corrected. This bug affects tracemalloc. The existence of the hooks changes nothing for this issue.

This should be a collaborative effort. If debuggers and profilers need a feature, then we're happy to help find a way to add that without undue performance impact. But you need to say what you need, not just make changes then get upset when I complain.

Yet again, there is no new features here this is a bug fix for a feature that was added long ago that now is ALSO used by more tools but is not a new feature is an existing feature of tracemalloc

[pablogsal]

But you need to say what you need, not just make changes then get upset when I complain.

I don't get upset because you complain. I get upset because you are complaining in a bug fix PR about a feature we already added to Python long ago and us exposing the same feature to 3.13. And you are complaining here and now where is not productive in any way or form.

So not only I need to spend considerable time to debug the problem and fix it but also now I need to discuss with you about something that we have already discussed, run benchmarks for and it's a done deal because is on 3.13 anyway and that even if we reverted for whatever reason doesn't change the fact that this is a bug and is breaking tracemalloc. So I need to spend a lot of extra energy discussing this and going nowhere. That's what I am upset about

[pablogsal]

There are more places where we need to fix this. I opened #125712.

@markshannon As you mentioned that you are unhappy with the fix, I will hold the PR until your review it. If you are unhappy with this fix please tell me how you would like to fix it.

[mdboom]

For what it's worth, this PR didn't seem to have a measurable effect on the benchmarks: https://github.com/faster-cpython/benchmarking-public/blob/main/results/bm-20241018-3.14.0a1%2B-f8ba9fb/bm-20241018-linux-x86_64-python-f8ba9fb2ce6690d2dd05-3.14.0a1%2B-f8ba9fb-vs-base.svg

[markshannon]

It looks like 0.5% slower from the timings chart
And this is only a partial fix.

Plus, I would expect the impact of the JIT to be worse due to the increase in size of _Py_DECREF_SPECIALIZED.

[pablogsal]

It looks like 0.5% slower from the timings chart

That's noise. I have repeated the benchmarks in my machine and the total shows as 0.48% FASTER.

In any case, I have not landed any more fixes so if you want to change the way we do this we have still time, but please, tell me what do you want to do.