GeezerVison is showing github.com — _Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking · Issue #125703 · python/cpython
_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703DescriptionPR #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:
|
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
Activitypablogsal commented on Oct 18, 2024 pablogsal commented on Oct 18, 2024 pablogsal commented on Oct 18, 2024 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. added a commit that references this issue on Oct 18, 2024gh-125703: Correctly honour tracemalloc hooks on specialized DECREF p… markshannon commented on Oct 21, 2024 @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 commented on Oct 21, 2024
@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. Aasland vstinner commented on Oct 21, 2024 I 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 markshannon commented on Oct 21, 2024 @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 commented on Oct 21, 2024 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 commented on Oct 21, 2024
I will open a new issue where we can track these improvements for 3.14.
Opened #125790 added a commit that references this issue on Oct 21, 2024gh-125703: Correctly honour tracemalloc hooks on more PyDECREF specia… pablogsal commented 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.
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|
