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

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

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Oct 18, 2024

@vstinner
Copy link
Member

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?

Copy link
Member

@vstinner vstinner left a 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.

@pablogsal
Copy link
Member Author

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

@pablogsal pablogsal merged commit f8ba9fb into python:main Oct 18, 2024
41 checks passed
@pablogsal pablogsal deleted the gh-125703 branch October 18, 2024 16:09
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 18, 2024
…CREF paths (pythonGH-125704)

(cherry picked from commit f8ba9fb)

Co-authored-by: Pablo Galindo Salgado <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 18, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Oct 18, 2024
@pablogsal pablogsal added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Oct 18, 2024
@miss-islington-app
Copy link

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

@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 18, 2024
…CREF paths (pythonGH-125704)

(cherry picked from commit f8ba9fb)

Co-authored-by: Pablo Galindo Salgado <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 18, 2024
…CREF paths (pythonGH-125704)

(cherry picked from commit f8ba9fb)

Co-authored-by: Pablo Galindo Salgado <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Oct 18, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Oct 18, 2024
@bedevere-app
Copy link

bedevere-app bot commented Oct 18, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Oct 18, 2024
@pablogsal
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

pablogsal commented Oct 18, 2024

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
Copy link
Member Author

pablogsal commented Oct 18, 2024

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
Copy link
Member

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
Copy link
Member Author

pablogsal commented Oct 18, 2024

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
Copy link
Member Author

pablogsal commented Oct 18, 2024

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
Copy link
Member Author

pablogsal commented Oct 18, 2024

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
Copy link
Contributor

mdboom commented Oct 19, 2024

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
Copy link
Member

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
Copy link
Member Author

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.

Dec 2, 2024
…ECREF paths (GH-125704) (#125705)

gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths (GH-125704)
(cherry picked from commit f8ba9fb)

Co-authored-by: Pablo Galindo Salgado <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants