Repository navigation
_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703
Description
Activity
CC: @sweeneyde @Yhg1s
CC: @mdboom @markshannon
I suspect that we are missing more calls to tracemalloc hooks when the memory is reused. Basically every call to
_Py_ForgetReferenceneeds to have a call to tracemalloc hooks but the ones left happen in reallocations, so I would tackle those later when this is fixed.- added a commit that references this issue
on Oct 18, 2024 - added 3 commits that reference this issue
on Oct 18, 2024 7 remaining items
@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 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)
Reacted by Erlend E. AaslandI think that we should first make the code correct (fix tracemalloc/the callback), then make the code fast.
Reacted by Pablo Galindo Salgado and Erlend E. Aasland@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).
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_SPECIALIZEDthen 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.
- 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
- added a commit that references this issue
on Oct 21, 2024 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.
- added a commit that references this issue
on Dec 2, 2024
PR #30872 added
_Py_DECREF_SPECIALIZEDwhich callsdestructover the object. WhendestructisPyObject_Freethis skipstracemalloccounting 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
This may also qualify a regression as makes tracemalloc detect incorrect memory usage for these objects
Linked PRs