-
-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths #125704
Conversation
pablogsal
commented
Oct 18, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: _Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
This change makes _Py_DECREF_SPECIALIZED() behaves as _Py_Dealloc() again.
All versions are affected from 3.11 onwards |
Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…CREF paths (pythonGH-125704) (cherry picked from commit f8ba9fb) Co-authored-by: Pablo Galindo Salgado <[email protected]>
GH-125705 is a backport of this pull request to the 3.13 branch. |
Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11. |
Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12. |
…CREF paths (pythonGH-125704) (cherry picked from commit f8ba9fb) Co-authored-by: Pablo Galindo Salgado <[email protected]>
…CREF paths (pythonGH-125704) (cherry picked from commit f8ba9fb) Co-authored-by: Pablo Galindo Salgado <[email protected]>
GH-125706 is a backport of this pull request to the 3.11 branch. |
GH-125707 is a backport of this pull request to the 3.12 branch. |
Ah, I was wrong, 3.11 and 3.12 are not affected because there is no |
Did anyone benchmark this? According to PEP 454, tracemalloc "has no overhead when the module is not tracing memory allocations." |
There seems to be a trend of adding tracing hooks without proper assessment of the performance impact, or wider discussion. Can we please stop doing this. |
@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. |
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.
Mark this is a bigfix not a place to consider features that were added many releases ago. |
I am asking that you consider the performance impact of your changes and give me time for review. If anything is disrespectful it is the casual disregard for our work which these sorts of changes show. 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. |
What changes? This PR is fixing a bug in an optimisation that was incorrect and was breaking an existing feature, the feature being tracemalloc.
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.
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.
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 |
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 |
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. |
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 |
It looks like 0.5% slower from the timings chart Plus, I would expect the impact of the JIT to be worse due to the increase in size of |
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. |