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-101430: Update tracemalloc to handle presize properly. #101745

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Feb 9, 2023

@corona10
Copy link
Member Author

corona10 commented Feb 9, 2023

AS-IS

# ./python.exe -Wd -X tracemalloc=2 gh-101430.py
/Users/user/oss/cpython/gh-101430.py:5: ResourceWarning: Uh oh
  warnings.warn("Uh oh", ResourceWarning, source=self)

TO-BE

# ./python.exe -Wd -X tracemalloc=2 gh-101430.py
/Users/user/oss/cpython/gh-101430.py:5: ResourceWarning: Uh oh
  warnings.warn("Uh oh", ResourceWarning, source=self)
Object allocated at (most recent call last):
  File "/Users/user/oss/cpython/gh-101430.py", lineno 10
    func()
  File "/Users/user/oss/cpython/gh-101430.py", lineno 8
    m = MyClass()

@corona10
Copy link
Member Author

corona10 commented Feb 9, 2023

I now read all the spec of the preheader from #29879.
Looks like preheader should be considered when the PyType_IS_GC is true.

@corona10
Copy link
Member Author

corona10 commented Feb 9, 2023

@colesbury PTAL

@@ -1405,7 +1406,8 @@ _tracemalloc__get_object_traceback(PyObject *module, PyObject *obj)

type = Py_TYPE(obj);
if (PyType_IS_GC(type)) {
ptr = (void *)((char *)obj - sizeof(PyGC_Head));
const size_t presize = _PyType_PreHeaderSize(type);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct, but I think it would be more straightforward if you skip the PyType_IS_GC check (and always subtract the pre-header size.) _PyType_PreHeaderSize returns zero for non-GC types.

For context, here's an example in _PyType_AllocNoTrack that operates on GC and non-GC types and unconditionally adds the pre-header size:

cpython/Objects/typeobject.c

Lines 1300 to 1301 in ecfd2d3

const size_t presize = _PyType_PreHeaderSize(type);
char *alloc = PyObject_Malloc(size + presize);

@colesbury
Copy link
Contributor

@corona10 There's also one more tracemalloc related use of sizeof(PyGC_Head) that I missed in the bug report (_PyMem_DumpTraceback calls into tracemalloc):

cpython/Objects/object.c

Lines 2390 to 2398 in ecfd2d3

void *ptr;
PyTypeObject *type = Py_TYPE(obj);
if (_PyType_IS_GC(type)) {
ptr = (void *)((char *)obj - sizeof(PyGC_Head));
}
else {
ptr = (void *)obj;
}
_PyMem_DumpTraceback(fileno(stderr), ptr);

@corona10
Copy link
Member Author

corona10 commented Feb 9, 2023

@corona10 There's also one more tracemalloc related use of sizeof(PyGC_Head) that I missed in the bug report (_PyMem_DumpTraceback calls into tracemalloc):

Thank you for catching all of them!

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is good, but I've looped in @vstinner for an extra pair of expert eyes on this.

@corona10
Copy link
Member Author

corona10 commented Feb 9, 2023

@gpshead

AFAIK, Victor is enjoying his detoxing for OSS.
I will merge the PR since there looks like no serious issue.

cc @vstinner
If there needs something to fix, please let me know :)

@corona10 corona10 merged commit 5b946d3 into python:main Feb 9, 2023
@corona10 corona10 deleted the gh-101430 branch February 9, 2023 23:30
carljm added a commit to carljm/cpython that referenced this pull request Feb 10, 2023
* main:
  Fix some typos in asdl_c.py (pythonGH-101757)
  pythongh-101747: Fix refleak in new `OrderedDict` repr (pythonGH-101748)
  pythongh-101430: Update tracemalloc to handle presize properly. (pythongh-101745)
  pythonGH-101228: Fix typo in docstring for read method of `_io.TextIOWrapper` class (python#101227)
  Fix typo in `test_fstring.py` (python#101600)
  pythongh-101726: Update the OpenSSL version to 1.1.1t (pythonGH-101727)
  pythongh-101283: Fix 'versionchanged' for the shell=True fallback on Windows in 3.12 (pythonGH-101728)
  LibFFI build requires x64 Cygwin, and skip the ARM build (pythonGH-101743)
@vstinner
Copy link
Member

vstinner commented Apr 5, 2023

Thanks for this nice fix @corona10! _PyType_PreHeaderSize() is convenient and better than previously copied-pasted code ;-)

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.

5 participants