_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703

pablogsal · 2024-10-18T14:50:09Z

PR #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:

cpython/Objects/object.c

Lines 2924 to 2928 in 2e950e3

    
           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

The text was updated successfully, but these errors were encountered:

pablogsal · 2024-10-18T14:50:48Z

CC: @sweeneyde @Yhg1s

…CREF paths

pablogsal · 2024-10-18T15:00:52Z

CC: @mdboom @markshannon

pablogsal · 2024-10-18T15:08:54Z

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.

…aths (#125704)

…CREF paths (pythonGH-125704) (cherry picked from commit f8ba9fb) Co-authored-by: Pablo Galindo Salgado <[email protected]>

pablogsal · 2024-10-18T16:50:01Z

3.11 and 3.12 are not affected because there is no tracemalloc_remove_trace

sweeneyde · 2024-10-18T18:26:36Z

The macro version in ceval.c may also be affected:

cpython/Python/ceval.c

Line 93 in f8ba9fb

#define _Py_DECREF_SPECIALIZED(arg, dealloc) \

I might be misunderstanding how the macros expand, but I believe this macro is unused anyway.

…specialized paths

markshannon · 2024-10-21T10:21:50Z

@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 · 2024-10-21T10:27:24Z

@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)

…specialized paths

vstinner · 2024-10-21T11:52:41Z

I think that we should first make the code correct (fix tracemalloc/the callback), then make the code fast.

markshannon · 2024-10-21T13:58:12Z

@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 · 2024-10-21T14:36:10Z

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 · 2024-10-21T14:38:29Z

I will open a new issue where we can track these improvements for 3.14.

Opened #125790

…lized paths (#125712)

pablogsal · 2024-10-21T14:40:43Z

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.

…DECREF specialized paths (pythonGH-125712) (cherry picked from commit 3d1df3d) Co-authored-by: Pablo Galindo Salgado <[email protected]>

… specialized paths (GH-125712) (#125791) (cherry picked from commit 3d1df3d)

…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]>

pablogsal added the 3.13 bugs and security fixes label Oct 18, 2024

pablogsal added the 3.14 new features, bugs and security fixes label Oct 18, 2024

pablogsal added a commit to pablogsal/cpython that referenced this issue Oct 18, 2024

pythongh-125703: Correctly honour tracemalloc hooks on specialized DE…

ceb5d4c

…CREF paths

bedevere-app bot mentioned this issue Oct 18, 2024

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

Merged

pablogsal added 3.12 bugs and security fixes 3.11 only security fixes labels Oct 18, 2024

mdboom added the performance Performance or resource usage label Oct 18, 2024

pablogsal added a commit that referenced this issue Oct 18, 2024

gh-125703: Correctly honour tracemalloc hooks on specialized DECREF p…

f8ba9fb

…aths (#125704)

bedevere-app bot mentioned this issue Oct 18, 2024

[3.13] gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths (GH-125704) #125705

Merged

This was referenced Oct 18, 2024

[3.11] gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths (GH-125704) #125706

Closed

[3.12] gh-125703: Correctly honour tracemalloc hooks on specialized DECREF paths (GH-125704) #125707

Closed

pablogsal closed this as completed Oct 18, 2024

pablogsal reopened this Oct 18, 2024

pablogsal removed 3.11 only security fixes 3.12 bugs and security fixes labels Oct 18, 2024

pablogsal closed this as completed Oct 18, 2024

pablogsal reopened this Oct 18, 2024

pablogsal added a commit to pablogsal/cpython that referenced this issue Oct 18, 2024

pythongh-125703: Correctly honour tracemalloc hooks on more PyDECREF …

a9c4aa3

…specialized paths

bedevere-app bot mentioned this issue Oct 18, 2024

gh-125703: Correctly honour tracemalloc hooks on more PyDECREF specialized paths #125712

Merged

pablogsal added a commit to pablogsal/cpython that referenced this issue Oct 21, 2024

pythongh-125703: Correctly honour tracemalloc hooks on more PyDECREF …

4c1937d

…specialized paths

pablogsal mentioned this issue Oct 21, 2024

Optimise the way tracemalloc and PyRefTracer hooks work #125790

Open

pablogsal added a commit that referenced this issue Oct 21, 2024

gh-125703: Correctly honour tracemalloc hooks on more PyDECREF specia…

3d1df3d

…lized paths (#125712)

pablogsal closed this as completed Oct 21, 2024

bedevere-app bot mentioned this issue Oct 21, 2024

[3.13] gh-125703: Correctly honour tracemalloc hooks on more PyDECREF specialized paths (GH-125712) #125791

Merged

pablogsal added a commit that referenced this issue Oct 28, 2024

[3.13] gh-125703: Correctly honour tracemalloc hooks on more PyDECREF…

00b13ec

… specialized paths (GH-125712) (#125791) (cherry picked from commit 3d1df3d)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703

_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703

pablogsal commented Oct 18, 2024 •

edited by bedevere-app bot

Loading

pablogsal commented Oct 18, 2024

pablogsal commented Oct 18, 2024

pablogsal commented Oct 18, 2024 •

edited

Loading

pablogsal commented Oct 18, 2024

sweeneyde commented Oct 18, 2024

markshannon commented Oct 21, 2024

pablogsal commented Oct 21, 2024 •

edited

Loading

vstinner commented Oct 21, 2024

markshannon commented Oct 21, 2024

pablogsal commented Oct 21, 2024

pablogsal commented Oct 21, 2024

pablogsal commented Oct 21, 2024

_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703

_Py_DECREF_SPECIALIZED doesn't respect pymalloc tracking #125703

Comments

pablogsal commented Oct 18, 2024 • edited by bedevere-app bot Loading

Linked PRs

pablogsal commented Oct 18, 2024

pablogsal commented Oct 18, 2024

pablogsal commented Oct 18, 2024 • edited Loading

pablogsal commented Oct 18, 2024

sweeneyde commented Oct 18, 2024

markshannon commented Oct 21, 2024

pablogsal commented Oct 21, 2024 • edited Loading

vstinner commented Oct 21, 2024

markshannon commented Oct 21, 2024

pablogsal commented Oct 21, 2024

pablogsal commented Oct 21, 2024

pablogsal commented Oct 21, 2024

pablogsal commented Oct 18, 2024 •

edited by bedevere-app bot

Loading

pablogsal commented Oct 18, 2024 •

edited

Loading

pablogsal commented Oct 21, 2024 •

edited

Loading